2

I have a method that calculates the price a certain vendor gets for an item.

Simplified it checks whether we have a price for that specific vendor, if not check if we have prices for that specific vendor group, if not get the normal price.

There are several possible outcomes here:

Success
nothing found
MultiplePricesFound = a mistake in our data 
Vendor is in multiple groups = user needs to pick the group

So instead of just returning a double or null or -1 to indicate an error, I build a class that looks somewhat like this. I don't want typos in my result, so I made it an enum.

public class PriceResult {
   double Value;
   PriceResults Result;
   Dictionary<int, double> VendorGroupPriceDictionary;
}
public enum PriceResults {NothingFound, Success, MultipleVendorGroups, MultiplePrices }

The dictionary holds multiple vendor groups and their according prices.

This will work. However, I created a new class and an enum which will be used by probably exactly one method. This feels somewhat overkill to me.

I'm wondering whether this indicates anything? Do I need to simplify the calculating method?

  • 3
    " i created a new class and an enum which will be used by probably exactly one method" - (1) no, you created a thing that lets you express a concept more clearly then relying on magic numbers and unstructured constants (quite handy when reading code), and (2), it's going to be used by more then one method - everything that calls it is going to use it also. Now, whether or not there's a better/simpler way to represent these concepts is a different matter; the point is that it's not an overkill. – Filip Milovanović Dec 4 '19 at 19:09
  • You've tagged this as object-oriented. Object orientation would probably favor subclasses over the enums and one class that is the union of all (subclass') fields. Of course, that might seem even more of an indication of complexity, but i would submit it is not complex to use classes to model things -- we're not specifically trying to minimize class count after all. I also agree with the comment by @FilipMilovanović. – Erik Eidt Dec 4 '19 at 20:27
  • Do you need to transmit the actual prices when there's a MultiplePricesFound result? Side note, having a method return a structure that can be one of several things (a double OR a list of string OR an error message) is resolved by using a language that supports Union Types. If you can write this function in F# and integrate it into your C# app, maybe take a look at their Union Types, which they call "Discriminated Unions". – Graham Dec 10 '19 at 18:04
  • Leaving aside the problem you're actually asking about: it is a very bad practice to represent a price as a double. Doubles are for physical quantities such as length or speed or mass. Use decimal for prices, or even better, create a wrapper struct around decimal called USDollars so that you don't end up accidentally treating US dollars as Canadian dollars when you do an international order someday. – Eric Lippert Dec 10 '19 at 18:40
2

However i created a new class and an enum which will be used by probably exactly one method. This feels somewhat overkill to me.

Not necessarily. Creating a small helper class which provides a useful abstraction is often a sensible approach. This class can sometimes just be a DTO without any behaviour, which does not produce too much overhead (often it helps to reduce boilerplate code, long argument lists and the necessity of addtional comments).

To your actual example case: it is pretty hard to tell without seeing the code which will actually uses the PriceResult if this abstraction is a useful one. I can only make an some educated guess on how to simplify this.

What looks suspicious here is the redundancy in this class as well as mixing up the success of the calculation with the different types of price tables.

  • If the state is NothingFound, or MultiplePrices, the attributes Value and VendorGroupPriceDictionary probably have no meaningful values? So I guess it does not really make sense to have PriceResult object in this case at all? So one could probably separate the sucess state into a separate enum and model the calculation function like

     CalculationSuccess TryCalcPriceForVendor(Vendor vendor, out PriceResult priceResult)
    

    where the enum looks like

    enum CalculationSuccess {Success, NothingFound, MultiplePrices}
    

    This design makes sense if success state evaluation is done separately from dealing with valid PriceResult objects. If that is not the case and the PriceResult requires the meta information about it being "invalid" in different places, better stick with the old design.

  • A vendor might be put into its own vendor group with just one element, and there might be something like a "DefaultVendor" or "NonVendor" kind of vendor ID for associating the "normal prices" of items. If you can model your system that way, it may allow you to get rid of the distinction between the cases of different result representations for a vendor group, a vendor or vendor-independent prices. This would lead to

     public class PriceResult {
        Dictionary<int, double> VendorGroupPriceDictionary;
     }
    

    where this dictionary will contain only one entry for single vendors or "normal" prices.

    However, this change might cause changes (or at least allow changes) to other parts of your system as well, so it will be again necessary to look first how this code will be used to determine if that brings you any benefits.

| improve this answer | |
0

I would try to find a different design.

Not because it is complicated, that may be because you have a complicated behavior (I can't comment on this, since not enough is known for that), but because with this bag of data that you return you push all the evaluation logic onto the caller. The caller has to know intricate details of what you do, what data structures you fill out, how they are related. Basically everything.

Instead, what you could do is offer the exact function the caller actually wants from you. Does it want to compare prices, does it want to select prices, does it want to print it? For start, hide the data of the result and offer business-related methods. I.e. FindPrice(VendorSelector), FindSinglePrice(), etc. Stuff that you need on the other side.

If the caller really wants to know everything this result object has, then effectively the caller's internal logic controls your service's internal logic. In other words, they are probably very tightly coupled, which is bad.

| improve this answer | |
0

My impression is that you are still thinking procedurally. I recommend thinking of this in an object-oriented way. You're not calling something that returns data; you are creating a data object, called a "Quote," that contains some logic.

Also, you'll want an interface so your code can be SOLID.

class Quote : IQuote
{
}

First, you need some way to create a quote. You can use a constructor, and if that gets too complicated you might write a factory.

class Quote : IQuote
{
    public Quote(Vendor vendor, Product product)
    {
        //Compute the quote
    }
}

class QuoteFactory : IQuoteFactory
{
    public IQuote GetQuote(Vendor vendor, Product product)
    {
        return new Quote(vendor, product);
    }
}

When the quote is computed, you will need to expose all sorts of stuff. This is stuff you'd put in the interface and then implement in the class. I don't know your implementation, but I can conjecture what your interface might look like:

interface IQuote
{
    bool IsPriceAvailable { get; }

    decimal Price { get; }

    bool HasMultiplePrices { get; }

    bool HasMultipleVendors { get; }

    bool IsProductAvailable { get; }
}

Now when you need to use this class, the logic is straightforward and easy to read:

IQuote quote = _quoteFactory.GetQuote(vendor, product);
if (quote.IsPriceAvailable)
{
    Display("The price is {0:#.00}", quote.Price);
}
else
{
    Display("Couldn't come up with a quote.");
    if (!quote.IsProductAvailable) 
    {
        Display("Product is not available to this vendor.")'
        return;
    }    
    if (quote.HasMultiplePrices) 
    {
        Display("Could not provide quote because the product is available under several price points.");
        return;
    }    
    //etc.
}
| improve this answer | |

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.