Sign in to follow this  
NUCLEAR RABBIT

Cleaning Things up (C++)

Recommended Posts

NUCLEAR RABBIT    318
Hello, I've been reading through some posts, and have seen some people talking about how their code should be clean. I wanted to know if someone could tell me if my code is clean, and if not, give me some tips on how to make your code, cleaner? Thanks For any Help. Example Code #1:
/**$*%$*%$*%$*%$*%$*%$*%$*%$*%$*%$*%$*%$*$
$*%    PROGRAMMER NAME: BRANDON WALL   *$%
%*$    DATE:  4-21-06                  %*$
*%$*%$*%$*%$*%$*%$*%$*%$*%$*%$*%$*%$*%$**/ 

// Chapter 2 | Exercise 1 (Using Switch)

#include <string>
#include <iostream>
#include <ctime>
#include <windows.h>
#include <cstdlib>
using namespace std;

int main()
{
    
    //Sets Difficulty Levels
    enum diffs {EASY, NORMAL, HARD};
    diffs myDiff;
    
    //Gets Users Choice
    int diff;
    cout << "*******************************\n"
          << "**                           **\n"
          << "**     Difficulty Levels     **\n"
          << "**                           **\n"
          << "*******************************\n\n"
          << "1) Easy\n"
          << "2) Normal\n"
          << "3) Hard\n\n"
          << "Your Selection ";
    cin >> diff;
    
    //Switch Loop
    switch (diff)
    {
           case 1:
                       //Sets NEW Difficulty
                       myDiff = EASY;
                       
                       //Tells user Selection
                       cout << "\n\nDifficulty Set to Easy.";
                       
                       //Delay for 2 Seconds
                       Sleep(2000);
                       
                       //Clear Screen For ending
                       system("CLS");
                       break;
                       
           case 2:
                       //Sets NEW Sifficulty
                       myDiff = NORMAL;
                       
                       //Tells user
                       cout << "\n\nDifficulty Set to Normal.";
                       
                       //Delay for 2 Seconds
                       Sleep(2000);
                       
                       //Ends
                       system("CLS");
                       break;
                       
           case 3:
                       //Sets NEW Difficulty
                       myDiff = HARD;
                       
                       //Tells user
                       cout << "\n\nDifficulty Set to Hard.\n\n";
                       
                       //Delays for 2 seconds
                       Sleep(2000);
                       
                       //Ends
                       system("CLS");
                       break;
                       
           default:
                       
                       //Tells user that isnt acceptable
                       cout << "\n\nSorry, That isn't an Option";
                       
                       //Delays for 2 Seconds
                       Sleep(2000);
                       
                       //Clears screen
                       system("CLS");
                       
                       //Takes user back to the menu to enter a Valid Option
                       main();
                       break;
                       
    }
    
    return 0;
    
}

Example Code# 2:
/**$*%$*%$*%$*%$*%$*%$*%$*%$*%$*%$*%$*%$*$
$*%    PROGRAMMER NAME: BRANDON WALL   *$%
%*$    DATE:   4-19-06                 %*$
*%$*%$*%$*%$*%$*%$*%$*%$*%$*%$*%$*%$*%$**/ 

// Shimmer Inc. DataBase

#include <string>
#include <iostream>
#include <windows.h>
using namespace std;


void menu( int& output )
{
        cout << "\t\t\t+ Welcome to Shimmers Inc. +\n\n"
         << "***************************************************\n"
         << "**                                               **\n"
         << "**   Navigation:                                 **\n"
         << "**   1) Create New Shimmer Account               **\n"
         << "**   2) Log In With Existing Shimmer Account     **\n"
         << "**   3) Exit                                     **\n"
         << "**                                               **\n"
         << "***************************************************\n\n"
         << "Please Make Your Selection: ";
    cin >> output;
}

void poop()
{
    int menuSelection;
        //Declaire Variables
    string userName;
    string password;
    string name;
    string email;
    string logName;
    string logPass;
    
    //Declaires the menu function
    menu(menuSelection);
    
    if(menuSelection == 1)
    {
                    //Clears screen for users info
                    system("cls");
                    
                    //username
                    //Gets users new account info
                    cout << "*Setp 1*\n\n";
                    cout << "1) Please enter Your New Username (One Word): ";
                    cin >> userName;
                    
                    
                    //password
                    cout << "2) Please enter Your Account Pssword: ";
                    cin >> password;
                    
                    
                    //name
                    cout << "\n\n*Step 2*\n\n"
                         << "1) First Name: ";
                    cin >> name;
                    
                    //email
                    cout << "2) Email Address: ";
                    cin >> email;
                    
                    //Thank you ending
                    cout << "\nThank You.";
                    Sleep(2000);
                    
                    //clear screen for loading message
                    system("cls");
                    
                    //Loading message
                    cout << "\t\t\t\tL";
                         Sleep(1000);
                    cout << "O";
                         Sleep(1000);
                    cout << "A";
                         Sleep(1000);
                    cout << "D";
                         Sleep(1000);
                    cout << "I";
                         Sleep(1000);
                    cout << "N";
                         Sleep(1000);
                    cout << "G\n\n";
                         Sleep(2000);
                    //Waits for user to cont.
                    system("cls");
                    
                    //Gives final Summary
                    cout << "Thank Your for Choosing Shimmer!\n"
                         << "Heres Your Account Info: \n\n"
                         << "Account Name: "
                         << userName
                         << "\nPassword: "
                         << password
                         << "\n\nPlease Write your Account Information Down Incase you Forget.\n"
                         << "Please Log In when you are Transfered to the Menu Screen.\n\n\n";
                    system("pause");
                    system("cls");
                    menu(menuSelection);
    }
    
    
    if(menuSelection == 2)
    {
                     //Tells user to enter their account name
                     system("cls");
                     
                     cout << "********************************\n"
                          << "**                            **\n"
                          << "**   Shimmer Sccount Log In   **\n"
                          << "**                            **\n"
                          << "********************************\n\n"
                          << "Account Name: ";
                     cin >> logName;
                     
                     //Tells user to enter their account password
                     cout << "Password: ";
                     cin >> logPass;
                     
                     if(logName != userName && logPass != password)
                     {
                                cout << "\n\nAccess Denied.\n\n";
                                //Delays for another try
                                Sleep(2000);
                                system("cls");
                                poop();
                     }
                     if(logName == userName && logPass == password)
                     {
                          cout << "\nWelcome, "
                               << name
                               << "\n\n";
                          
                          //waits for user to exit
                          system("pause");
                     }
    }
    
    
    else if(menuSelection == 3)
    {
                     cout << "\n\nOk, Bye.\n\n";
                     Sleep(1000);
    }
    
    else
    {
         cout << "\n\nThats not an Option. Bye\n\n";
         Sleep(1000); 
         
    }
                    
}

int main()
{
    poop();
    return 0;   
}

Share this post


Link to post
Share on other sites
dave    2187
Hey bud,

Looks nice, one suggestion though.

No matter how convenient it is, do not write:

using namespace std;

Because:

What is a namespace for?

- To help clarify naming lookup and your code by reducing the possibility of two functions in two libraries with the same declaration causing ambiguity between them for the compiler.

There is no point in bringing everything from the std namespace in to the local namespace because it defies the point of having the namespace as described above. In case you havn't seen the correct way of doing it, it is like this:

using std::vector;
using std::map;

etc..


Keep up the good work,

Dave

Share this post


Link to post
Share on other sites
Anon Mike    1098
"poop" is pretty much never a good name for a function. Maybe if you were writing some sort of biological simulator...

You over comment a bit. Generally you use comment to explain things that may be difficult to understand or that need special attention for some reason. It's pretty rare that you need to comment every single line. It also helps to move things into function with meaningful names, e.g. you can call ClearScreen() instead of system("CLS") and needing a comment for it. The line between to much commenting and not enough is pretty fine though so be careful.

Your functions are a bit long and try to do to much. You could break the cases up into seperate functions with meaningful names and it would probably easier to understand.

Share this post


Link to post
Share on other sites
bradbobak    1825
in default: you call 'main()'.. you shouldn't be doing this!! rather put a loop around your function and simply 'break' out when a valid option is entered..
what you are doing is calling 'main()' over and over when a bad selection is made rather than just looping until a good selection is entered.

Share this post


Link to post
Share on other sites
NotAYakk    876
Yeuch.

1> Huge long functions are evil.
Write short functions with as few parameters are reasonable.

2> Variable scope should be minimized. Variables should be declaired as late as reasonable, and go out of scope as soon as reasonable.

3> Blocks of variables should be struct-ized.
See:

string userName;
string password;
string name;
string email;
string logName;
string logPass;

at the very least, stuff that into a struct or two.

4> Branches should be short. If lots of things are done in a branch, have the branch call a function (or a set of functions) with descriptives names that does the work.

5> Your code is needlessly recursive.

Try something more like:

struct LoginData { ... };
struct PlayerID { ... };

...
PlayerID id;
bool logged_in = false;
while(!logged_in) {
LoginData login_data = prompt_login();
id = try_login(login_data);
logged_in = IsValidID(id);
}


The goal here is to be able to read the entire function and know what it does at a glance, even if the implementation details are hidden in helper functions.

Next, your code has lots of "magic numbers" -- like the 2000 in "sleep 2000". Toss those magic numbers into constants, at the very least. And maybe even call "WaitForTheUserToRead()" instead of "Sleep(2000)" directly (this lets you easily change your code's behaviour, and is self-documenting why you are doing something, as opposed to what you are doing).

Why is more important than What.

While your code isn't spagetti code, I wouldn't call your code clean.

My apologies if I came accross too bombastic and/or harsh. I am too bombastic and harsh, but I apologize for not hiding it better. ;)

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this