views:

307

answers:

6

I have an object called Parameters that gets tossed from method to method down and up the call tree, across package boundaries. It has about fifty state variables. Each method might use one or two variables to control its output.

I think this is a bad idea, beacuse I can't easily see what a method needs to function, or even what might happen if with a certain combination of parameters for module Y which is totally unrelated to my current module.

What are some good techniques for decreasing coupling to this god object, or ideally eliminating it ?

        public void ExporterExcelParFonds(ParametresExecution parametres)
    {
        ApplicationExcel appExcel = null;
        LogTool.Instance.ExceptionSoulevee = false;


        bool inclureReferences = parametres.inclureReferences;
        bool inclureBornes = parametres.inclureBornes;
        DateTime dateDebut = parametres.date;
        DateTime dateFin = parametres.dateFin;

        try
        {
            LogTool.Instance.AfficherMessage(Variables.msg_GenerationRapportPortefeuilleReference);

            bool fichiersPreparesAvecSucces = PreparerFichiers(parametres, Sections.exportExcelParFonds);
            if (!fichiersPreparesAvecSucces)
            {
                parametres.afficherRapportApresGeneration = false;
                LogTool.Instance.ExceptionSoulevee = true;
            }
            else
            {

The caller would do :

                PortefeuillesReference pr = new PortefeuillesReference();
            pr.ExporterExcelParFonds(parametres);
A: 

Query each client for their required parameters and inject them?

Example: each "object" that requires "parameters" is a "Client". Each "Client" exposes an interface through which a "Configuration Agent" queries the Client for its required parameters. The Configuration Agent then "injects" the parameters (and only those required by a Client).

jldupont
Can you expand on this, maybe with an example? I think I agree with this, but I want to be sure.
Gurdas Nijor
I don't understand.
Andrei Tanasescu
I've added details.
jldupont
Pretty sweet. I'm thinking about your solution's consequences.
Andrei Tanasescu
+1  A: 

If all your methods are using the same Parameters class then maybe it should be a member variable of a class with the relevant methods in it, then you can pass Parameters into the constructor of this class, assign it to a member variable and all your methods can use it with having to pass it as a parameter.

A good way to start refactoring this god class is by splitting it up into smaller pieces. Find groups of properties that are related and break them out into their own class.

You can then revisit the methods that depend on Parameters and see if you can replace it with one of the smaller classes you created.

Hard to give a good solution without some code samples and real world situations.

Ryu
A: 

It sounds like you are not applying object-oriented (OO) principles in your design. Since you mention the word "object" I presume you are working within some sort of OO paradigm. I recommend you convert your "call tree" into objects that model the problem you are solving. A "god object" is definitely something to avoid. I think you may be missing something fundamental... If you post some code examples I may be able to answer in more detail.

SingleShot
Yeah, well it's a legacy system, and it's poorly written, I agree.
Andrei Tanasescu
Please post a code example... how you deal with this situation depends on the details.
SingleShot
A: 

(I am assuming this is within a Java or .NET environment) Convert the class into a singleton. Add a static method called "getInstance()" or something similar to call to get the name-value bundle (and stop "tramping" it around -- see Ch. 10 of "Code Complete" book).

Now the hard part. Presumably, this is within a web app or some other non batch/single-thread environment. So, to get access to the right instance when the object is not really a true singleton, you have to hide selection logic inside of the static accessor.

In java, you can set up a "thread local" reference, and initialize it when each request or sub-task starts. Then, code the accessor in terms of that thread-local. I don't know if something analogous exists in .NET, but you can always fake it with a Dictionary (Hash, Map) which uses the current thread instance as the key.

It's a start... (there's always decomposition of the blob itself, but I built a framework that has a very similar semi-global value-store within it)

Roboprog
I am trying to avoid having a singleton. I actually dislike singletons because of where I've seen it used. Using it here would be a step back.
Andrei Tanasescu
-1 the fact that you need threadlocal to reduce the "globalness" of the "singleton" indicates that you shouldn't be using globals in the first place. The OP may be talking about a single-threaded winforms app anyway.
Wim Coenen
+4  A: 

First, at the risk of stating the obvious: pass the parameters which are used by the methods, rather than the god object.

This, however, might lead to some methods needing huge amounts of parameters because they call other methods, which call other methods in turn, etcetera. That was probably the inspiration for putting everything in a god object. I'll give a simplified example of such a method with too many parameters; you'll have to imagine that "too many" == 3 here :-)

public void PrintFilteredReport(
   Data data, FilterCriteria criteria, ReportFormat format)
{
   var filteredData = Filter(data, criteria);
   PrintReport(filteredData, format);
}

So the question is, how can we reduce the amount of parameters without resorting to a god object? The answer is to get rid of procedural programming and make good use of object oriented design. Objects can use each other without needing to know the parameters that were used to initialize their collaborators:

// dataFilter service object only needs to know the criteria
var dataFilter = new DataFilter(criteria);

// report printer service object only needs to know the format
var reportPrinter = new ReportPrinter(format);

// filteredReportPrinter service object is initialized with a
// dataFilter and a reportPrinter service, but it doesn't need
// to know which parameters those are using to do their job
var filteredReportPrinter = new FilteredReportPrinter(dataFilter, reportPrinter);

Now the FilteredReportPrinter.Print method can be implemented with only one parameter:

public void Print(data)
{
   var filteredData = this.dataFilter.Filter(data);
   this.reportPrinter.Print(filteredData);
}

Incidentally, this sort of separation of concerns and dependency injection is good for more than just eliminating parameters. If you access collaborator objects through interfaces, then that makes your class

  • very flexible: you can set up FilteredReportPrinter with any filter/printer implementation you can imagine
  • very testable: you can pass in mock collaborators with canned responses and verify that they were used correctly in a unit test
Wim Coenen
Best answer, in my opinon.
Andrei Tanasescu
A: 

For the parameters that dictate behavior, one can instantiate an object that exhibits the configured behavior. Then client classes simply use the instantiated object - neither the client nor the service have to know what the value of the parameter is. For instance for a parameter that tells where to read data from, have the FlatFileReader, XMLFileReader and DatabaseReader all inherit the same base class (or implement the same interface). Instantiate one of them base on the value of the parameter, then clients of the reader class just ask for data to the instantiated reader object without knowing if the data come from a file or from the DB.

To start you can break your big ParametresExecution class into several classes, one per package, which only hold the parameters for the package.

Another direction could be to pass the ParametresExecution object at construction time. You won't have to pass it around at every function call.

philippe