3

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?

  • 3
    Why are you trying to get a Human object if you already have a Human object? – Winston Ewert Sep 2 '16 at 2:08
  • 2
    This is a poor example for polymorphism. Especially as it could all be handled via four enums. – user949300 Sep 2 '16 at 2:16
  • 4
    Re: Polymorphism I find this problematic. Intuitively, it is perfectly reasonable to be a grandfather, father, son, grandson, brother, uncle, and nephew all at once. These are all traits/relationships of the individual, not defining characteristics. It is more that human should have a collection of relationships. And they can change over time. Son becomes a father, while not stopping being a son. Then the problem can be represented using composition and not inheritance. Also, the 'type' the human would depend on an external reference (i.e., my son sees a father, my wife sees a husband). – Kristian H Sep 2 '16 at 11:24
  • 1
    @Leron: Can you state in prose what is the logic in GetHuman()? It seems to combine boolean flags in the input with a config setting, but what is the actual logic? – JacquesB Sep 2 '16 at 12:11
  • 1
    What I would also say, is you haven't considered all properties of the human. Can a father not also be a son? In which case maybe a | bitwise field could be the way to go – Richard Simpson Sep 2 '16 at 16:26
19

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
    }
}
  • Kudos for actually suggesting a simpler and better solution. – JacquesB Sep 2 '16 at 7:13
  • 3
    Nice answer. Note that you can (at least in Java) eliminate the switch by giving each enum the implementation of create(). – user949300 Sep 2 '16 at 21:09
  • Good answer. Not all of our systems are made out of objects; objects don't transition from physical reality into software. We try to eliminate as many if/else as possible, but somebody has to parse input/data/text (if/else) and create objects out of them. – blackpen Sep 5 '16 at 21:20
3

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.

2

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.

  • Gender uses Role, Role uses Male and Male uses Gender. It will work, but some part inside of me screams no. – Stephan Bijzitter Sep 6 '16 at 6:19
  • @StephanBijzitter sounds like the same part that was screaming at me. If you have a better way to completely remove conditionals please share. – candied_orange Sep 6 '16 at 6:26
1

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.

1

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)
  • What bothers me here is the amount of boolean properties. I need to make some decisions based on the values of those booleans and again I end up in an if-else situation. I wonder is it a common approach or it's most likely an indicator for design problem? What if I need to check for IsCousin, IsNephew and so on? Do you think it's ok to add as much boolean properties as you need in this kind of situation and do you see even better approach? As one chess gradnmaster says "If you see a good move, keep looking for even better". This is what I want to do here. – Leron_says_get_back_Monica Sep 6 '16 at 21:40
  • I do not know if it is useful in your problem domain to know if someone is a cousin or not. If it is, the IsCousin property would be appropriate. Ifs are not necessarily bad, in case of the property implementations they do not clutter and just clearly define the property. And they are nicely encapsulated in the property getter. If you have a long method with lots of ifs, it is often an indicator of poor design. – Martin Maat Sep 6 '16 at 22:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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