I recently came across a piece of code at work that has a repeating if-else condition that checks on an enum called OperationType :

public enum OperationType
{ A, B }

Right now the class's job is to run an operation either on device A or on device B, while reading from a SharedDevice and store some values basically for an X,Y plot. We record the characteristics of the SharedDevice in the function of DeviceA or DeviceB. The problem is that we need to iterate over a list of different parameters and send them to the SharedDevice. This list is different for device A and for device B.

Device class:

public class Device
{
    public double CurrentValue { get; }
    public DeviceParameters Parameters { get; set; }
}

And here is the class responsible for executing this operation:

public class MyOperationExecuter
{
    public Device SharedDevice { get; }
    public Device DeviceA { get; }
    public Device DeviceB { get; }

    public List<DeviceParameters> ParametersA { get; }
    public List<DeviceParameters> ParametersB { get; }

    public List<double> XValuesOfA { get; }
    public List<double> YValuesOfA { get; }
    public List<double> XValuesOfB { get; }
    public List<double> YValuesOfB { get; }

    public void DoMyOperation(OperationType operationType)
    {
        List<DeviceParameters> changingDeviceParameters;

        if (operationType == OperationType.A)
        {
            changingDeviceParameters = ParametersA;
        }
        else
        {
            changingDeviceParameters = ParametersB;
        }

        if (operationType == OperationType.A)
        {
            XValuesOfA.Clear();
            YValuesOfA.Clear();
        }
        else
        {
            XValuesOfB.Clear();
            YValuesOfB.Clear();
        }

        foreach (var parameters in changingDeviceParameters)
        {
            // set the device parameters
            SharedDevice.Parameters = parameters;

            // retrieve the device readings and store the values in the correct dataprovider
            if (operationType == OperationType.A)
            {
                XValuesOfA.Add(DeviceA.CurrentValue);
                YValuesOfA.Add(SharedDevice.CurrentValue));
            }
            else
            {
                XValuesOfB.Add(DeviceB.CurrentValue);
                YValuesOfB.Add(SharedDevice.CurrentValue);
            }
        }

        // save updated x,y data
        Save();
    }
}

As you can see there is a repeating if statement which is not very future proof, since we have to check for the enum in every single step. Also we might need to add an C-type device which would result in an ever growing switch statement. We might also need to execute operations on both A and B. How should I refactor this operation so I can keep extending it without this always repeating if-else logic?

CodePudding user response：

A fairly simple way would be to declare a variable representing A or B:

var XValues = operationType == OperationType.A ? XValuesOfA : XValuesOfB;

then you can just use XValues. Do the same for DeviceA. If you have more operations you could use a switch expression.

A neater solution would be to make separate objects containing everything needed for A or B, so your class could simply check the operation type and then delegate all the work to respective object. I.e.

public class MyDevice
{
    public Device SharedDevice { get; }
    public Device Device { get; }

    public List<DeviceParameters> Parameters { get; }

    public List<double> XValuesOf { get; }
    public List<double> YValuesOf { get; }

    public void DoMyOperation()
    {
    ...
    }
}

I would also recommend using a single list containing both X and Y values, something like a Vector2. I find this easier to use, and helps avoid repeating code.

CodePudding user response：

Without changing class fields/properties I'd go with new method:

private void SetParameters(List<DeviceParameters> parameters, List<double> xValues, List<double> yValues, Device device)
{
    xValues.Clear();
    yValues.Clear();
    foreach(var parameter in parameters)
    {
        SharedDevice.Parameters = parameter;
        xValues.Add(device.CurrentValue);
        yValues.Add(SharedDevice.CurrentValue);
    }
}

And then in DoMyOperation it's enough to:

if (operationType == OperationType.A)
{
    SetParameter(ParametersA, XValuesOfA, YValuesOfA, DeviceA);
}
else
{
    SetParameter(ParametersB, XValuesOfB, YValuesOfB, DeviceB);
}

CodePudding user response：

You should add new class. Which will be used to define device type specific properties.

A class like this;

public class MyDeviceValues
{
    public MyDeviceValues(List<DeviceParameters> parameters, List<double> xValuesOf, List<double> yValuesOf)
    {
        Parameters = parameters;
        XValues = xValuesOf;
        YValues = yValuesOf;
    }

    public List<DeviceParameters> Parameters { get; }

    public List<double> XValues { get; }

    public List<double> YValues { get; }
}

So, you can have a generic DoMyOperation function. It will be like this:

public void DoMyOperation(MyDeviceValues myDeviceValues)
{
    var changingDeviceParameters = myDeviceValues.Parameters;

    myDeviceValues.XValues.Clear();
    myDeviceValues.YValues.Clear();

    foreach (var parameters in changingDeviceParameters)
    {
        // set the device parameters
        SharedDevice.Parameters = parameters;

        // retrieve the device readings and store the values in the correct dataprovider
        myDeviceValues.XValues.Add(DeviceA.CurrentValue);
        myDeviceValues.YValues.Add(SharedDevice.CurrentValue);
    }

    // save updated x,y data
    Save();
}

Here is the refactored version of the whole code you pasted:

https://dotnetfiddle.net/dLyJl9