What can I do to improve this program?

[Erebusvega]

I am having trouble with this question. I've only been coding for about 3 months so I'm very new at this. What I need help with is to make the program say "My job here is done." or something similar after each step only if the statement is true. For example:

#include <iostream>
using namespace std;
int main() {
  string fix;
  cout << "Reboot the computer and try to connect.\n";
  cout << "Did that fix the problem?\n";
  cin >> fix;
  if (fix == "No" || fix == "no")
    {
    cout << "Reboot the router and try again.\n";
    cout << "Did that fix the problem?\n";
    cin >> fix;

      if (fix == "No" || fix == "no")
      {
        cout << "Make sure the cables between the router and modem are plugged in firmly.\n";
        cout << "Did that fix the problem?\n";
        cin >> fix;

          if (fix == "No" || fix == "no")
          {
            cout << "Move the router to a new location and try to connect.\n";
            cout << "Did that fix the problem?\n";
            cin >> fix;

              if (fix == "No" || fix == "no")
              {
                cout << "Get a new router.\n";
              }
          }
      }
  }
else
{
    if (fix == "Yes" || fix == "yes")
    {
        cout << "My job here is done.";
    }
}
}

Let's say I move the router to another location and it fixes my problem. Whenever I type in "Yes" it should display "My job here is done." My problem is that it doesn't display that at all. Only for the first problem. What can I do to fix this? Thank you in advance!!

[jonnin]

}
}
}
}
else
{
if (fix == "Yes" || fix == "yes")
{
cout << "My job here is done.";
}
}
}

^^^ please put code tags <> on the side bar editor. The above sample is kinda nuts to unravel, and the problem is almost surely due to brace placement.

[Erebusvega]

Whoops, my bad I wasn't aware of that. Hopefully I fixed it. So the problem with my program is the placement of the braces with my else statement?

[JLBorges]

> I've only been coding for about 3 months so

Surely you have written functions, haven't you?


> So the problem with my program is the placement of the braces with my else statement?

20. Avoid long functions. Avoid deep nesting.

Summary

Short is better than long, flat is better than deep: Excessively long functions and
nested code blocks are often caused by failing to give one function one cohesive re-
sponsibility (see Item 5), and both are usually solved by better refactoring.

Discussion

... Excessive straight-line function length and excessive block nesting depth (e.g., if,
for, while, and try blocks) are twin culprits that make functions more difficult to un-
derstand and maintain, and often needlessly so.

Each level of nesting adds intellectual overhead when reading code because you
need to maintain a mental stack (e.g., enter conditional, enter loop, enter try, enter
conditional, ...). Have you ever found a closing brace in someone's code and won-
dered which of the many fors, whiles, or ifs it matched? Prefer better functional de-
composition to help avoid forcing readers to keep as much context in mind at a time.

Exercise common sense and reasonableness: Limit the length and depth of your
functions. All of the following good advice also helps reduce length and nesting:
• Prefer cohesion: Give one function one responsibility (see Item 5).
• Don't repeat yourself: Prefer a named function over repeated similar code snippets.
• ...
Alexandrescu and Sutter in 'C++ Coding Standards: 101 Rules, Guidelines, and Best Practices'

#include <iostream>
#include <string>
#include <vector>

bool fixed_with( const std::string& try_this )
{
    std::cout << '\n' << try_this << '\n' ;
    std::cout << "did that fix the problem (y/n)? " ;
    char ans ;
    std::cin >> ans ;
    return ans == 'y' || ans == 'Y' ;
}

bool try_fixes( const std::vector<std::string>& fix_list )
{
    // for each possible fix, in order
    for( const std::string& fix : fix_list )
    {
        if( fixed_with(fix) )
        {
            std::cout << "my job is done!\n" ;
            return true ;
        }
    }

    std::cout << "i give up!\n" ;
    return false ;
}

int main()
{
    const std::vector<std::string> fix_list =
    {
        "Reboot the computer and try to connect.",
        "Make sure the cables between the router and modem are plugged in firmly.",
        "Move the router to a new location and try to connect.",
        "Get a new router."
    };

    try_fixes(fix_list) ;
}

[Erebusvega]

Ah I see what you mean. Thanks for the info!

[jonnin]

and the original issue, even if you no longer want to do it this way:
excess braces. Now that its formatted, we can look at it easier...

what you have :
if(whatever)  //the else is tied ONLY TO THIS ONE
  {
     if(other) //this is inside the other if, and NOT TIED TO THE ELSE
       {
           ...
        }
   }
else
{

}

the only way that else can trigger is if whatever is false.
the logic you wanted was to chain them like this:

if(whatever
{
}
else if (other)
{
}
else

and as a side note the final else has a condition, making it possible for NO response at all. There should probably be a 'default' response to the user here, as a friendly gesture ... a final "input was invalid" type print out.

also the yes/no thing is annoying. you have 3 solid choices in a console program here..
1) take yes and no, upper case them, and check vs "YES" and "NO" (this accepts yes, Yes, YES, yEs, ... etc all possible combos covered)
2) use y and n as above, possibly with a toupper just in case (pun!)
3) use a menu / numeric response (1 for yes, 2 for no) setup

what you have works too, its just a bit picky at the user and a bit wordy in the code.
**by habit I do upper. you can use the same idea on lower, of course.