How to tackle a ‘branched’ arrow head anti-pattern? [duplicate]

I recently read this question that features, the arrow anti-pattern.

I have something similar in code I’m trying to refactor except that it branches. It looks a little something like this:

Plain text
Copy to clipboard
Open code in new window
EnlighterJS 3 Syntax Highlighter
<code>if(fooSuccess==true&&barSuccess!=true){
if(mooSuccess==true){
.....
}else if (mooSuccess!=true){
.....
}
}else if(fooSuccess!=true&&barSuccess==true){
if(mooSuccess==true){
.....
}else if (mooSuccess!=true){
if(cowSuccess==true){
.....
}else if (cowSuccess!=true){
.....
}
}
}
......
</code>
<code>if(fooSuccess==true&&barSuccess!=true){ if(mooSuccess==true){ ..... }else if (mooSuccess!=true){ ..... } }else if(fooSuccess!=true&&barSuccess==true){ if(mooSuccess==true){ ..... }else if (mooSuccess!=true){ if(cowSuccess==true){ ..... }else if (cowSuccess!=true){ ..... } } } ...... </code>
if(fooSuccess==true&&barSuccess!=true){
    if(mooSuccess==true){
           .....
    }else if (mooSuccess!=true){
           .....
    }
}else if(fooSuccess!=true&&barSuccess==true){
    if(mooSuccess==true){
           .....
    }else if (mooSuccess!=true){
        if(cowSuccess==true){
                    .....
        }else if (cowSuccess!=true){
                    .....
        }
    }
}
......

In short it looks like this

Plain text
Copy to clipboard
Open code in new window
EnlighterJS 3 Syntax Highlighter
<code>if
if
if
if
do something
endif
else if
if
if
do something
endif
endif
else if
if
if
do something
endif
endif
endif
</code>
<code>if if if if do something endif else if if if do something endif endif else if if if do something endif endif endif </code>
if
   if
     if
       if
         do something
       endif
    else if
     if
       if
         do something
        endif
    endif
    else if
     if
       if
         do something
        endif
    endif
 endif

Outline borrowed from Coding Horror article on the subject

And the code goes on through different permutations of true and false for various flags. These flags are set somewhere ‘up’ in the code elsewhere, either by user input or by the result of some method.

How can I make this kind of code more readable? My intention is that eventually I will have a Bean-type object that contains all the choices the previous programmer tried to capture with this branching anti-pattern. For instance, if in the outline of the code above we really do only have three, I have an enum set inside that bean:

Plain text
Copy to clipboard
Open code in new window
EnlighterJS 3 Syntax Highlighter
<code>enum ProgramRouteEnum{
OPTION_ONE,OPTION_TWO,OPTION_THREE;
boolean choice;
void setChoice(boolean myChoice){
choice = myChoice;
}
boolean getChoice(){
return choice;
}
}
</code>
<code>enum ProgramRouteEnum{ OPTION_ONE,OPTION_TWO,OPTION_THREE; boolean choice; void setChoice(boolean myChoice){ choice = myChoice; } boolean getChoice(){ return choice; } } </code>
enum ProgramRouteEnum{
    OPTION_ONE,OPTION_TWO,OPTION_THREE;

    boolean choice;
    void setChoice(boolean myChoice){
        choice = myChoice;
    }
    boolean getChoice(){
        return choice;
    }
}

Is this an acceptable cure? Or is there a better one?

2

The code can be simplified like this:

Plain text
Copy to clipboard
Open code in new window
EnlighterJS 3 Syntax Highlighter
<code>boolean condA = ( fooSuccess && !barSuccess && mooSuccess )
boolean condB = ( fooSuccess && !barSuccess && !mooSuccess )
boolean condC = (!fooSuccess && barSuccess && mooSuccess )
boolean condD = (!fooSuccess && barSuccess && !mooSuccess && cowSuccess)
boolean condE = (!fooSuccess && barSuccess && !mooSuccess && !cowSuccess)
if (condA) {
....
return;
}
if (condB) {
....
return;
}
... and so son
if (condE) {
....
return;
}
</code>
<code>boolean condA = ( fooSuccess && !barSuccess && mooSuccess ) boolean condB = ( fooSuccess && !barSuccess && !mooSuccess ) boolean condC = (!fooSuccess && barSuccess && mooSuccess ) boolean condD = (!fooSuccess && barSuccess && !mooSuccess && cowSuccess) boolean condE = (!fooSuccess && barSuccess && !mooSuccess && !cowSuccess) if (condA) { .... return; } if (condB) { .... return; } ... and so son if (condE) { .... return; } </code>
boolean condA = ( fooSuccess  && !barSuccess &&  mooSuccess )
boolean condB = ( fooSuccess  && !barSuccess && !mooSuccess )
boolean condC = (!fooSuccess  &&  barSuccess &&  mooSuccess )
boolean condD = (!fooSuccess  &&  barSuccess && !mooSuccess &&  cowSuccess)
boolean condE = (!fooSuccess  &&  barSuccess && !mooSuccess && !cowSuccess)

if (condA) {
    ....
    return;
}
if (condB) {
    ....
    return;
}

... and so son 

if (condE) {
    ....
    return;
}

In essence:

  • Evaluate complex conditions into a single boolean
  • Since conditions are exclusive (as it’s typically the case in nested arrow heads) you don’t have to nest the ifs, just add a return so that no other block is run.
  • This reduces the cyclomatic complexity enormously and makes the code trivial.
  • Having what exactly gets run in every case, you can even do a Karnaugh table to identify and remove redundant condition combinations, just as they do in logic circuits design. But you didn’t provide that in the example.
  • Be sure to extract all this logic into its own method so the return statements don’t interfere with blocks that have to be run anymay.
  • Descriptive names should be chosen for conA..condF.

9

user61852 has a pretty good answer solving the general problem of simplifying nested conditionals. However, I was intrigued with your proposed state machine-like solution, and wanted to illustrate how that can sometimes be preferable to a set of binary flags.

It all depends on the sequence of obtaining the flags’ values and how many combinations are valid. If you have n flags, you have 2n states. For 4 flags, that’s 16 states, and as you observed, only 3 of them may have any useful meaning. In situations like that, using a state variable instead can simplify things greatly.

Let’s say you have a lock that will open if 4 keys are pressed in the right order. If you press any key incorrectly, it immediately resets back to the start of the sequence. A very naïve way to implement a keypress handler using binary flags is:

Plain text
Copy to clipboard
Open code in new window
EnlighterJS 3 Syntax Highlighter
<code>void key_pressed(char key)
{
if (!key1correct)
{
if (key == pin[0])
{
key1correct = true;
}
}
else if (!key2correct)
{
if (key == pin[1])
{
key2correct = true;
}
else
{
key1correct = false;
}
}
else if (!key3correct)
{
if (key == pin[2])
{
key3correct = true;
}
else
{
key1correct = false;
key2correct = false;
}
}
else
{
if (key == pin[3])
{
key4correct = true;
}
else
{
key1correct = false;
key2correct = false;
key3correct = false;
}
}
if (key1correct && key2correct && key3correct && key4correct)
{
open_lock();
key1correct = false;
key2correct = false;
key3correct = false;
key4correct = false;
}
}
</code>
<code>void key_pressed(char key) { if (!key1correct) { if (key == pin[0]) { key1correct = true; } } else if (!key2correct) { if (key == pin[1]) { key2correct = true; } else { key1correct = false; } } else if (!key3correct) { if (key == pin[2]) { key3correct = true; } else { key1correct = false; key2correct = false; } } else { if (key == pin[3]) { key4correct = true; } else { key1correct = false; key2correct = false; key3correct = false; } } if (key1correct && key2correct && key3correct && key4correct) { open_lock(); key1correct = false; key2correct = false; key3correct = false; key4correct = false; } } </code>
void key_pressed(char key)
{
   if (!key1correct)
   {
      if (key == pin[0])
      {
         key1correct = true;
      }
   }
   else if (!key2correct)
   {
       if (key == pin[1])
       {
           key2correct = true;
       }
       else
       {
           key1correct = false;
       }
   }
   else if (!key3correct)
   {
       if (key == pin[2])
       {
           key3correct = true;
       }
       else
       {
           key1correct = false;
           key2correct = false;
       }
   }
   else
   {
       if (key == pin[3])
       {
           key4correct = true;
       }
       else
       {
           key1correct = false;
           key2correct = false;
           key3correct = false;
       }
   }

   if (key1correct && key2correct && key3correct && key4correct)
   {
       open_lock();
       key1correct = false;
       key2correct = false;
       key3correct = false;
       key4correct = false;
   }
}

A simplified, flattened version using binary flags (like user61852’s answer) looks like this:

Plain text
Copy to clipboard
Open code in new window
EnlighterJS 3 Syntax Highlighter
<code>void key_pressed(char key)
{
if (!key1correct && key == pin[0])
{
key1correct = true;
return;
}
if (key1correct && !key2correct && key == pin[1])
{
key2correct = true;
return;
}
if (key1correct && key2correct && !key3correct && key == pin[2])
{
key3correct = true;
return;
}
if (key1correct && key2correct && key3correct && key == pin[3])
{
open_lock();
key1correct = false;
key2correct = false;
key3correct = false;
return;
}
key1correct = false;
key2correct = false;
key3correct = false;
}
</code>
<code>void key_pressed(char key) { if (!key1correct && key == pin[0]) { key1correct = true; return; } if (key1correct && !key2correct && key == pin[1]) { key2correct = true; return; } if (key1correct && key2correct && !key3correct && key == pin[2]) { key3correct = true; return; } if (key1correct && key2correct && key3correct && key == pin[3]) { open_lock(); key1correct = false; key2correct = false; key3correct = false; return; } key1correct = false; key2correct = false; key3correct = false; } </code>
void key_pressed(char key)
{
    if (!key1correct && key == pin[0])
    {
        key1correct = true;
        return;
    }

    if (key1correct && !key2correct && key == pin[1])
    {
        key2correct = true;
        return;
    }

    if (key1correct && key2correct && !key3correct && key == pin[2])
    {
        key3correct = true;
        return;
    }

    if (key1correct && key2correct && key3correct && key == pin[3])
    {
        open_lock();
        key1correct = false;
        key2correct = false;
        key3correct = false;
        return;
    }

    key1correct = false;
    key2correct = false;
    key3correct = false;
}

That’s a lot easier to read, but still in both of these solutions you have states like key2correct && !key1correct that are completely invalid and unused. Whereas using a state variable num_correct_keys instead of the binary flags looks like this:

Plain text
Copy to clipboard
Open code in new window
EnlighterJS 3 Syntax Highlighter
<code>void key_pressed(char key)
{
if (key == pin[num_correct_keys])
++num_correct_keys;
else
num_correct_keys = 0;
if (num_correct_keys == 4)
{
open_lock();
num_correct_keys = 0;
}
}
</code>
<code>void key_pressed(char key) { if (key == pin[num_correct_keys]) ++num_correct_keys; else num_correct_keys = 0; if (num_correct_keys == 4) { open_lock(); num_correct_keys = 0; } } </code>
void key_pressed(char key)
{
    if (key == pin[num_correct_keys])
        ++num_correct_keys;
    else
        num_correct_keys = 0;

    if (num_correct_keys == 4)
    {
        open_lock();
        num_correct_keys = 0;
    }
}

Granted, this is a contrived example, but people often write code like the first version in less obvious situations, sometimes spread across multiple files. Using a state variable often greatly simplifies code, especially when the flags represent a sequence of events.

I’m not sure your example is a particularly good one for this question, but it’s possible to condense it to be much simpler and still preserve the same logic:

Plain text
Copy to clipboard
Open code in new window
EnlighterJS 3 Syntax Highlighter
<code>if(fooSuccess && !barSuccess)
{
if(mooSuccess)
{
// .....
}
else
{
// .....
}
}
else if (!fooSuccess && barSuccess)
{
if(mooSuccess)
{
// .....
}
else if(cowSuccess)
{
// .....
}
else
{
// .....
}
}
</code>
<code>if(fooSuccess && !barSuccess) { if(mooSuccess) { // ..... } else { // ..... } } else if (!fooSuccess && barSuccess) { if(mooSuccess) { // ..... } else if(cowSuccess) { // ..... } else { // ..... } } </code>
if(fooSuccess && !barSuccess)
{
    if(mooSuccess)
    {
        // .....
    }
    else
    {
        // .....
    }
}
else if (!fooSuccess && barSuccess)
{
    if(mooSuccess)
    {
        // .....
    }
    else if(cowSuccess)
    {
        // .....
    }
    else 
    {
        // .....
    }
}

Notice how there’s no more than one level deep of ifs. To clean this up, I took the following steps:

  • Never check whether a boolean value is == true, != true, or != false. Occasionally I’ve found it worth checking whether it == false to make it more apparent that that’s the expected case, but even that should be exceptional.
  • If your else block consists of nothing but an if/else block, then you can merge it with the parent else.
  • If your if statement only has one input/variable, you don’t need to check the inverse for the else.

Trang chủ Giới thiệu Sinh nhật bé trai Sinh nhật bé gái Tổ chức sự kiện Biểu diễn giải trí Dịch vụ khác Trang trí tiệc cưới Tổ chức khai trương Tư vấn dịch vụ Thư viện ảnh Tin tức - sự kiện Liên hệ Chú hề sinh nhật Trang trí YEAR END PARTY công ty Trang trí tất niên cuối năm Trang trí tất niên xu hướng mới nhất Trang trí sinh nhật bé trai Hải Đăng Trang trí sinh nhật bé Khánh Vân Trang trí sinh nhật Bích Ngân Trang trí sinh nhật bé Thanh Trang Thuê ông già Noel phát quà Biểu diễn xiếc khỉ Xiếc quay đĩa Dịch vụ tổ chức sự kiện 5 sao Thông tin về chúng tôi Dịch vụ sinh nhật bé trai Dịch vụ sinh nhật bé gái Sự kiện trọn gói Các tiết mục giải trí Dịch vụ bổ trợ Tiệc cưới sang trọng Dịch vụ khai trương Tư vấn tổ chức sự kiện Hình ảnh sự kiện Cập nhật tin tức Liên hệ ngay Thuê chú hề chuyên nghiệp Tiệc tất niên cho công ty Trang trí tiệc cuối năm Tiệc tất niên độc đáo Sinh nhật bé Hải Đăng Sinh nhật đáng yêu bé Khánh Vân Sinh nhật sang trọng Bích Ngân Tiệc sinh nhật bé Thanh Trang Dịch vụ ông già Noel Xiếc thú vui nhộn Biểu diễn xiếc quay đĩa Dịch vụ tổ chức tiệc uy tín Khám phá dịch vụ của chúng tôi Tiệc sinh nhật cho bé trai Trang trí tiệc cho bé gái Gói sự kiện chuyên nghiệp Chương trình giải trí hấp dẫn Dịch vụ hỗ trợ sự kiện Trang trí tiệc cưới đẹp Khởi đầu thành công với khai trương Chuyên gia tư vấn sự kiện Xem ảnh các sự kiện đẹp Tin mới về sự kiện Kết nối với đội ngũ chuyên gia Chú hề vui nhộn cho tiệc sinh nhật Ý tưởng tiệc cuối năm Tất niên độc đáo Trang trí tiệc hiện đại Tổ chức sinh nhật cho Hải Đăng Sinh nhật độc quyền Khánh Vân Phong cách tiệc Bích Ngân Trang trí tiệc bé Thanh Trang Thuê dịch vụ ông già Noel chuyên nghiệp Xem xiếc khỉ đặc sắc Xiếc quay đĩa thú vị
Trang chủ Giới thiệu Sinh nhật bé trai Sinh nhật bé gái Tổ chức sự kiện Biểu diễn giải trí Dịch vụ khác Trang trí tiệc cưới Tổ chức khai trương Tư vấn dịch vụ Thư viện ảnh Tin tức - sự kiện Liên hệ Chú hề sinh nhật Trang trí YEAR END PARTY công ty Trang trí tất niên cuối năm Trang trí tất niên xu hướng mới nhất Trang trí sinh nhật bé trai Hải Đăng Trang trí sinh nhật bé Khánh Vân Trang trí sinh nhật Bích Ngân Trang trí sinh nhật bé Thanh Trang Thuê ông già Noel phát quà Biểu diễn xiếc khỉ Xiếc quay đĩa

How to tackle a ‘branched’ arrow head anti-pattern? [duplicate]

I recently read this question that features, the arrow anti-pattern.

I have something similar in code I’m trying to refactor except that it branches. It looks a little something like this:

Plain text
Copy to clipboard
Open code in new window
EnlighterJS 3 Syntax Highlighter
<code>if(fooSuccess==true&&barSuccess!=true){
if(mooSuccess==true){
.....
}else if (mooSuccess!=true){
.....
}
}else if(fooSuccess!=true&&barSuccess==true){
if(mooSuccess==true){
.....
}else if (mooSuccess!=true){
if(cowSuccess==true){
.....
}else if (cowSuccess!=true){
.....
}
}
}
......
</code>
<code>if(fooSuccess==true&&barSuccess!=true){ if(mooSuccess==true){ ..... }else if (mooSuccess!=true){ ..... } }else if(fooSuccess!=true&&barSuccess==true){ if(mooSuccess==true){ ..... }else if (mooSuccess!=true){ if(cowSuccess==true){ ..... }else if (cowSuccess!=true){ ..... } } } ...... </code>
if(fooSuccess==true&&barSuccess!=true){
    if(mooSuccess==true){
           .....
    }else if (mooSuccess!=true){
           .....
    }
}else if(fooSuccess!=true&&barSuccess==true){
    if(mooSuccess==true){
           .....
    }else if (mooSuccess!=true){
        if(cowSuccess==true){
                    .....
        }else if (cowSuccess!=true){
                    .....
        }
    }
}
......

In short it looks like this

Plain text
Copy to clipboard
Open code in new window
EnlighterJS 3 Syntax Highlighter
<code>if
if
if
if
do something
endif
else if
if
if
do something
endif
endif
else if
if
if
do something
endif
endif
endif
</code>
<code>if if if if do something endif else if if if do something endif endif else if if if do something endif endif endif </code>
if
   if
     if
       if
         do something
       endif
    else if
     if
       if
         do something
        endif
    endif
    else if
     if
       if
         do something
        endif
    endif
 endif

Outline borrowed from Coding Horror article on the subject

And the code goes on through different permutations of true and false for various flags. These flags are set somewhere ‘up’ in the code elsewhere, either by user input or by the result of some method.

How can I make this kind of code more readable? My intention is that eventually I will have a Bean-type object that contains all the choices the previous programmer tried to capture with this branching anti-pattern. For instance, if in the outline of the code above we really do only have three, I have an enum set inside that bean:

Plain text
Copy to clipboard
Open code in new window
EnlighterJS 3 Syntax Highlighter
<code>enum ProgramRouteEnum{
OPTION_ONE,OPTION_TWO,OPTION_THREE;
boolean choice;
void setChoice(boolean myChoice){
choice = myChoice;
}
boolean getChoice(){
return choice;
}
}
</code>
<code>enum ProgramRouteEnum{ OPTION_ONE,OPTION_TWO,OPTION_THREE; boolean choice; void setChoice(boolean myChoice){ choice = myChoice; } boolean getChoice(){ return choice; } } </code>
enum ProgramRouteEnum{
    OPTION_ONE,OPTION_TWO,OPTION_THREE;

    boolean choice;
    void setChoice(boolean myChoice){
        choice = myChoice;
    }
    boolean getChoice(){
        return choice;
    }
}

Is this an acceptable cure? Or is there a better one?

2

The code can be simplified like this:

Plain text
Copy to clipboard
Open code in new window
EnlighterJS 3 Syntax Highlighter
<code>boolean condA = ( fooSuccess && !barSuccess && mooSuccess )
boolean condB = ( fooSuccess && !barSuccess && !mooSuccess )
boolean condC = (!fooSuccess && barSuccess && mooSuccess )
boolean condD = (!fooSuccess && barSuccess && !mooSuccess && cowSuccess)
boolean condE = (!fooSuccess && barSuccess && !mooSuccess && !cowSuccess)
if (condA) {
....
return;
}
if (condB) {
....
return;
}
... and so son
if (condE) {
....
return;
}
</code>
<code>boolean condA = ( fooSuccess && !barSuccess && mooSuccess ) boolean condB = ( fooSuccess && !barSuccess && !mooSuccess ) boolean condC = (!fooSuccess && barSuccess && mooSuccess ) boolean condD = (!fooSuccess && barSuccess && !mooSuccess && cowSuccess) boolean condE = (!fooSuccess && barSuccess && !mooSuccess && !cowSuccess) if (condA) { .... return; } if (condB) { .... return; } ... and so son if (condE) { .... return; } </code>
boolean condA = ( fooSuccess  && !barSuccess &&  mooSuccess )
boolean condB = ( fooSuccess  && !barSuccess && !mooSuccess )
boolean condC = (!fooSuccess  &&  barSuccess &&  mooSuccess )
boolean condD = (!fooSuccess  &&  barSuccess && !mooSuccess &&  cowSuccess)
boolean condE = (!fooSuccess  &&  barSuccess && !mooSuccess && !cowSuccess)

if (condA) {
    ....
    return;
}
if (condB) {
    ....
    return;
}

... and so son 

if (condE) {
    ....
    return;
}

In essence:

  • Evaluate complex conditions into a single boolean
  • Since conditions are exclusive (as it’s typically the case in nested arrow heads) you don’t have to nest the ifs, just add a return so that no other block is run.
  • This reduces the cyclomatic complexity enormously and makes the code trivial.
  • Having what exactly gets run in every case, you can even do a Karnaugh table to identify and remove redundant condition combinations, just as they do in logic circuits design. But you didn’t provide that in the example.
  • Be sure to extract all this logic into its own method so the return statements don’t interfere with blocks that have to be run anymay.
  • Descriptive names should be chosen for conA..condF.

9

user61852 has a pretty good answer solving the general problem of simplifying nested conditionals. However, I was intrigued with your proposed state machine-like solution, and wanted to illustrate how that can sometimes be preferable to a set of binary flags.

It all depends on the sequence of obtaining the flags’ values and how many combinations are valid. If you have n flags, you have 2n states. For 4 flags, that’s 16 states, and as you observed, only 3 of them may have any useful meaning. In situations like that, using a state variable instead can simplify things greatly.

Let’s say you have a lock that will open if 4 keys are pressed in the right order. If you press any key incorrectly, it immediately resets back to the start of the sequence. A very naïve way to implement a keypress handler using binary flags is:

Plain text
Copy to clipboard
Open code in new window
EnlighterJS 3 Syntax Highlighter
<code>void key_pressed(char key)
{
if (!key1correct)
{
if (key == pin[0])
{
key1correct = true;
}
}
else if (!key2correct)
{
if (key == pin[1])
{
key2correct = true;
}
else
{
key1correct = false;
}
}
else if (!key3correct)
{
if (key == pin[2])
{
key3correct = true;
}
else
{
key1correct = false;
key2correct = false;
}
}
else
{
if (key == pin[3])
{
key4correct = true;
}
else
{
key1correct = false;
key2correct = false;
key3correct = false;
}
}
if (key1correct && key2correct && key3correct && key4correct)
{
open_lock();
key1correct = false;
key2correct = false;
key3correct = false;
key4correct = false;
}
}
</code>
<code>void key_pressed(char key) { if (!key1correct) { if (key == pin[0]) { key1correct = true; } } else if (!key2correct) { if (key == pin[1]) { key2correct = true; } else { key1correct = false; } } else if (!key3correct) { if (key == pin[2]) { key3correct = true; } else { key1correct = false; key2correct = false; } } else { if (key == pin[3]) { key4correct = true; } else { key1correct = false; key2correct = false; key3correct = false; } } if (key1correct && key2correct && key3correct && key4correct) { open_lock(); key1correct = false; key2correct = false; key3correct = false; key4correct = false; } } </code>
void key_pressed(char key)
{
   if (!key1correct)
   {
      if (key == pin[0])
      {
         key1correct = true;
      }
   }
   else if (!key2correct)
   {
       if (key == pin[1])
       {
           key2correct = true;
       }
       else
       {
           key1correct = false;
       }
   }
   else if (!key3correct)
   {
       if (key == pin[2])
       {
           key3correct = true;
       }
       else
       {
           key1correct = false;
           key2correct = false;
       }
   }
   else
   {
       if (key == pin[3])
       {
           key4correct = true;
       }
       else
       {
           key1correct = false;
           key2correct = false;
           key3correct = false;
       }
   }

   if (key1correct && key2correct && key3correct && key4correct)
   {
       open_lock();
       key1correct = false;
       key2correct = false;
       key3correct = false;
       key4correct = false;
   }
}

A simplified, flattened version using binary flags (like user61852’s answer) looks like this:

Plain text
Copy to clipboard
Open code in new window
EnlighterJS 3 Syntax Highlighter
<code>void key_pressed(char key)
{
if (!key1correct && key == pin[0])
{
key1correct = true;
return;
}
if (key1correct && !key2correct && key == pin[1])
{
key2correct = true;
return;
}
if (key1correct && key2correct && !key3correct && key == pin[2])
{
key3correct = true;
return;
}
if (key1correct && key2correct && key3correct && key == pin[3])
{
open_lock();
key1correct = false;
key2correct = false;
key3correct = false;
return;
}
key1correct = false;
key2correct = false;
key3correct = false;
}
</code>
<code>void key_pressed(char key) { if (!key1correct && key == pin[0]) { key1correct = true; return; } if (key1correct && !key2correct && key == pin[1]) { key2correct = true; return; } if (key1correct && key2correct && !key3correct && key == pin[2]) { key3correct = true; return; } if (key1correct && key2correct && key3correct && key == pin[3]) { open_lock(); key1correct = false; key2correct = false; key3correct = false; return; } key1correct = false; key2correct = false; key3correct = false; } </code>
void key_pressed(char key)
{
    if (!key1correct && key == pin[0])
    {
        key1correct = true;
        return;
    }

    if (key1correct && !key2correct && key == pin[1])
    {
        key2correct = true;
        return;
    }

    if (key1correct && key2correct && !key3correct && key == pin[2])
    {
        key3correct = true;
        return;
    }

    if (key1correct && key2correct && key3correct && key == pin[3])
    {
        open_lock();
        key1correct = false;
        key2correct = false;
        key3correct = false;
        return;
    }

    key1correct = false;
    key2correct = false;
    key3correct = false;
}

That’s a lot easier to read, but still in both of these solutions you have states like key2correct && !key1correct that are completely invalid and unused. Whereas using a state variable num_correct_keys instead of the binary flags looks like this:

Plain text
Copy to clipboard
Open code in new window
EnlighterJS 3 Syntax Highlighter
<code>void key_pressed(char key)
{
if (key == pin[num_correct_keys])
++num_correct_keys;
else
num_correct_keys = 0;
if (num_correct_keys == 4)
{
open_lock();
num_correct_keys = 0;
}
}
</code>
<code>void key_pressed(char key) { if (key == pin[num_correct_keys]) ++num_correct_keys; else num_correct_keys = 0; if (num_correct_keys == 4) { open_lock(); num_correct_keys = 0; } } </code>
void key_pressed(char key)
{
    if (key == pin[num_correct_keys])
        ++num_correct_keys;
    else
        num_correct_keys = 0;

    if (num_correct_keys == 4)
    {
        open_lock();
        num_correct_keys = 0;
    }
}

Granted, this is a contrived example, but people often write code like the first version in less obvious situations, sometimes spread across multiple files. Using a state variable often greatly simplifies code, especially when the flags represent a sequence of events.

I’m not sure your example is a particularly good one for this question, but it’s possible to condense it to be much simpler and still preserve the same logic:

Plain text
Copy to clipboard
Open code in new window
EnlighterJS 3 Syntax Highlighter
<code>if(fooSuccess && !barSuccess)
{
if(mooSuccess)
{
// .....
}
else
{
// .....
}
}
else if (!fooSuccess && barSuccess)
{
if(mooSuccess)
{
// .....
}
else if(cowSuccess)
{
// .....
}
else
{
// .....
}
}
</code>
<code>if(fooSuccess && !barSuccess) { if(mooSuccess) { // ..... } else { // ..... } } else if (!fooSuccess && barSuccess) { if(mooSuccess) { // ..... } else if(cowSuccess) { // ..... } else { // ..... } } </code>
if(fooSuccess && !barSuccess)
{
    if(mooSuccess)
    {
        // .....
    }
    else
    {
        // .....
    }
}
else if (!fooSuccess && barSuccess)
{
    if(mooSuccess)
    {
        // .....
    }
    else if(cowSuccess)
    {
        // .....
    }
    else 
    {
        // .....
    }
}

Notice how there’s no more than one level deep of ifs. To clean this up, I took the following steps:

  • Never check whether a boolean value is == true, != true, or != false. Occasionally I’ve found it worth checking whether it == false to make it more apparent that that’s the expected case, but even that should be exceptional.
  • If your else block consists of nothing but an if/else block, then you can merge it with the parent else.
  • If your if statement only has one input/variable, you don’t need to check the inverse for the else.

Trang chủ Giới thiệu Sinh nhật bé trai Sinh nhật bé gái Tổ chức sự kiện Biểu diễn giải trí Dịch vụ khác Trang trí tiệc cưới Tổ chức khai trương Tư vấn dịch vụ Thư viện ảnh Tin tức - sự kiện Liên hệ Chú hề sinh nhật Trang trí YEAR END PARTY công ty Trang trí tất niên cuối năm Trang trí tất niên xu hướng mới nhất Trang trí sinh nhật bé trai Hải Đăng Trang trí sinh nhật bé Khánh Vân Trang trí sinh nhật Bích Ngân Trang trí sinh nhật bé Thanh Trang Thuê ông già Noel phát quà Biểu diễn xiếc khỉ Xiếc quay đĩa Dịch vụ tổ chức sự kiện 5 sao Thông tin về chúng tôi Dịch vụ sinh nhật bé trai Dịch vụ sinh nhật bé gái Sự kiện trọn gói Các tiết mục giải trí Dịch vụ bổ trợ Tiệc cưới sang trọng Dịch vụ khai trương Tư vấn tổ chức sự kiện Hình ảnh sự kiện Cập nhật tin tức Liên hệ ngay Thuê chú hề chuyên nghiệp Tiệc tất niên cho công ty Trang trí tiệc cuối năm Tiệc tất niên độc đáo Sinh nhật bé Hải Đăng Sinh nhật đáng yêu bé Khánh Vân Sinh nhật sang trọng Bích Ngân Tiệc sinh nhật bé Thanh Trang Dịch vụ ông già Noel Xiếc thú vui nhộn Biểu diễn xiếc quay đĩa Dịch vụ tổ chức tiệc uy tín Khám phá dịch vụ của chúng tôi Tiệc sinh nhật cho bé trai Trang trí tiệc cho bé gái Gói sự kiện chuyên nghiệp Chương trình giải trí hấp dẫn Dịch vụ hỗ trợ sự kiện Trang trí tiệc cưới đẹp Khởi đầu thành công với khai trương Chuyên gia tư vấn sự kiện Xem ảnh các sự kiện đẹp Tin mới về sự kiện Kết nối với đội ngũ chuyên gia Chú hề vui nhộn cho tiệc sinh nhật Ý tưởng tiệc cuối năm Tất niên độc đáo Trang trí tiệc hiện đại Tổ chức sinh nhật cho Hải Đăng Sinh nhật độc quyền Khánh Vân Phong cách tiệc Bích Ngân Trang trí tiệc bé Thanh Trang Thuê dịch vụ ông già Noel chuyên nghiệp Xem xiếc khỉ đặc sắc Xiếc quay đĩa thú vị
Trang chủ Giới thiệu Sinh nhật bé trai Sinh nhật bé gái Tổ chức sự kiện Biểu diễn giải trí Dịch vụ khác Trang trí tiệc cưới Tổ chức khai trương Tư vấn dịch vụ Thư viện ảnh Tin tức - sự kiện Liên hệ Chú hề sinh nhật Trang trí YEAR END PARTY công ty Trang trí tất niên cuối năm Trang trí tất niên xu hướng mới nhất Trang trí sinh nhật bé trai Hải Đăng Trang trí sinh nhật bé Khánh Vân Trang trí sinh nhật Bích Ngân Trang trí sinh nhật bé Thanh Trang Thuê ông già Noel phát quà Biểu diễn xiếc khỉ Xiếc quay đĩa
Thiết kế website Thiết kế website Thiết kế website Cách kháng tài khoản quảng cáo Mua bán Fanpage Facebook Dịch vụ SEO Tổ chức sinh nhật