3
\$\begingroup\$

I have a class Call and 3 types of calls. Each type of call has a specific rate. So I have created an CallType enum and I will apply the right rate when making a call.

Here is my code so far:

public partial class Devices : Form
{
    public Devices()
    {
        InitializeComponent();
    }

    private void button1_Click(object sender, EventArgs e)
    {
        var call = new Call(CallType.Regular);
        call.MakeCall();
    }

    private void button2_Click(object sender, EventArgs e)
    {
        var call = new Call(CallType.Important);
        call.MakeCall();
    }

    private void button3_Click(object sender, EventArgs e)
    {
        var call = new Call(CallType.Critical);
        call.MakeCall();
    }
}

public enum CallType
{
    None,
    Regular,
    Important,
    Critical
};

public class Call
{
    private CallType callType;
    const decimal regularRate = 0.25M;
    private decimal ratePerMinute;

    public Call(CallType callType)
    {
        this.callType = callType;
    }

    public void MakeCall()
    {
        switch (callType)
        {
            case CallType.Regular:
                ratePerMinute = regularRate;
                break;
            case CallType.Important:
                ratePerMinute = regularRate * 1.5M;
                break;
            case CallType.Critical:
                ratePerMinute = regularRate * 2.5M;
                break;
        }
    }
}

One of my colleague said that this code is bad: not loosely coupling and not scalable. He suggested to use inheritance instead.

How can I transform this code to respect loose coupling and scalabilty?

\$\endgroup\$

3 Answers 3

4
\$\begingroup\$
public void MakeCall()
{
    switch (callType)
    {
        case CallType.Regular:
            ratePerMinute = regularRate;
            break;
        case CallType.Important:
            ratePerMinute = regularRate * 1.5M;
            break;
        case CallType.Critical:
            ratePerMinute = regularRate * 2.5M;
            break;
    }
}

Seeing this method I would say the same. You cannot easily add new call-types and you cannot easily change their rate because they are all in the same switch. You have to open it every time. The Open-Closed-Priciple is violated. Even worse, the rate is not a part of the call-type but some magic number inside the switch.

What you can do is to create a class for each call-type. This means forget about the enum. Remove everything but the basic stuff from the Call and give it a protected constructor:

public abstract class Call
{
    protected Call(decimal rate) => Rate = rate;

    public decimal Rate { get;}

    public void MakeCall()
    {
        // do something...
    }
}

Use it to derive other calls (I did it for one of them);

public class RegularCall : Call
{
    private const decimal DeafultRate = 0.25m;

    public RegularCall() : base(DeafultRate ) {}
}

Alternatively you can make the property abstract and return the value here instead of passing it via the constructor:

public class RegularCall : Call
{
    public override decimal Rate => 0.25m;
}

And use some other service to create an instance of a call and do some other stuff:

public class Caller
{
    public static void MakeCall<TCall>() where TCall : Call, new
    {
        var call = new TCall();
        call.MakeCall();
    }
}

Each button would the just do this:

private void button1_Click(object sender, EventArgs e)
{
    Caller.MakeCall<RegularCall>();
}

Now you can add as many calls as you like without modifying anything.

\$\endgroup\$
2
\$\begingroup\$

t3chb0t's answer seems like a pretty good solution, I don't feel like much can be improved on that but instead I'd like to touch on another part of your question.

Your event handlers all do the same thing but make the call with different number you can subscribe your button events in a single line like this using the enum, I wouldn't suggest using this right now because as I said the generic solution seems better to me, but I will leave it here just so you can see the technique:

public Devices()
{
    InitializeComponent();
    SetupEventHandlers();
}

private void SetupEventHandlers()
{
    button1.Click += (sender, args) => MakeCall(CallType.Regular);
    button2.Click += (sender, args) => MakeCall(CallType.Important);
    button3.Click += (sender, args) => MakeCall(CallType.Critical);
}

private void MakeCall(CallType callType)
{
    var call = new Call(callType);
    call.MakeCall();
}

Or like this with generics:

button1.Click += (sender, args) => Caller.MakeCall<RegularCall>();
button2.Click += (sender, args) => Caller.MakeCall<ImportantCall>();
button3.Click += (sender, args) => Caller.MakeCall<CriticalCall>();
\$\endgroup\$
1
\$\begingroup\$

Change MakeCall to virtual (or abstract) and override in the children of Call to adjust frequencies.

As it stands now, you'd need to add a new entry to your enum as well as a new case to your switch statement to make changes, whereas using inheritance you would only need to change one place to make a new call type, namely the new class. Any time you find yourself using an enum whose name logically comes out with the word Type, you should strongly consider making inherited types instead.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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