4

I am doing a refactor over certain code.

We have a list of investors with amounts assigned to each. The total of amounts should be equal to another total, but sometimes there are a couple of cents of difference, so we use different algorithms to assign these differences to each investor.

The current code is something like this:

public void Round(IList<Investors> investors, Enum algorithm, [here goes a list of many parameters]) {

   // some checks and logic here - OMMITED FOR BREVITY

  // pick method given algorithm Enum

  if (algoritm == Enum.Algorithm1) {
      SomeStaticClass.Algorithm1(investors, remainders, someParameter1, someParameter2, someParameter3, someParameter4)
  } else if (algoritm == Enum.Algorithm2) {
     SomeStaticClass.Algorithm2(investors, remainders, someParameter3)
  }
}

so far we only have two algorithms. I have to implement the third one. I was given the possibility to refactor both existing implementations as well as do some generic code to make this function for future algorithms, maybe custom to each client.

My first thought was "ok, this is a strategy pattern". But the problem I see is that both algorithms receive a different parameter list (except for the first two). And future algorithms can receive a different list of parameters as well. The only thing in "common" is the investor list and the remainders.

How can I design this so I have a cleaner interface? I thought of

  1. Establishing an interface with ALL possible parameters, and share it among all implementations.
  2. Using an object with all possible parameters as properties, and use that generic object as part of the interface. I would have 3 parameters: The list of investors, the remainders object, and a "parameters" object. But in this case, I have a similar problem. To instantiate each object and fill the required properties depends on the algorithm (unless I set all of them). I would have to use a factory (or something) to instantiate it, using all parameters in the interface, am I right? I would be moving the problem of too many parameters to that "factory" or whatever.
  3. Using a dynamic object instead of a statically typed object. Still presents the same problems as before, the instantiation

I also thought of using the Visitor Pattern, but as I understand, that would be the case if I had different algorithms for different entities to use, like, another class of investors. So I don't think it is the right approach.

So far the one that convinces me the most is the second, although I am still a bit reticent about it.

Any ideas?

Thanks

Gonzalo.-
  • 12,512
  • 5
  • 50
  • 82
  • Are the parameters all the same type? If so they could be placed in a list. Could just 1 algorithm iterate the parameters list and do what is needed? – brumScouse Oct 06 '17 at 22:35
  • current implementations have decimal values, ints, and enums. There could be a string though – Gonzalo.- Oct 06 '17 at 22:37
  • 1
    Are all the params set regardless of algorithm? Whats determining algo? Why not new up algo there instead? Seems like unnecessary extra level – brumScouse Oct 06 '17 at 23:23
  • an enum is used to determine the algorithm. so far there are two only, I will add the third one, and I want cleaner code for future algorithms – Gonzalo.- Oct 06 '17 at 23:32
  • 1
    It seems like this Round method is pretty pointless. Can you not just call the algorithm you want in place of calling this Round method? – MineR Oct 07 '17 at 06:02
  • Round code is larger, this is a simplified version. I could call each a algorithm by using new. It is precisely what I want to avoid, because I want to have a cleaner interface of what I do (applying an algorithm) instead of worrying which algorithm I have to instantiate. The basic case of strategy pattern – Gonzalo.- Oct 07 '17 at 13:46
  • @Gonzalo.- how did you end up solving this problem? – LazyEval Jan 03 '22 at 11:02
  • I don't remember... I already left that project (and company) many years ago. I think we somewhat ended up breaking the contract defined (which went against what I established in this question) – Gonzalo.- Jan 04 '22 at 18:25

2 Answers2

2

Strategy has different implementations. Its straightforward when all alternate Concrete Strategies require same type signature. But when concrete implementations start asking for different data from Context, we have to gracefully take a step back by relaxing encapsulation ("breaking encapsulation" is known drawback of strategy), either we can pass Context to strategies in method signature or constructor depending upon how much is needed.

By using interfaces and breaking big object trees in to smaller containments we can restrict the access to most of the Context state.

following code demonstrates passing through method parameter.

    public class Context {
        private String name;
        private int id;
        private double salary;
        Strategy strategy;
        void contextInterface(){
            strategy.algorithmInterface(this);
        }
        public String getName() {
            return name;
        }
        public int getId() {
            return id;
        }
        public double getSalary() {
            return salary;
        }
    }

    public interface Strategy {
    // WE CAN NOT DECIDE COMMON SIGNATURE HERE
    // AS ALL IMPLEMENTATIONS REQUIRE DIFF PARAMS
    void algorithmInterface(Context context);
    }

    public class StrategyA implements Strategy{
        @Override
        public void algorithmInterface(Context context) {
            // OBSERVE HERE BREAKING OF ENCAPSULATION 
            // BY OPERATING ON SOMEBODY ELSE'S DATA
            context.getName();
            context.getId();
        }
    }

    public class StrategyB implements Strategy{
        @Override
        public void algorithmInterface(Context context) {
            // OBSERVE HERE BREAKING OF ENCAPSULATION 
            // BY OPERATING ON SOMEBODY ELSE'S DATA
            context.getSalary();
            context.getId();
        }
    }
Kedar Tokekar
  • 418
  • 3
  • 10
  • ok, you are following option two. But what you say is to always fill all the properties in the context object, even though some of them are not needed in the algorithm. Am I right ? If not, I don't see how are you solving the different signature problem here, because you're moving it outside of the strategy and moving it into the context instantiation object – Gonzalo.- Oct 07 '17 at 13:49
  • 1
    @Gonzalo.- Your context seems more complex, so maybe a context object passed at each call to the strategy method doesn't make sense. An alternative is to have each concrete strategy instantiated with a specific constructor having needed context. A simple factory could encapsulate the creation logic. Then the AlgorithmX method would be polymorphic and possibly free of arguments. You could also use this answer with a simple factory to create the Context instance. – Fuhrmanator Oct 08 '17 at 03:59
  • @Gonzalo.-thats true. After second careful pass through the code, I found few queries regarding necessary conditions for strategy. Are these algorithms real _replacement_ of each other? does Round method belong to the class where it should be? (may be not that's the reason it requires so many parameters.). Can we identify good context from the code suitable for all strategies (context should not change state over different strategies). May be it will require more refactoring so as to isolate only strategy applicable part separate. though not sure as do not know complete code. – Kedar Tokekar Oct 09 '17 at 00:39
  • yes the criterias for algorithm are different - each algorithm is picked through a configuration in the database, and each algorithm differentiate in two things (which I want to separate in two different strategy classes but that's for the future) - The way they sort the investors (which investor should I pick first?) and how many units do I assign (should I give the first one half of the remainders ? Should I give the minimum unit?). But that's, to me, parts of the internal implementation of "round" – Gonzalo.- Oct 09 '17 at 12:42
0

Okay, I might be going in the wrong direction... but it seems kinda weird that you're passing in arguments to all the algorithms, and the identifier to which algorithm to actually use. Shouldn't the Round() function ideally just get what it needs to operate?

I'm imagining the function that invokes Round() to look something like:

if (something)
    algToUse = Enum.Algorithm1;
else
    if (otherthing)
        algToUse = Enum.Algorithm2;
    else
        algToUse = Enum.Algorithm3;
Round(investors, remainder, algToUse, dayOfMonth, lunarCycle, numberOfGoblinsFound, etc);

... what if, instead, you did something like this:

public abstract class RoundingAlgorithm
{
    public abstract void PerformRounding(IList<Investors> investors, int remainders);
}
public class RoundingRandomly : RoundingAlgorithm
{
    private int someNum;
    private DateTime anotherParam;
    public RoundingRandomly(int someNum, DateTime anotherParam)
    {
        this.someNum = someNum;
        this.anotherParam = anotherParam;
    }
    public override void PerformRounding(IList<Investors> investors, int remainder)
    {
        // ... code ...
    }
}
// ... and other subclasses of RoundingAlgorithm

// ... later on:
public void Round(IList<Investors> investors, RoundingAlgorithm roundingMethodToUse)
{
    // ...your other code (checks, etc)...

    roundingMethodToUse.Round(investors, remainders);
}    

... and then your earlier function simply looks like:

RoundingAlgorithm roundingMethod;
if (something)
    roundingMethod = new RoundingByStreetNum(1, "asdf", DateTime.Now);
else
    if (otherthing)
        roundingMethod = new RoundingWithPrejudice(null);
    else
        roundingMethod = new RoundingDefault(1000);
Round(investors, roundingMethod);

... basically, instead of populating that Enum value, just create a RoundingAlgorithm object and pass that in to Round() instead.

Kevin
  • 2,133
  • 1
  • 9
  • 21
  • unfortunately I can't update Round method signature. I don't like it either, but where I placed my question is the "starting point" from which I can refactor. Maybe the issue is that "Round" has more logic than just rounding (which I have omited, but mentioned in the comments), so it feels weirder than it should – Gonzalo.- Oct 13 '17 at 20:53
  • Why can't you update the Round() method signature? Aren't you updating it already to add another parameter at the end of your inputs? – Kevin Oct 13 '17 at 20:57
  • yes but I can't update where it is called in another dlls and no, I am not updating it so far, I have the complete list of arguments. I am not adding a new parameter at the end of Round signature – Gonzalo.- Oct 13 '17 at 20:58
  • Ahhhhhh.... I see the confusion. I kept seeing "I'm trying to write an interface" and "pass in a global parameters object" - and I assumed you were doing that to Round. You're talking about changing the SomeStaticClass.Algorithm1 objects, not the Round() function. Okay, yeah, you're probably boned. As long as the people calling Round() are passing in the parameters in such a helter-skelter way, and each of the algorithms wants different sets of parameters? You *could* write a class for each argument collection... but it'd probably be uglier than what you've got in the original post. – Kevin Oct 13 '17 at 21:38