Code refactoring c# question around several if statements performing the same check

I have a method that has a load of if statements that seems a bit silly although I'm not sure how to improve the code.

Here's an example. This logic was inside the view which is now in the c开发者_如何学运维ontroller which is far better but is there something I'm missing, maybe a design pattern that stops me having to check against panelCount < NumberOfPanelsToShow and handling the panelCount every condition? Maybe not, just feels ugly!

Many thanks

if (model.Train && panelCount < NumberOfPanelsToShow)
{
    panelTypeList.Add(TheType.Train);
    panelCount++;
}
if (model.Car && panelCount < NumberOfPanelsToShow)
{
    panelTypeList.Add(TheType.Car);
    panelCount++;
}
if (model.Hotel && panelCount < NumberOfPanelsToShow)
{
    panelTypeList.Add(TheType.Hotel);
    panelCount++;
}
...

Assuming Model.Train, Model.Car, Model.Plane are just boolean indicators as to the type of "model" (instead of you creating Train : Model, Plane : Model etc)

public enum ModelType { Train, Car, Plane };

public class Model { 
    ... 
    public ModelType {get;set;}
    ... 
}

and in your test:

if(panelCount < NumberOfPanelsToShow)
switch (Model.ModelType)
{
    case ModelType.Train :
        ...
        break;

    case ModelType.Plane :
...
}

HOWEVER, since Car, PLane and Train are different, you really SHOULD have a base type Model, derive Car, Plane and Train from Model and then you can overload methods to handle each type

if (panelCount < NumberOfPanelsToShow)
{
    panelCount += AddModel(model);
}

private int AddModel(Plane model)
{ // do plane stuff here and on success return 1 else 0; }

private int AddModel(Train model)
{ // do train stuff here and on success return 1 else 0; }

private int AddModel(Car model)
{ // do car stuff here and on success return 1 else 0; }

Usually the way to refactor this is to do it polymorphicly with the responsibility for deciding what to do being with the object, but as your decision is based on different aspects of the same object, I'm not sure you can do that here.

I would be tempted to refactor to a method instead:

public AddIfSpace(bool thingToTest, TheType typeToAdd, ref currentCount)
{
    if (currentCount<NumberOfPanelsToShow && thingToTest)
    {
     panelTypeList.Add(typeToAdd);
     currentCount++;
    }
}

then call it:

AddIfSpace(model.Car,TheType.Car,ref panelCount);
AddIfSpace(model.Train,TheType.train,ref panelCount);
AddIfSpace(model.Hotel,TheType.Hotel,ref panelCount);

you might need fewer or more params depending on scope of your variables, and you could make it an extension method on the type of panelTypeList if that has restricted scope so you end up with something like:

panelTypeList.AddIfSpace(model.Car,TheType.Car,ref panelCount);
panelTypeList.AddIfSpace(model.Train,TheType.Train,ref panelCount);
panelTypeList.AddIfSpace(model.Hotel,TheType.Hotel,ref panelCount);

This at least avoids the repeated logic in your code and means that you have a single place to modify if you want to change how the check works, and if you name the method nicely, should convey the intention and make the code more readable.

I'm going to take a stab a this, making some assumptions.

It looks like you have some sort of "Panel" class which you're trying to fill up with information from data-model objects (Car, Hotel, Train...). Instead of that, why not use an MVVM type pattern with a polymorphic Panel class (or an IPanel interface), and subclasses of CarPanel, HotelPanel, TrainPanel, etc... With ViewModel objects, your view code gets even simpler:

var newPanel = PanelFactory.GetPanel(data);  //Returns a CarPanel, HotelPanel, TrainPanel...
panelTypeList.Add(newPanel);
panelCount++;

Dont have VS right now to test compiling the code, but this should convey the logic. You can build up the array list with as many of the types without having to repeat the test logic with big if-else or switch statements.

    TheType[] typesWithPanel = new TheType[] { TheType.Train, TheType.Car, TheType.Hotel};
    if (typesWithPanel.Contains(model.ModelType) && panelCount < NumberOfPanelsToShow)
    {
        panelTypeList.Add(model.ModelType);
        panelCount++;
    }

What I'd probably do is have an enum type on the Model to switch against.

if(panelCount < NumberOfPanelsToShow)
{
  switch(model.modelType)
  {
   case Model.Car:
    // do stuff
  break;
  }
  panelCount++;
}

We'll need more information though really to determine how to handle the cases and how many conditions can apply at any given time.

UPDATE

Given the additional information you've provided, some of the other answers here seem more appropriate. This answer won't be sufficient given your current case.

Not sure what you'd call this design pattern. But I read once that whenever you find yourself doing repetitive blocks of code for different types (as you're doing) that you are better off putting that functionality into the objects themselves.

You can break it out like this:

private const NumberOfPanelsToShow = 4;
private int panelCount = 0;

private void addToPanel(IPanelAddable element, PanelList panelTypeList) {
  if(panelCount < NumberofPanelsToShow) {
    element.addToPanelTypeList(panelTypeList);
    panelCount++;
  }
}

IPanelAddable can be an interface that defines the addToPanelTypeList method (or whatever). Then in your original method:

if (model.Train)
{
    addToPanel(TheType.Train);
}
if (model.Car)
{
    addToPanel(TheType.Car);
}
if (model.Hotel)
{
    addToPanel(TheType.Hotel);
}

Actually you want to avoid the above, simplify the above /w a base class, etc.... Then your TheType.* objects would obviously need to implement the "addToPanelTypeList" method:

panelTypeList.Add(this);

You can use interfaces or inheritance to achieve the same goal...

You could always take a look at the specification pattern. Quite handy for boolean logic:

public interface ISpecification<T>
{
    public bool IsSatisfiedBy(T candidate);
}