4
\$\begingroup\$

I am very new to design pattern. Recently, I learned about the Chain of Responsibility pattern, so I tried to practice it. These nested conditionals are what I'm trying to replace with polymorphism using Chain of Responsibility:

        if (gender == 'M')
        {
            if (age >= 1 && age <= 10){
                if (name.Contains("A"))
                    strInformation = "Male Child, contains 'A'";

                else (name.Contains("A") == false)
                    strInformation = "Male Child, not contains 'A'";
            }

            else if (age >= 11 && age <= 20) {
                if (name.Contains("A"))
                    strInformation = "Male Teen, contains 'A'";

                else (name.Contains("A") == false)
                    strInformation = "Male Teen, not contains 'A'";
            }
        }

        else if (gender == 'F')
        {
            if (age >= 1 && age <= 10){
                if (name.Contains("A"))
                    strInformation = "Female Child, contains 'A'";
                else if(name.Contains("A") == false)
                    strInformation = "Female Child, not contains 'A'";
            }

            else if (age >= 11 && age <= 20) {
                if (name.Contains("A"))
                    strInformation = "Female Teen, contains 'A'";
                else (name.Contains("A") == false)
                    strInformation = "Female Teen, not contains 'A'";
            }
        }

I know you guys can easily determine what that code is trying to achieve. Next, I'm going to show you my implementations of CoR pattern regarding that.

    //This is my main class that handles all the request
    public abstract class Handler
    {
        protected Handler handler;
        public void SetNext(Handler _handler){
            this.handler = _handler;
        }

        public virtual string Execute(string name, int age, char gender)
        {
            if(handler != null) {
                return handler.Execute(name, age, gender);
            }
            return "Not implemented error!!";
        }
    }

    public class Male : Handler
    {
        public override string Execute(string name, int age, char gender)
        {
            if (gender == 'M' && age >= 1 && age <= 10) {
                handler = new Malechild();
            }

            else if (gender == 'M' && age >= 11 && age <= 20) {
                handler = new MaleTeen();
            }
            return base.Execute(name, age, gender);
        }
    }

    public class Malechild : Handler
    {
        public override string Execute(string name, int age, char gender)
        {
            if(name.Contains("A"))
            {
                handler = new MaleChildContainsA();
            }

            else if(name.Contains("A") == false)
            {
                handler = new MaleChildNotContainsA();
            }

            return base.Execute(name, age, gender);
        }
    }

    public class MaleChildContainsA : Handler
    {
        public override string Execute(string name, int age, char gender)
        {
            return "Male Child, contains 'A'";
        }
    }

    public class MaleChildNotContainsA : Handler
    {
        public override string Execute(string name, int age, char gender)
        {
            return "Male Child, NOT contains 'A'";
        }
    }

And so on!!! I will not put the other code as I think it is prohibited by codereview to put so much code and less explanation and details, just imagine the next step, I also have MaleTeen, MaleTeenContainsA, MaleTeenNotcontainsA classes who do the same as the Male, MaleChildContainsA, and MaleChildNotContainsA classes. Aside from that, I still have classes for Female, I won't just put it. Imagine, how long my code is. It's horrible..

I implemented it on my Main Method like this..

        Handler male = new Male();
        Handler female = new Female();

        male.SetNext(female);

        var m = male.Execute(name, age, gender);

        Console.WriteLine("Answer: {0}", m);

I've got no error by doing this code, but I know I did it wrong. Maybe I over over-engineered it which is wrong according to many articles that I've read. Now, my question is, how to do it right? If you encounter the same problem as that, what is the right way of doing it? Is it wrong to implement Chain of Responsibility for that problem?

\$\endgroup\$
1
  • 1
    \$\begingroup\$ You're making the same mistake with both approaches: the 3 checks (gender, age and name) are unrelated, but you're writing code for every possible combination (resulting in an explosion of if/else statements and classes). Also note that letting handlers alter their chain is probably a bad idea: try calling male.Execute twice, first with a male and then with a female - it'll fail to recognize the female. I think the KISS (Keep It Stupidly Simple) and DRY (Don't Repeat Yourself) patterns are more useful here. ;) \$\endgroup\$ Commented Jul 5, 2017 at 15:25

2 Answers 2

9
\$\begingroup\$

If all you want to do is set the display string you have a much easier choice:

public string GetDisplayStringForPerson(string name, int age, char gender)
{
    // If using C#6, you can use string interpolation here instead.
    return string.Format("{0} {1}, {2} 'A'",
        GetGenderDisplay(gender),
        GetAgeDisplay(age),
        GetNameContainsADisplay(name));
}

public string GetGenderDisplay(char gender)
{
    if (gender == 'M') 
    {
        return "Male";
    }
    if (gender == 'F') 
    {
        return "Female";
    }
    return "Other/non binary";
} 

public string GetAgeDisplay(int age)
{
    if (age >= 1 && age <= 10) 
    {
        return "Child";
    }
    if (age >= 10 && age < 20) 
    {
        return "Teen";
    }
    return "Other";
}

public string GetNameContainsADisplay(string name)
{
    return name.Contains("A") ? "contains" : "doesn't contain";
}

A better alternative would be to create a Person class to encapsulate gender, age and name.

Chain of responsibility doesn't fit here IMO. You only have 3 independent factors making up part of a string - CoR is too complex for it.

\$\endgroup\$
3
\$\begingroup\$

I think @RobH's answer is spot on and wholeheartedly agree with him that Chain of Responsibility doesn't fit here. The comment to your original post by @Pieter explains it well by mentioning KISS and DRY.

Let me offer a more practical example. Let's say you wrote this for a specific application for a ticketing system and the price of a ticket depends upon your age classification. Consider a year from now that the company who hired you wants to you or someone else to modify the age classifications by adding 2 more levels. With your current implementation, you have to change this code twice. This opens the door that you or someone else forgot to change it everywhere, or else the changes were slightly different for Male than Female, when they really should be the same.

If you follow RobH's advice, you only need to change this once:

public string GetAgeDisplay(int age)
{
    if (age < 2) 
    {
        return "Infant";
    }
    if (age < 10)
    {
        return "Child";
    }
    if (age < 18) 
    {
        return "Teen";
    }
    if (age < 65)
    {
        return "Adult";
    }
    return "Senior";
}

and you don't have to worry that a 19 year old Male is considered an Adult, whereas a 19 year old female is a Teen because someone forgot to correctly update all the code in all the places.

\$\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.