2

I have a design problem, and no-one to help me thrash it out. Except for you.

The system handles data files. Each row can be compared against a number of criteria, chosen by the user. The row is then true or false for that criteria.

Each criteria also has a price and a cost (and a margin: price - cost). However, the user is only charged if a row matches one of the criteria. And they only pay for one of the matched criteria: the one used should be the one with the highest margin.

Structurally, it looks like this in pseudo code:

SystemRow
{
    bool Criteria1 { get; set; }
    bool Criteria2 { get; set; } //etc
}

There's a separate DTO which holds a user's selected criteria as a bitwise Enum. It's passed into a row processing function along with the row.

ProcessRow(selectedCriteria, row)
{
    if(selectedCriteria.HasFlag(CriteriaEnum.Criteria1)
    {
        // this sets row.Critera1 to true if it matches conditions
        EvaluateRowForCriteria1(row);
    }
    // then evaluate other criteria one by one
}

CalculatePrice(ListOfRows)
{
    decimal runningTotal;
    foreach(row in ListOfRows)
    {
       runningTotal += [the cost of one - and only one - matching criteria]
    }
}

Each of these functions exists in a separate class, in accordance with SRP.

I can think of several ways to do this, but each is messy: hard to maintain and inefficient.

First, I can add a "Price" and "Margin" property to "Row". Then, in the class where "ProcessRow" lives I can pull all the prices and margins from the DB and store them in local variables. During "ProcessRow" I can compare the Margin of a criteria with the Marign of the row and, if better, assign the price. Then I can sum the prices in CalculatePrice.

Second, I can fetch the Price and Margin in CalculatePrice instead. Then, for each row, I can use the selected CriteriaX on the row, find the correct one to charge, and add it to the running total.

What makes this such a mess is that there's about 20 of these criteria. So either way there's going to have to be a ton of variables kicking around if I don't want to have to go to the Db to get the values on each row.

Is there a better design I can leverage here?

  • Your code does not make sense to me compared to your words. If a row has bool criteria then why are you doing a string comparison? "Compare the Margin of a criteria with the Marign of the row and, if better, assign the price" is not clear to me. You need to more clearly define how a margin is calculated to get a proper answer. – paparazzo Mar 30 '17 at 18:53
  • Confusion. Is a criteria an object with price and cost or is it a boolean? – radarbob Mar 30 '17 at 19:01
  • @radarbob: currently, it seems the criteria are not modeled as objects, that is exactly what is missing here to clean up the mess described by the OP. See my answer. – Doc Brown Mar 30 '17 at 19:30
  • @Paparazzi Sorry for the confusion - it's not a string comparison. That's just a selector to see which services have been chosen. The actual comparison that checks against a given critera is irrelevant, so I left it out of the question. Have edited, hopefully to make it clearer. – Bob Tway Mar 31 '17 at 8:34
1

Analysis of your approach

Current situation:

  • you don't know in advance the criteria desired by the user;
  • you keep the criteria to be used for a query in a container;
  • you iterate through all the rows;
  • for each row you look for all matching criteria;
  • your issue is to price each row by maximizing the margin;

Your current options are:

  • option A: price the row in the matching process, changing price if there's a better margin for another matching criteria;
  • option B: price the row at summation, keeping track of the best criteria.

As you have more than 20 criteria, both of your solution imply checking all of the matching criteria. In the worst case you'd try to match all 20 conditions: that's 19 too much !

Better alternative

Instead of asking yourself which is the most profitable criteria, and this for every row, I propose you to:

  • first order your container of selection criteria requested by the user, by decreasing order of profitability.
  • then for a given row, try successively each criteria IN THE RIGHT ORDER. So when a criteria matches, it's the most profitable and you no longer need to match the remaining criteria for the same line.
  • you can calculate the price when processing the line, or when making the summation: in any case you've only one price to use.

This approach considerably decreases the matching time for successful rows. The performance is unchanged for unselected rows.

|improve this answer|||||
  • Thanks for this. Feel foolish not to have spotted this for myself. The switch to a dynamic price was a late addition, otherwise I might have followed this approach from the first. – Bob Tway Mar 31 '17 at 8:36
1

Model your criteria as a class hierarchy, not just as a list of strings: derive each criteria class from a common interface or abstract base class:

  interface ICriteria
  { 
      bool DoesItMatch(Row row);
      double Price{get;}
      double Cost{get;}
  }

Now all you need is a factory class which creates a list of concrete ICriteria objects from the user's selection (which I assume will query your db just once). After you have this list, order it in descending order by the margin. Then the cost evaluation will be as simple as

double CalculateHighestMargin(List<ICriteria> orderedCriterias, row)
{
     foreach(var crit in orderedCriterias)
     {
        if(crit.DoesItMatch(row))
            return crit.Price - crit.Cost;
     }
     return 0;
}

Note there is no need to store the boolean values for the rows in each row object, or the price and cost of each row (at least not for the requirements described in the question).

|improve this answer|||||
  • Thanks for this. It's a great solution but I may not be able to leverage it, as I've had an object model imposed on me that I'm not sure I can change. – Bob Tway Mar 31 '17 at 8:35
  • 1
    @MattThrower: I don't know which idiot in your team is forbidding you to write code, but this is not about changing anything like a persistent object model, it is about adding some helper classes to organize your functions in a better way. But if you are not allowed to design such simple code structures, you should probably look for a different employer. – Doc Brown Mar 31 '17 at 11:10
  • +1 Dr. Brown comment. Rather than change jobs, wrap the model objects you're stuck with new objects (classes) that do the right thing. You'll want to expose the existing API for compatibility, and have new methods as needed. You may want factory/builder/whatever class(es) to ensure proper construction and better keep your "working classes" decoupled. The idea is "adapter" – radarbob Apr 12 '17 at 19:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.