34

Say I have a class like this for calculating the cost of travelling different distances with different modes of transportation:

public class TransportationCostCalculator
{
    public double DistanceToDestination { get; set; }

    public decimal CostOfTravel(string transportMethod)
    {
        switch (transportMethod)
        {
            case "Bicycle":
                return (decimal)(DistanceToDestination * 1);
            case "Bus":
                return (decimal)(DistanceToDestination * 2);
            case "Car":
                return (decimal)(DistanceToDestination * 3);
            default:
                throw new ArgumentOutOfRangeException();
        }
    }

This is fine and all, but switch cases can be a nightmare to maintenance wise, and what if I want to use airplane or train later on? Then I have to change the above class. What alternative to a switch case could I use here and any hints to how?

I'm imagining using it in a console application like this which would be run from the command-line with arguments for what kind of transportation vehicle you want to use, and the distance you want to travel:

class Program
{
    static void Main(string[] args)
    {
        if(args.Length < 2)
        {
            Console.WriteLine("Not enough arguments to run this program");
            Console.ReadLine();
        }
        else
        {
            var transportMethod = args[0];
            var distance = args[1];
            var calculator = new TransportCostCalculator { DistanceToDestination = double.Parse(distance) };
            var result = calculator.CostOfTravel(transportMethod);
            Console.WriteLine(result);
            Console.ReadLine();
        }
    }
}

Any hints greatly appreciated!

Khaine775
  • 2,715
  • 8
  • 22
  • 51
  • Write for each CostOfTravel an own class. BicycleCostCalculator, BusCalculatorFactory etc – Jannik Feb 25 '16 at 13:36
  • 4
    Instead of using a string for transportMethod, may be you can try creating an enum. – VivekDev Feb 25 '16 at 13:37
  • It is very popular topic. Get rid of switch. I think in this case switch is good ; ) 6 answer to do this is i think record. Howover it is what programer like most do this think : ) – blogprogramisty.net Feb 25 '16 at 13:55
  • This is a classic case for inheritance – romanoza Feb 25 '16 at 13:56
  • 1
    How are your chances of adding "airplane" *without* having to recompile / roll out your application due to some other reason elsewhere? Even if it's just about the documentation? -- The `Dictionary`-based answers *look* nice, but note that they *seperate* the point of exception from the point of transport definition. Your switch will throw the `ArgumentOutOfRangeException` right there where it is obvious that "airplane" is missing -- you could even give that information in the exception message... (Not saying you, or any of the answers, are wrong. Just looking at it from a different angle.) – DevSolar Feb 25 '16 at 16:38
  • 2
    I think you need to think architecturally. If cost per mile is the only consideration when it comes to means of transport, then stick with your switch-case statement. But if there are other considerations so that you have another conditional statement somewhere else based on means of transport ("do you have fuel receipts?" for example), it's then that you should consider a MeansOfTransport abstract base class, and Bus/Car/Bicycle as concrete classes which fill in those details. – Graham Feb 25 '16 at 17:23
  • You could use an Enum and a bit of reflection to parse the userstring into an element name – beppe9000 Feb 26 '16 at 00:02
  • Using a magic string is the root of your issue. If a constant literal (string, number, ...) appears multiple times, then you are repeating yourself, and repeating yourself leads to maintenance issues. `switch` are fine when the compiler can check that all values are covered. – Matthieu M. Feb 26 '16 at 08:38
  • This is a standard example where the strategy pattern makes sense. – theDmi Feb 26 '16 at 09:53

11 Answers11

39

You could do something like this:

public class TransportationCostCalculator {
    Dictionary<string,double> _travelModifier;

    TransportationCostCalculator()
    {
        _travelModifier = new Dictionary<string,double> ();

        _travelModifier.Add("bicycle", 1);
        _travelModifier.Add("bus", 2);
        _travelModifier.Add("car", 3);
    }


    public decimal CostOfTravel(string transportationMethod) =>
       (decimal) _travelModifier[transportationMethod] * DistanceToDestination;
}

You could then load the transportation type and it's modifier in a configuration file instead of using a switch statement. I put it in the constructor to show the example, but it could be loaded from anywhere. I would also probably make the Dictionary static and only load it once. There is no need to keep populating it each time you create a new TransportationCostCalculator especially if it isn't going to change during runtime.

As noted above, here is how you could load it by a configuration file:

void Main()
{
  // By Hard coding. 
  /*
    TransportationCostCalculator.AddTravelModifier("bicycle", 1);
    TransportationCostCalculator.AddTravelModifier("bus", 2);
    TransportationCostCalculator.AddTravelModifier("car", 3);
  */
    //By File 
    //assuming file is: name,value
    System.IO.File.ReadAllLines("C:\\temp\\modifiers.txt")
    .ToList().ForEach(line =>
        {
           var parts = line.Split(',');
        TransportationCostCalculator.AddTravelModifier
            (parts[0], Double.Parse(parts[1]));
        }
    );
    
}

public class TransportationCostCalculator {
    static Dictionary<string,double> _travelModifier = 
         new Dictionary<string,double> ();

    public static void AddTravelModifier(string name, double modifier)
    {
        if (_travelModifier.ContainsKey(name))
        {
            throw new Exception($"{name} already exists in dictionary.");
        }
        
        _travelModifier.Add(name, modifier);
    }
    
    public double DistanceToDestination { get; set; }

    TransportationCostCalculator()
    {
        _travelModifier = new Dictionary<string,double> ();
    }


    public decimal CostOfTravel(string transportationMethod) =>
       (decimal)( _travelModifier[transportationMethod] * DistanceToDestination);
}

Edit: It was mentioned in the comments that this wouldn't allow the equation to be modified if it ever needed to change without updating the code, so I wrote up a post about how to do it here: https://kemiller2002.github.io/2016/03/07/Configuring-Logic.html.

kemiller2002
  • 113,795
  • 27
  • 197
  • 251
  • 3
    I like this method better than refactoring into a new design pattern.. it's quicker, easier, and has less overhead. – mike Feb 25 '16 at 13:43
  • 2
    While your answer definitely meets the OP's requirements, I have to ask - is this any more maintainable than a switch statement? Essentially `case X: return Y` is simply replaced by `_travelModifier.Add(X,Y)` which seems like the same maintenance effort to me? – dmeglio Feb 25 '16 at 15:10
  • 7
    Once you test that adding it through a configuration file works (mine puts i in the constructor just to show an example), you can add, modify, delete entries into the table without recompiling the code. That way the code works the same way it always does, because it didn't change. – kemiller2002 Feb 25 '16 at 16:01
  • 1
    Very interesting answer. How could a config file like that look? And how could I load it into the dictionary? This is fairly new to me. – Khaine775 Feb 25 '16 at 16:44
  • 4
    @Khaine775 a simple text file with key/value pairs would work: "bicycle=1\nbus=2\ncar=3" –  Feb 25 '16 at 16:59
  • That makes sense! But how could I load it into the dictionary on program start? – Khaine775 Feb 25 '16 at 21:44
  • 1
    If you make the dictionary static (and public) you could just access it like: TransportationCostCalculator._travelModifier. I would probably name it something different more like: TransportationCostCalculator.TravelModifier. Just add the entries on the program startup. You could be really fancy too, and create custom entry in the App.config which stores the values and loads them on demand. – kemiller2002 Feb 25 '16 at 21:48
  • 2
    This does work, but I think using a small struct or class with a named property for the multiplier is more explicit and readable. Additionally, what happens if a new type doesn't use a simple multiplier of distance, but some other calculation? That's when strategy/factory wins it. – ventaur Feb 26 '16 at 02:19
  • God bless dictionaries – D. Ben Knoble Feb 26 '16 at 07:47
  • Since it was brought up, I wrote up how you would use a configuration file that would allows for placing equations in it: http://structuredsight.com/2016/03/07/configuring-logic/ – kemiller2002 Mar 07 '16 at 18:25
35

It looks to me like any solution based on your current method is flawed in one critical way: No matter how you slice it, you're putting data in your code. This means every time you want to change any of these numbers, add a new vehicle type, etc., you have to edit code, and then recompile, distribute a patch, etc.

What you really should be doing is putting that data where it belongs - in a separate, non-compiled file. You can use XML, JSON, some form of database, or even just a simple config file. Encrypt it if you want, not necessarily needed.

Then you'd simply write a parser that reads the file and creates a map of vehicle type to cost multiplier or whatever other properties you want to save. Adding a new vehicle would be as simple as updating your data file. No need edit code or recompile, etc. Much more robust and easier to maintain if you plan to add stuff in the future.

John Kugelman
  • 349,597
  • 67
  • 533
  • 578
Darrel Hoffman
  • 4,436
  • 6
  • 29
  • 41
  • 5
    This is a really important point. Well said. Code is for code, data files are for data. Try to keep them separate. – Floris Feb 25 '16 at 22:16
  • 2
    Data-driven solutions, as suggested here, reduce the amount of code blocks for unit testing. You don't **have** to place things in a config file but as @Floris mentions, it's the separation that counts. – mungflesh Feb 26 '16 at 01:08
  • This should be the accepted answer imo. Separation of code and data is what is most important here – TheIronKnuckle Feb 26 '16 at 05:17
  • 1
    It's an excellent advice... though at the same time need be nuanced. Having data in code (as an `Enum` for example) allows compile-time checking which has benefits too; and a proper configuration handling (with legible errors in case the configuration is wrong) also has a cost (development and maintenance) so as always there's a trade-off. – Matthieu M. Feb 26 '16 at 08:46
14

Sounds like a good candidate for dependency-injection:

interface ITransportation {
    decimal CalcCosts(double distance);
}

class Bus : ITransportation { 
    decimal CalcCosts(double distance) { return (decimal)(distance * 2); }
}
class Bicycle : ITransportation { 
    decimal CalcCosts(double distance) { return (decimal)(distance * 1); }
}
class Car: ITransportation {
    decimal CalcCosts(double distance) { return (decimal)(distance * 3); }
}

Now you can easily create a new class Plane:

class Plane : ITransportation {
    decimal CalcCosts(double distance) { return (decimal)(distance * 4); }
}

Now create a constrcutor for your calculator that expects an instance of ITransportation. Within your CostOfTravel-method you can now call ITransportation.CalcCosts(DistanceToDestination).

var calculator = new TransportationCostCalculator(new Plane());

This has the advantage that you can exchange your actual transportation-instance without any code-change to your TransportationCostCalculator-class.

To complete this design you might also create a TransportationFactory as follows:

class TransportationFactory {
    ITransportation Create(string type) {
        switch case "Bus": return new Bus(); break
        // ...
}

Which you call like

ITransportation t = myFactory.Create("Bus");
TransportationCostCalculator calculator = new TransportationCostCalculator(t);
var result = myCalculator.CostOfTravel(50);
fiscblog
  • 694
  • 1
  • 12
  • 27
MakePeaceGreatAgain
  • 35,491
  • 6
  • 60
  • 111
  • 3
    But the problem remains: how do you determine which ITransportation class (Bus, Bicycle, Plane, etc) to instantiate? – Matt Feb 25 '16 at 13:40
  • 3
    @Matt usually with a factory method... which tends to include a switch statement :) so it still leaves that as a result. – dmeglio Feb 25 '16 at 15:11
  • 3
    Yes, the factory will have the switch, but it's in only one place then. Imagine when you want to add another way to calculate costs in the future. You'd end up with more switch statements with the original approach, but not so with the factory. – ventaur Feb 26 '16 at 02:12
8

You could define an abstract class like this, and have each TransportationMethod extend the abstract class:

abstract class TransportationMethod {
    public TransportationMethod() {
        // constructor logic
    }

    abstract public double travelCost(double distance);
}

class Bicycle : TransportationMethod {
    public Bicycle() : base() { }

    override public double travelCost(double distance) {
        return distance * 1;
    }
}

class Bus : TransportationMethod {
    public Bus() : base() { }

    override public double travelCost(double distance) {
        return distance * 2;
    }
}

class Car : TransportationMethod {
    public Car() : base() { }

    override public double travelCost(double distance) {
        return distance * 3;
    }
}

So in your actual method call, it could be rewritten like this:

public decimal CostOfTravel(TransportationMethod t) {
    return t.travelCost(DistanceToDestination);
}
TayTay
  • 6,882
  • 4
  • 44
  • 65
  • Correct me if I'm wrong, but Abstract Classes can't have a constructor. – Matt Feb 25 '16 at 13:42
  • @Matt They can. It can however onöy be called from the derived classes. – MakePeaceGreatAgain Feb 25 '16 at 13:44
  • Please correct the syntaxt. There is no `super` in c#. – romanoza Feb 25 '16 at 13:51
  • 1
    Thanks all for setting me straight on Abstract Constructors. Just updated my own code, much more slick now. – Matt Feb 25 '16 at 14:01
  • 3
    I think inheritance is the way to go. While a dictionary is fine too, inheritance scales better, and can handle types which are only introduced later (one *could* add types to a dictionary later but would need an interface just for adding transportation modes). Inheritance also scales *much* better when more differences between modes of transportation emerge (speed etc.). – Peter - Reinstate Monica Feb 25 '16 at 14:48
  • In this example, `CostOfTravel(TransportationMethod t)` shouldn't even exist anymore. Any potential caller to that method should instead just call `t.travelCost()` directly – BlueRaja - Danny Pflughoeft Feb 25 '16 at 22:01
  • @BlueRaja-DannyPflughoeft yes, that's fine. I just wanted to put it in context of his current code and show how it addressed his question, i.e, refactoring the need for the `switch`. – TayTay Feb 26 '16 at 13:13
5

You could use a strategy class for each type of travel. But, then you'd probably need a factory to create the strategy based upon the transport method which would likely have a switch statement to return the appropriate calculator.

    public class CalculatorFactory {
        public static ICalculator CreateCalculator(string transportType) {
            switch (transportType) {
                case "car":
                    return new CarCalculator();
                ...
public class CarCalculator : ICalculator {
    public decimal Calc(double distance) {
        return distance * 1;
    }
}
....
Jack Hughes
  • 5,514
  • 4
  • 27
  • 32
4

You can make a Dictionary that returns a multiplier based on transport.

public class TransportationCostCalculator
{
    Dictionary<string, int> multiplierDictionary;

    TransportationCostCalculator () 
    {
         var multiplierDictionary= new Dictionary<string, int> (); 
         dictionary.Add ("Bicycle", 1);
         dictionary.Add ("Bus", 2);
         ....
    }

    public decimal CostOfTravel(string transportMethod)
    {
         return  (decimal) (multiplierDictionary[transportMethod] * DistanceToDestination);       
    }
Valentin
  • 5,380
  • 2
  • 24
  • 38
2

I think the answer is some kind of database.

If you use some, the TransportCostCalculator ask the database for the multiplayer to the given transportmethod.

The database may be a text-file or an xml or an SQL-server. Simply a key-value-pair.

If you want to use code-only there is - tmo - no way to avoid the translation from transportmethod to multiplayer (or cost). So some kind of swicht is needed.

With the database you put the dictionary out of your code and you must not change your code to apply new transportmethods or change the values.

PBum
  • 123
  • 10
  • 3
    Seems like overkill. You're adding the overhead of file IO and/or network IO for a very simple calculation. You're suggestion of a SQL database is accomplishing the same thing that a `Dictionary<>` would in C#. You're just simply storing the key-value-pair in an external resource instead of C#. – dmeglio Feb 25 '16 at 15:29
  • 2
    @dman2306 I appreciate your comment, because it points to examining the requirements of the project. Which matters more: avoiding I/O or avoiding the need to recompile? I inherently like the external text file/database approach because it avoids touching the code's methodology once set up. But, after adding missed modes of transportation to the code a few times, how many more are you likely to need to add (and thus recompile for)? Thereafter, it is in code, and as I'm learning and as you pointed out, much faster. – GG2 Feb 25 '16 at 17:40
  • If you're worried about performance loss from I/O, you're doing it wrong. You should load the file ONCE when you start the program and save the data in memory. If you're looking at the file every time the function is called, of course it's going to be slow. The only reason you'd want to do that is if A.) The data file is HUGE, and won't all fit in memory at once, or B.) You expect the data to change frequently *while the program is running*. It doesn't sound like either of these is the case. – Darrel Hoffman Feb 26 '16 at 15:04
1

This is a case for the strategy design pattern. Create a base class, say TravelCostCalculator, then develop classes for each mode of travel you will consider, each overriding a common method, Calculate(double). You can then instantiate the specific TravelCostCalculator as needed using the factory pattern.

The trick is in how to construct the factory (without a switch statement). The way I do this is by having a static class constructor (public static Classname() - not an instance constructor) that registers each strategy class with the factory in a Dictionary<string, Type>.

Since C# does not run class constructors deterministically (like C++ does in most cases) you have to explicitly run them to ensure they will run. This could be done in the main program or in the factory constructor. The downside is that if you add a strategy class, you must also add it to the list of constructors to be run. You can either create a static method that must be run (Touch or Register) or you can also use System.Runtime.CompilerServices.RuntimeHelpers.RunClassConstructor.

class Derived : Base
{
    public static Derived()
    {
        Factory.Register(typeof(Derived));
    }
}

// this could also be done with generics rather than Type class
class Factory
{
    public static Register(Type t)
    {
        RegisteredTypes[t.Name] = t;
    }
    protected Dictionary<string, Type t> RegisteredTypes;

    public static Base Instantiate(string typeName)
    {
        if (!RegisteredTypes.ContainsKey(typeName))
            return null;
        return (Base) Activator.CreateInstance(RegisteredTypes[typeName]);
    }
}
tyson
  • 166
  • 1
  • 9
  • 2
    Instead of dealing with static constructors, you could use reflection to look for classes that inherit from `TravelCostCalculator`. You could then have a custom attribute on each to specify a name like `[TravelMethod("Bus")]`. Or you could do it by convention and just name the derived classes METHODTravelCostCalculator where METHOD would be the name your search for. – dmeglio Feb 25 '16 at 15:44
1

I prefer to use Enum for that like this:

public enum TransportMethod
{
    Bicycle = 1,
    Bus = 2,
    Car = 3
}

And use it like this method:

public decimal CostOfTravel(string transportMethod)
{
    var tmValue = (int)Enum.Parse(typeof(TransportMethod), transportMethod);
    return DistanceToDestination  * tmValue;
}

Note that above method is case-sensitive, So you can capitalize first char;

Related Answer

Community
  • 1
  • 1
shA.t
  • 16,580
  • 5
  • 54
  • 111
0

It was said before but i want to give related topic another shot.

This is a good example for reflection. "Reflection objects are used for obtaining type information at runtime. The classes that give access to the metadata of a running program are in the System.Reflection namespace."

By using reflection, you will avoid compiling code if another switch type such as train is wanted to add the program. You will solve the problem on the fly by using a config file.

I recently solved a similar problem with strategy patttern, by using dependency injection but I still end up with switch statement. It doesnt solve your problem this way. Method suggested by tyson still needs recompile if a new type added to dictionary.

An example of what i am talking about: Dynamic Loading of Custom Configuration XML using Reflection in C# : http://technico.qnownow.com/dynamic-loading-of-custom-configuration-xml-using-reflection-in-c/

ozan.yarci
  • 89
  • 1
  • 5
-1

Define a look up table array 3 by 2. Look up rate value in array cell adjacent to transport type. Calculate cost based on rate.