views:

265

answers:

7

Hi. I tend to sucessfully write compact applications that can encapsulate many business logic in a simple and non-redundant way. I tend to create small methods, but over time I arrive to methods that have too many parameters. I know every code requires its design, but I see and antipattern in my behaviour and I am not sure which would be the best way to fight against it.

A typical situation would be something like:

   public loanStructure CalculateSomeComplicatedInterestRate(enum clientType, 
        int totalAmout, bool useTaxes, bool doYearlySummary, bool doNotReloadClient, etc)
   {
        loanStructure ret = new loanStructure();
        if (doNotReloadClient) ... ;
        ...
        if (doYearlySummary) GroupResults(ret);
        return ret;
   }

and inside the method, a tree of calls forwards the boolean settings (doYearlySummary, doNotReloadClient, etc) to different business rules that acts on the calculation.

I.E., the problem does not reside on the fact that all parameters could be encapsulated into an object (bigParameterStructure)... I am not comfortable with the masterMethod pattern, but making overloads such as CalculateSomeComplicatedInterestRateMonthly and CalculateSomeComplicatedInterestRateYearly would just hide a private CalculateSomeComplicatedInterestRate.... some problem!!

Of coarse object-orienting design would help... but I still endup having this kind of methos somewhere on my objects...

Well guys... any help is welcome.

Pablo.

+2  A: 

I usually create "filter" structs that I can pass to those methods, so in your case.

(quick and dirty, not production quality.)

public struct InterestRateCalculationFilter
{
  public enum ClientType {get;set;}
  public int TotalAmount {get;set;}
  public bool UseTaxes {get;set;}
  public bool DoYearlySummary {get;set;}
  public bool DoNotReloadClient {get;set;}

  //Constructors go here, set defaults, etc.
}

Usage:

InterestRateCalculationFilter filter = new InterestRateCalculationFilter();
filter.ClientType = Customer;
filter.TotalAmount = 700;
filter.UserTaxes = true; 
filter.DoYearlySummary = false;

LoanStructure loanStructure = CalculateSomeComplicatedInterestRate(filter);

This isn't always a good idea, but I do go to it often enough, when parameter lists get over ten items, and they don't necessarily fall into an object structure nicely.

Matthew Vines
Better not let http://stackoverflow.com/users/23354/marc-gravell see those structs, or he'll be all over you like a rash ;-)fwiw though, I find class rather than struct more useful (because struct is pass by copy) in this instance **_if_** the callee modifies input values, otherwise you have to pass by ref. Also look at object initialisation (csc >= 3 I think) to trim the fat on usage.
Si
You bring up a good point. I've always used structs to represent these types of things, though I find a lot of people are saying to use classes. But it does make me wonder where structs fit into todays application development space. To Google!!
Matthew Vines
I agree about object initialisation syntax, but he didn't specify a language in his question, and I didn't want to throw too big of a curve ball in there with syntax. Note the disclaimer :)
Matthew Vines
@Si, Why would this type of refactoring technique be a bad thing? Do you have any pointers or SO questions dealing with this?
Magnus Skog
Magnus, you end up with an object or structure which makes absolutely no sense, except in the context of some arbitrary method of another class - that's a misuse of object concept. If argument structure describes something more or less independent, there is no such problem. Marginal cases, like MouseEventArgs, are at least not tied to a specific method.
ima
A lot of this depends on usage. If InterestRateCalculationFilter() is a method only called in one or two places, I will probably just suck it up and have the parameter list. However, if InterestRateCalculationFilter() is an integral part of my application, and is called dozens or hundreds of times I think it makes a lot of sense to encapsulate the arguments into some sort of containing structure, either a class or a struct. But, as I say in my answer, this is not always the right thing to do. Situation always dictates.
Matthew Vines
+1  A: 

This approach has certain downsides, but one way to simplify the argument list is to create an "arguments" class that has properties to hold all of the parameters that can be passed in.

For example (C#):

public class InterestCalculationArguments // or maybe a struct...
{
    public EClientType ClientType {get;set;}
    public int TotalAmount {get;set;}
    // etc.
}

One nice thing with this approach is that you can subclass the arguments class to extend the arguments that can be passed in, or you can also add a "NameValueCollection" (e.g. a string-string map) to allow the caller to pass in additional arguments that the method can process. You can use this same approach for the return object, a class to hold various return values. This approach might also insulate the caller from small changes to the arguments/return objects.

Andy White
great idea! I`m going to use it right now
chester89
+2  A: 

An alternative to "arguments struct" approach, which often works better in complicated cases, is to move the logic of a big method to a separate class:

InterestRateCalculator rateCalc = new InterestRateCalculator();
rateCalc.ClientType = ...
rateCalc.TotalAmount = ...
rateCalc.CalculateSomeComplicatedInterestRate();

It can be combined with a sort of factory:

InterestRateCalculator myCalc 
    = sharedCalc.MakeMeAnInterestRateCalculator();

And since you mentioned overloading function to simplify arguments list, you can sometimes replace boolean arguments with class inheritance.

ima
+3  A: 

Split complicated methods into separate classes and create instance properties for the appropriate parameters (in your example: clientType, useTaxes, doYearlySummary and doNotReloadClient).

I can highly recommend Robert C. Martins "Clean Code: A Handbook of Agile Software Craftsmanship" which gives a very nice introduction into creating clean code.

Christian Schwarz
+4  A: 

If you find yourself switching between a few sets of multiple parameters, consider stashing the code in a private complicated method and provide some public methods which take less parameters and call the private one providing appropriate defaults for a part of parameters. This will clean up the public interface.

Then, unless you really cannot do it for performance reasons (which is not likely), break up the private method into calls to other private methods, to keep code more readable. You can get rid of comments in this way, by choosing self-descriptive method names.

On the other hand, if some of the parameters are not about WHAT to do, but HOW to do it (i.e. you can have an enum which switches between simple and continuous compounding), consider using polymorphism and having two classes implementing a common interface, each in a different way, OR delegating this piece functionality to a component of the main class which can be changed from this to that implementation (a'la "dependency injection"). The second approach will make your code more flexible, as the users of your class will be able to provide their own components and change the behaviour of your main class without touching its code.

You should also think about who uses your class. Maybe the parameter list in your method is so long, because the method has grown over time to accomodate different needs of different users ("user" in this context means another piece of code, even written by you)? If so, at the very least I would create one method for one purpose, another method for another purpose. They can still share common code. Ideally, you would want to have two classes serving two purposes, to be able to change them in the future without breaking the other functionality.

quant_dev
+1  A: 

Sometimes it's straightforward to move such parameter lists into a class, if it makes sense. For example, you might have InterestRateDefinition which could be used in many different places.

However it often promotes the creation of classes that simply exist to serve as parameter list and the class is meaningless. I've done this a fair bit myself in the past and they tend to clutter the code rather than provide clarity. These days, if the argument list does not lend itself to a class, then I turn the function itself into a class.

So you might have an InterestRateCalculator class instead, which takes either a number of public inputs or properties. This resolves the problem and maintains a sense of "code purpose" plus you can providing defaults and alternate ways to call your routine through the class (e.g. CalculateNextCoupon, CalculateAnnual etc.)

Joel Goodwin
A: 

When I face code smells I scan Refactoring: Improving the Design of Existing Code and Refactoring to Patterns. This books are plenty of useful advice to refactor code.

JuanZe
Also see "Clean Code" by Robert C. Martin
Ian Ringrose