Design pattern for replacing nested if statements (arrow anti-pattern)

Question

I noticed my code looks really ugly, and is hard to maintain. Basically I need do to some person check. Pseudo code is like this (BTW I can't "cut" anything in query, and it's not really the point of my question):

List<Person> persons = getPersonsBySomeQuery();
    
if (checkAnyPersonExists(persons)) {
  if (atLeastOneWithGivenNameExist(persons)) {
    if (noOneWithOtherNameExists(persons)) {
      filteredPersons = filterPersonsWithGivenName(persons);
      if (singleName(filteredPersons)) {
        // single person found
        if (someParameterFromQueryIsTrue) {
          if (someComplicatedLogicIsOK) {
            // found exactly one person, all OK!!!
          } else {
            // found exactly one person with given name, but not OK
          }
        } else {
           // found exactly one person but parameter from query is not OK
        }
      } else {
        // found only persons with given name, but more than one
      }
    } else {
      // found person(s) with given name, but also some with different name
    }
  } else {
    // found person(s), all have different name
  }
} else {
  // no one found
}

So I don't have that much experience with design patterns, but i read about them, and I started thinking I could implement Chain of Responsibility pattern. Like, every if-else could be one element in chain, and also this filter element on line 6 could also be "Chainable".

In the end it could look something like:

AbstractChainElement getCheckChain() {
   return new CheckAnyExist(
          new CheckOneWIthGivenNameExist(
          new CheckNoOneWithOtherNameExist(
          new FilterOnlyGivenName(
          new CheckIsSingleName(
          new CheckOtherParameters(
          new CheckWithSomeComplicatedLogic()))))));          
}

getCheckChain().doChain(persons);

Do you think it's good way to do it or is there anything more elegant?

With this I could construct chain for checking for different check types also, using factory or even abstract factory pattern.

Something like:

FactoryCheckThesePersonsByTheseParameters implements AbstractFactory {

     List<Person> getPersons() {
       // get persons with THIS query
     }

     AbstractChainElement getCheckChain() {
       // check persons THIS way
     }
}

FactoryCheckThatPersonsByThatParameters implements AbstractFactory {

     List<Person> getPersonsBySomeParameters() {
       // get persons with THAT query
     }

     AbstractChainElement getCheckChain() {
       // check persons THAT way
     }
}

score 2 · Answer 1 · 2018-03-31T09:51:07.467

This is not about design patterns, but coding style and optimizations.

You can combine conditions for one. i.e. This:

if (checkAnyPersonExists(persons)) {
  if (atLeastOneWithGivenNameExist(persons)) {
    if (noOneWithOtherNameExists(persons) {
    }
  }
}

First, This Seems to be making multiple queries on the same subject/collection. If you're using this to make queries to a database, then it'd be inefficient. You can do better by combining the criteria in a single query for example.

If this is not making queries to a database, then you can directly get the count of persons with the given name and based on the number you can determine what to do instead of repeating the query with slight variations.

On the other hand, using pattern is not always absolutely the best practice because it can add an overhead. A simple If-statement, switch-case or an if-not-statement with a return are better than a call to another function/method because it's faster.

I think you need to be more specific.

Edit: For example, if you have something similar to the following:

if (i == 1) {
    // action 1
} else {
    if (i == 2) {
        // action 2
    } else {
        if (i == 3) {
            // action 3
        } else {
            // action 4
        }
    }
}

You can use a switch statement

switch (i) {
    case 1:
        // action 1
        break;
    case 2:
        // action 2
        break;
    case 3:
        // action 3
        break;
    default:
        // action 4
        break;
}

Another example might be that if you have:

if (i == 1) {
    // a couple of sentences action
}
else if (x == 2) {
    // a couple of sentences action
}
else if (y == 3) {
    // a couple of sentences action
}

// no more cope beyond the if statements

This could be re-factored as:

if (i == 1) {
    // a couple of sentences action
    return;
}

if (x == 2) {
    // a couple of sentences action
    return;
}

and so on

Another technique if you have for example:

if(name.equals("abc")){
    do something
} else if(name.equals("xyz")){
    do something different
} else if(name.equals("mno")){
    do something different
} ......
.....
else{ 
   error
}

It would be better to use a Map where you map each string (if it's a finite known collection of strings you validate against) to a specific handler. You can also consider moving some of the code to a separate method

I took these examples from several pages on SO because as you see there is no absolute rule for this, it depends on the code itself. So what you can do is maybe have some analysis tools to work with you on the code. I think IntelliJ IDEA of JetBrain is an excellent IDE that comes with built-in cool features. Eclipse also has some features like so but not as much. Anyway, this comes with practice.

Hi and thanks for reply. It is single query, the one before if statements. Everything after that in my case is in Java. Count as you describe is possible ofcourse, but the point is i want to get rid of nested if statements. Imagine i got 15 if statements, and i need to insert 16th check but after 11th check. Or i need new check that checks same first 7 things, and 10-12, and 15th. I was just wondering is my idea of chain of responsibility naive, or i'm on right path? "Checking" is kinda exclusive btw - i go check by check - i don't collect all checks together - just like in a chain. — Nikola, Mar 30 '18 at 17:36

score 1 · Answer 2 · answered Apr 02 '18 at 09:17

I would stick with if/else statements (although I would flatten them like asm suggested). If you really want to use design pattern here, then Chain of Responsibility would do the job. Another option is to use State pattern. I've provided C# console app with example implementation.

public class Person
{
    public String Name { get; set; }
    public bool IsActive { get; set; }

    public Person(String name, bool isActive)
    {
        this.Name = name;
        this.IsActive = isActive;
    }
}

public class PersonChecker
{
    public PersonCheckerState CurrentState { get; set; }
    public String Name { get; private set; }
    public bool ShouldBeActive { get; private set; }

    public void Check(IEnumerable<Person> persons, string name, bool shouldBeActive)
    {
        this.Name = name;
        this.ShouldBeActive = shouldBeActive;
        CurrentState = new InitialState(this);
        CurrentState.Check(persons);
    }
}

public abstract class PersonCheckerState
{
    protected PersonChecker _personChecker;
    public PersonCheckerState(PersonChecker personChecker)
    {
        this._personChecker = personChecker;
    }
    public abstract void Check(IEnumerable<Person> persons);
}

public class InitialState : PersonCheckerState
{
    public InitialState(PersonChecker personChecker) : base(personChecker)
    {
    }

    public override void Check(IEnumerable<Person> persons)
    {
        if (persons != null && persons.Any())
        {
            _personChecker.CurrentState = new AnyPersonExistsState(_personChecker);
            _personChecker.CurrentState.Check(persons);
        }
        else
        {
            Console.WriteLine("No one found");
        }
    }
}

public class AnyPersonExistsState : PersonCheckerState
{
    public AnyPersonExistsState(PersonChecker personChecker) : base(personChecker)
    {
    }

    public override void Check(IEnumerable<Person> persons)
    {
        if (persons.Any(p => p.Name == _personChecker.Name))
        {
            _personChecker.CurrentState = new AtLeastOneWithGivenNameExistsState(_personChecker);
            _personChecker.CurrentState.Check(persons);
        }
        else
        {
            Console.WriteLine("Found person(s), all have different names");
        }
    }
}

public class AtLeastOneWithGivenNameExistsState : PersonCheckerState
{
    public AtLeastOneWithGivenNameExistsState(PersonChecker personChecker) : base(personChecker)
    {
    }

    public override void Check(IEnumerable<Person> persons)
    {
        if (persons.Any(p => p.Name != _personChecker.Name))
        {
            Console.WriteLine("Found person(s) with given name, but also some with different name");
        }
        else // All persons have the same name
        {
            _personChecker.CurrentState = new NoOneWithOtherNameExistsState(_personChecker);
            _personChecker.CurrentState.Check(persons);
        }
    }
}

public class NoOneWithOtherNameExistsState : PersonCheckerState
{
    public NoOneWithOtherNameExistsState(PersonChecker personChecker) : base(personChecker)
    {
    }

    public override void Check(IEnumerable<Person> persons)
    {
        if (persons.Where(p => p.Name == _personChecker.Name).ToList().Count == 1)
        {
            _personChecker.CurrentState = new SinglePersonFoundState(_personChecker);
            _personChecker.CurrentState.Check(persons);
        }
        else
        {
            Console.WriteLine("Found only persons with given name, but more than one");
        }
    }
}

public class SinglePersonFoundState : PersonCheckerState
{
    public SinglePersonFoundState(PersonChecker personChecker) : base(personChecker)
    {
    }

    public override void Check(IEnumerable<Person> persons)
    {
        Person person = persons.Where(p => p.Name == _personChecker.Name).Single();
        if(person.IsActive == _personChecker.ShouldBeActive)
        {
            Console.WriteLine("Found exactly one person, all OK!!!");
        }
        else
        {
            Console.WriteLine("Found exactly one person but parameter IsActive is not OK");
        }
    }
}

class Program
{
    static void Main(string[] args)
    {
        List<Person> persons = new List<Person>();
        persons.Add(new Person("Mike", true));
        //persons.Add(new Person("John", true));
        PersonChecker personChecker = new PersonChecker();
        personChecker.Check(persons, "Mike", true);
        Console.ReadLine();
    }
}

Thank you for the suggestion. I was thinking about State pattern also, but dunno is it easy to change algorithm then. Like, let's say i got algorithm that checks A, then B, then C (chained). And if i need to stay with this algorithm but also make another, that checks A, then C. How i tell state A to go to B in first algorithm and to C in second. ;) — Nikola, Apr 02 '18 at 19:56
In my example you can add another property to `PersonChecker`, that would be used to determine where A should jump to B or straight to C. You would use it the same way as I used `ShouldBeActive` in `SinglePersonFoundState`. — Krzysztof Błażełek, Apr 02 '18 at 20:38

tranmq · Answer 3 · 2020-04-23T01:47:20.180

0

There are many techniques that you can apply to reduce the nesting.

Below is a couple of links to reference.

https://blog.jetbrains.com/idea/2017/08/code-smells-deeply-nested-code/ https://blog.codinghorror.com/flattening-arrow-code/

For the example you gave, you can combine the conditions, or use the Chain of Responsibility pattern passing the list and the predicate, or the Command pattern.

edited Apr 23 '20 at 01:47

answered Apr 23 '20 at 01:38

tranmq

15,168
3
31
27

score 0 · Answer 4 · answered Sep 23 '21 at 09:53

Or something like:

function getValidPerson(someQuery) {

    List<Person> persons = getPersonsBySomeQuery(someQuery);

    if (!checkAnyPersonExists(persons)) {
        return
    }

    if (!atLeastOneWithGivenNameExist(persons)) {
        return
    }

    if (!noOneWithOtherNameExists(persons)) {
        return
    }

    filteredPersons = filterPersonsWithGivenName(persons);

    if (!singleName(filteredPersons)) {
        return
    }

    // single person found

    if (!someParameterFromQueryIsTrue) {
        return
    }

    if (!someComplicatedLogicIsOK) {
        return
    }

    // found exactly one person, all OK!!!

    return person 
}

Design pattern for replacing nested if statements (arrow anti-pattern)

4 Answers4