Refactoring if-else logic to reflect the OOP principles

Question

I have read some of the related questions regarding how we can refactor a code based on if/else if statements to follow closely the OOP principles but I have difficulties to apply this to a concrete use-case.

I have the following base class:

    public class Human
    {
        public bool IsMother { get; set; }

        public bool IsFather { get; set; }

        public bool Isdaughter { get; set; }
        public bool IsSon { get; set; }

        public string WhatAmI { get; set; }
    }

and several derived classes:

    public class Parent : Human
    {
        public Parent()
        {
            WhatAmI = "Parent";
        }
    }

    public class Daughter : Human
    {
        public Daughter()
        {
            WhatAmI = "Daughter";
        }
    }

    public class Son : Human
    {
        public Son()
        {
            WhatAmI = "Son";
        }
    }

So based on a several conditions I should return a specific type of Human.

for example if I have this:

   var human = new Human();
   human.IsFather = false;
   human.IsMother = false;
   human.IsSon = true;
   human.Isdaughter = false;

   var newHuman = HumanHelper.GetHuman(human);
   Console.WriteLine(newHuman.WhatAmI);

the console output is "Son" which is correct but the implementation of the GetHuman(Human human) method is bothering me because of the abuse of if-else statements. And the concrete implementation is this:

    public static class HumanHelper
    {
        private static string kindOfHuman = ConfigurationManager.AppSettings["kindOfHuman"];
        public static Human GetGuman(Human human)
        {
            if (kindOfHuman == "parent")
            {
                if (human.IsMother || human.IsFather)
                {
                    return new Parent();
                }
            }
            else if (kindOfHuman == "child")
            {
                if (!human.IsMother && !human.IsFather)
                {
                    if (human.Isdaughter)
                    {
                        return new Daughter();
                    }
                    else if (human.IsSon)
                    {
                        return new Son();
                    }
                }
            }
            throw new ArgumentException("Should not get here");
        }
    }

It is difficult to read and I think that I am doing something simple and relatively standard in a non simple and non-standard way. So could you help me with improving this code?

Answer 1

That code

var human = new Human();
human.IsFather = false;
human.IsMother = false;
human.IsSon = true;
human.Isdaughter = false;

var newHuman = HumanHelper.GetHuman(human);
Console.WriteLine(newHuman.WhatAmI);

Could be simplified to

var human = new Son();

Console.WriteLine(newHuman.WhatAmI);

I know it looks stupid but if you're assigning hardcoded boolean values, you could rather directly instanciate the real type.

If you over-simplified your real code to make it understandable to us, I suggest you to add an enum representing the different roles

enum Role {
    FATHER,
    MOTHER,
    SON,
    DAUGHTER
};

And make a factory responsible for human creation

public static Human create(Role role) {
    switch(role) {
        case Role.FATHER:
        case Role.MOTHER:
            return new Parent();
        case Role.SON:
            return new Son();
        case Role.DAUGHTER:
            return new Daughter();
    }
}

Therefore, you can get rid of these ugly IsXXX in Human and turn it into an interface

interface Human {
    string WhatAmI(); //pay attention that this method seems redundant with ToString()
    // other methods ?
}

With derived classes

public class Parent : Human
{
    public string WhatAmI() {
        return "parent";
    }
}

...

The most important thing to understand here is that when a codebase is cluttered with if/else it's always because we made some initial design choices like "add a method IsSon, IsFather, etc." or every time we decide to pass a "flag" as a method parameter: write(bool shouldOverride) always result in a code looking like

public void write(shouldOverride) {
    if(shouldOverride) {
        //some behaviour
    }
    else {
        //some other behaviour
    }
}

Answer 2

Some observations:

1. The class Human has a double responsibility, since it is both a specification for what kind of objects you want created and a base class for the actual created objects. These are actually separate responsibilities and involves separate properties. The first Human should be a separate class called something like HumanSpecification. (The IsXXX properties belongs to HumanSpecification but WhatAmI belongs to Human)

   And Human should be abstract, but this is not possible as long as you also use it as specification object.

2. The specification should only allow valid configurations. A set of booleans are not right for expressing exclusive option. Eg. it is possible to set both IsMother and IsFather, which does not make sense. Use an enum instead for exclusive options. It is not obvious if a human can be father and son at the same time, but the specification allow it.

3. The WhatAmI property seem superflous and potentially dangerous because it might tempt clients to actually use this property. For debugging purposes you should use ToString().

If you implement these three changes you will have much simpler and less error-prone code.

The most confusing part is the logic inside the factory method, where the flags are combined with a config setting to produce an instance.

Reverse engineering the logic form the code:

kindOfHuman |IsMother |IsFather |IsDaughter |IsSon |Result
parent      |T        |F        |*          |*     |Parent
parent      |F        |T        |*          |*     |Parent
parent      |T        |T        |*          |*     |Parent
parent      |F        |F        |*          |*     |!ERROR!
child       |T        |T        |*          |*     |!ERROR!
child       |T        |F        |*          |*     |!ERROR!
child       |F        |T        |*          |*     |!ERROR!
child       |F        |F        |T          |*     |Daughter
child       |F        |F        |F          |T     |Son
child       |F        |F        |F          |F     |!ERROR!

(Where * means any truth value.) You will immediately notice some weird logic: Like if IsSon is true, the result may still be a Daughter!

Looking closer at the logic it seems the rules in practice only distinguish between three cases in the input, so the input could be simplified to an enum with options PARENT, DAUGHTER, SON. This simplifies the rules a lot:

kindOfHuman |spec     |Result
parent      |PARENT   |Parent
parent      |DAUGHTER |!ERROR
parent      |SON      |!ERROR
child       |PARENT   |!ERROR
child       |DAUGHTER |Daughter
child       |SON      |Son

The output will be the same but the logic is simplified a lot! Since there are only three valid outputs, I believe the most clear and readable is an if, like:

   if (spec == PARENT && kindOfHuman == "parent") {
       return new Parent();
   } else if (spec == DAUGHTER && kindOfHuman == "child") {
      return new Daughter();
   } else if (spec == SON && kindOfHuman == "child") {
      return new Son();
   }
   throw new ArgumentException("Should not get here");

I think this is pretty readable, but if you expect to often add or remove rules in the future, you might consider a more data driven approach, e.g configuring rules something like:

   var rules = new[]{ 
      Rule("parent", PARENT, ()=>new Parent()), 
      Rule("child", DAUGHTER, ()=>new Daughter()), 
      Rule("child", Son, ()=>new Son()),
   }

But YAGNI.

Answer 3

refactor a code based on if/else if statements to follow closely the OOP principles

If you don't mind I'd like to show how double dispatch (or visitor pattern if you prefer) can provide some options here. I won't be using Human as a value object of bools. I actually won't be using any if's at all.

Pardon me for getting java all over your c# party here (it was all I happened to have handy and I don't post code I haven't tested). I trust you'll be able to follow it. Same trick works just fine in c#.

public class Human {
    Gender gender;
    Role role;

    public Human(Gender gender, Role role) {
        this.gender = gender;
        this.role = role;
    }

    @Override
    public String toString() {
        return whatAmI();
    }

    String whatAmI() {
        return gender.whatAmI(role);
    }

    static interface Gender {
        String whatAmI(Role role);
    }

    static interface Role {
        public String whatAmI(Male male);
        public String whatAmI(Female female);
    }

 

    static class Male implements Gender {
        @Override
        public String whatAmI(Role role) {
            return role.whatAmI(this);
        }
    }

    static class Female implements Gender {
        @Override
        public String whatAmI(Role role) {
            return role.whatAmI(this);
        }
    }

    static class Parent implements Role{
        @Override
        public String whatAmI(Male male) {
            return "Parent";
        }

        @Override
        public String whatAmI(Female female) {
            return "Parent";
        }
    }

    static class Child implements Role{
        @Override
        public String whatAmI(Male male) {
            return "Son";
        }

        @Override
        public String whatAmI(Female female) {
            return "Daughter";
        }        }

 

    public static void main (String args[]){
        System.out.println(new Human(new Female(), new Parent()).whatAmI());
        System.out.println(new Human(new Male(), new Parent()).whatAmI());
        System.out.println(new Human(new Female(), new Child()).whatAmI());
        System.out.println(new Human(new Male(), new Child()).whatAmI());
   }
}

Outputs

Parent
Parent
Daughter
Son

It's a tad over designed. It figures out if you're a father or a mother then proceeds to forget about it. Got caught up mimicking your behavior.

I trust this shows one way you can refactor if-else-if code with OOP principles. They've been refactored right out of existence. Not that this approach doesn't have it's own flaws. Gender and Role know too much about each other for my taste. But it's nice to just tell the human what to be, rather than ask.

Answer 4

So you have a Human object, and from that object you want to get an object of a derived class using the base class's object's properties. Why didn't you just create a Son in the first place ?

Also, in those derived classes you should override the IsMother and IsFather and Isdaughter and IsSon properties, or set them in the constructor, otherwise you have a Son whose IsSon returns false.

If you want a static method to return the desired object, here is what you should do :

public static class HumanHelper
{
    enum Gender { Male, Female };

    private static string kindOfHuman = ConfigurationManager.AppSettings["kindOfHuman"];

    public static Human GetGuman(Gender gender)
    {
        if (kindOfHuman == "parent")
        {
            return new Parent();
        }
        else if (kindOfHuman == "child")
        {
            if (gender == Gender.Female)
            {
                return new Daughter();
            }
            else if (gender == Gender.Male)
            {
                return new Son();
            }
        }
        throw new ArgumentException("Should not get here");
    }
}

That way, you can remove a few ifs.

Answer 5

Since everything you did was wrong I am not going to suggest changes, I will suggest something different.

You want OOP and particularly inheritance. This serves no purpose modeling your world though.

As someone else noticed, sons can be fathers and vice versa. I add to that that any father is a son. Whether someone is a father or not is not determined by his type but by his relationships with other family nembers.

The fact that your entities are humans seems irrelevant, you are dealing with family members. So here's the class tree:

FamilyMember

That's right, just one class. It has these properties:

• Name (string)
• Sex (enum)
• Father (a reference to the father object which is a FamilyMember)
• Mother (a reference to the mother object which is a FamilyMember)
• Sons and Daughters (collections of FamilyMembers)
• IsSon (boolean, true when Sex is male)
• IsDaughter (boolean, true when Sex is female)
• IsFather (boolean, true when either Sons or Daughters have at least one member)
• IsMother (boolean, true when either Sons or Daughters have at least one member)