-5
\$\begingroup\$

I want to refactor below code.

Do() method below gets the position number from other component "_calComponent.GetPosition()" and compares the value with the pf control positions (PfControlPositions).

As you can see there are many if conditions that I had to use here. I did not like this approach. I am not able to write the switch case also because the position value comes dynamically and hence switch case throws compile time error.

I have a constraint that I can only modify DoSomething class.

Can you please suggest?

public interface ICalComponent
{
    int GetPosition();
}

public class DoSomething
{
    private readonly ICalComponent _calComponent;
    public DoSomething(ICalComponent calComponent)
    {
        _calComponent = calComponent;
    }
    public void Do()
    {
        var functionality = new Dictionary<ControlAction, ControlFunctionality>();
        int number = _calComponent.GetPosition(); 

        if (number == PfControlPositions.pf1PositionOff)
        {
            functionality[ControlAction.Medium] = ControlFunctionality.DoAction1;
        }

        if (number == PfControlPositions.pf1PositionOn)
        {
            functionality[ControlAction.Medium] = ControlFunctionality.DoAction2;
        }

        if (number == PfControlPositions.pf1PositionOn)
        {
            functionality[ControlAction.Medium] = ControlFunctionality.DoAction3;
        }

        if (number == PfControlPositions.pf2PositionOff)
        {
            functionality[ControlAction.Medium] = ControlFunctionality.DoAction4;
        }

        if (number == PfControlPositions.pf1PositionOn)
        {
            functionality[ControlAction.Medium] = ControlFunctionality.DoAction5;
        }

        if (number == PfControlPositions.pf1PositionOff)
        {
            functionality[ControlAction.Medium] = ControlFunctionality.DoAction6;
        }

        if (number == PfControlPositions.pf2PositionOn)
        {
            functionality[ControlAction.Medium] = ControlFunctionality.DoAction7;
        }

        if (number == PfControlPositions.pf2PositionOff)
        {
            functionality[ControlAction.Medium] = ControlFunctionality.DoAction8;
        }

        if (number == PfControlPositions.pf3PositionOn)
        {
            functionality[ControlAction.Medium] = ControlFunctionality.DoAction9;
        }

        if (number == PfControlPositions.pf3PositionOff)
        {
            functionality[ControlAction.Medium] = ControlFunctionality.DoAction10;
        }
    }
}

public class PfControlPositions
{
    public static ControlPosition pf1PositionOff = new ControlPosition(0);
    public static ControlPosition pf1PositionOn = new ControlPosition(1);

    public static ControlPosition pf2PositionOn = new ControlPosition(2);
    public static ControlPosition pf2PositionOff = new ControlPosition(3);

    public static ControlPosition pf3PositionOn = new ControlPosition(4);
    public static ControlPosition pf3PositionOff = new ControlPosition(5);

    public static ControlPosition pf4PositionOn = new ControlPosition(6);
    public static ControlPosition pf4PositionOff = new ControlPosition(7);

    public static ControlPosition pf5PositionOn = new ControlPosition(8);
    public static ControlPosition pf5PositionOff = new ControlPosition(9);
}

public class ControlPosition 
{
    private readonly UInt32 _physicalPosition;

    public ControlPosition(UInt32 physicalStepPosition)
    {
        _physicalPosition = physicalStepPosition;
    }

    public static implicit operator int(ControlPosition position)
    {
        return (int)position._physicalPosition;
    }
}

public enum ControlFunctionality
{
    DoAction1,
    DoAction2,
    DoAction3,
    DoAction4,
    DoAction5,
    DoAction6,
    DoAction7,
    DoAction8,
    DoAction9,
    DoAction10,
}

public enum ControlAction
{
    Dial = 0,
    Short = 1,
    Medium = 2
}

Thank you very much !!

\$\endgroup\$

closed as off-topic by Heslacher, 200_success Oct 20 '16 at 16:17

This question appears to be off-topic. The users who voted to close gave this specific reason:

If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ Welcome to Code Review! Questions must involve real code that you own or maintain. Pseudocode, hypothetical code, or stub code should be replaced by a concrete example. Questions seeking an explanation of someone else's code are also off-topic. \$\endgroup\$ – Heslacher Oct 20 '16 at 15:55
  • \$\begingroup\$ DoSomething and other placeholders are usually frowned upon on this site. If possible please edit your post to include your real, actual code - if that's not possible, please edit your post to use meaningful identifiers anyway. Anonymized code is okay, but not if it starts looking like a Stack Overflow-ish "MCVE". \$\endgroup\$ – Mathieu Guindon Oct 20 '16 at 15:58
1
\$\begingroup\$

You can replace such if mess with a dictionary like:

public void Do()
    {
        var functionality = new Dictionary<ControlAction, ControlFunctionality>();
        Dictionary<PfControlPositions, ControlFunctionality> functions = new Dictionary<PfControlPositions, ControlFunctionality>(){
            {PfControlPosition.pf1PositionOff, ControlFunctionality.DoAction1},
            {PfControlPosition.pf1PositionOn, ControlFunctionality.DoAction2},
            ... etc ...
        };

        functionality[ControlAction.Medium] = functions[_calComponent.GetPosition()];

    }
\$\endgroup\$

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