7
\$\begingroup\$

I've recently stared learning C++ so I'm new to all of it. I've got my fourth "software" under development. Could someone have a look through and give me some constructive criticism on what should I change or keep the same?

#include <iostream>
#include <string>
#include <stdlib.h>
#include <fstream>

using namespace std;


void mainmenu();


int choice; 
bool cinfail; 
int confirmation; 
string username, password, password2;

void writetofile(string username){
    ofstream writefile;
    string file = username+".txt";
    writefile.open(file.c_str());
    writefile << password;
    writefile.close();
    mainmenu(); }

void login(){
    cout << "You are being logged in!";}


void registerpassword(){
    cout << "Please enter the password:" << endl;
    cin >> password;
    cout << "Please renter your password:" << endl;
    cin >> password2;
    if (password == password2){
        cin.clear();
        cin.ignore(10000,'\n');
        writetofile(username);
        exit(1);
    }
    else;{
        cout << "Sorry invalid" << endl;
        registerpassword();
    }}


void registerme(){
    cout << "Please enter your username: " << endl;
    getline(cin, username);
    cout << "\nUsername -  \""<< username << "\"\nConfirm? \n\n[1] Yes\n[2] No" << endl;
    cin >> confirmation;
    if (confirmation == 1){
        registerpassword();
    }

    else; {
        cout << "Sorry invalid input, Please try again" << endl;
        cin.clear();
        cin.ignore(10000,'\n');
        registerme();
    }}


void exit(){
    exit(0);}

void mainmenu(){ cout << "Hello, Would you like to log in or register\n[1] Login\n[2] Register\n[3] Exit" <<endl; cin >> choice; do{
    cinfail = cin.fail();
    cin.clear();
    cin.ignore(10000,'\n');

    }while(cinfail == true);{
        switch(choice){
            case 1:
                login();
                break;

            case 2:
                registerme();
                break;

            case 3:
                exit();}}} 

main(){ 
mainmenu(); 
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Welcome to Code Review! Here, the title of a question should be about what the code does. Next, the introductory paragraph should give a bit more detail about what the code does, and the kind of review you're looking for. Please read the how-to-ask page in the help center for more details \$\endgroup\$ – Stop ongoing harm to Monica Mar 29 '16 at 17:55
9
\$\begingroup\$
  1. Use C++ headers instead of C headers. Change #include <stdlib.h> to #include <cstdlib>. You could actually remove this header altogether because you really should not be calling exit() in this program.

  2. Avoid using namespace std;. You are polluting the global namespace and increasing the probability of naming clashes. If you put using namespace std; into a header, you pollute the global namespace of every user that uses your header. Putting it into a CPP file is not as bad as putting it into a header, but the same dangers exist. See this link for more information.

  3. Avoid using global variables when they are not necessary. You want to limit the scope of your variables as much as possible. This program can be rewritten to use no global variable at all.

  4. Use appropriate function names. mainmenu() should be either main_menu() or MainMenu(), writetofile() should be either write_to_file() or WriteToFile(), etc.

  5. Get rid of the unnecessary recursion. Recursion is very nice an clean in some situations. Unfortunately, it's very ugly in this situation. This program can be rewritten to use no recursion at all.

  6. Learn about RAII. It is one of the most important aspects of the C++ language. In your writetofile function, you manually call writefile.close();. You do not have to do this. The std::ofstream will automatically clean up its own resources once it goes out of scope. This is one of the reasons why limiting the scope of your variables is important.

  7. Learn about std::numeric_limits. While cin.ignore(10000,'\n'); should work in most cases, std::cin.ignore(std::numeric_limits <std::streamsize>::max(), '\n'); will work in more cases.

Now with that out of the way, let's see how we can rewrite this program.

Headers

#include <fstream>
#include <iostream>
#include <limits> // for std::numeric_limits
#include <string>

Create a function to get user input

We want to get rid of the ugly recursion that we have going on. One thing we can do is create a helper function that can read user input for various values. This is a function template. It can return ints, std::strings, and other variable types. If bad input is given to this function, then it asks the user to try again.

template <typename T>
T get_input(const std::string &strQuery)
{
    std::cout << strQuery << "\n> ";
    T out = T();

    while (!(std::cin >> out)) {
        std::cin.clear();
        std::cin.ignore(std::numeric_limits <std::streamsize>::max(), '\n');
        std::cout << "Error!" "\n";
        std::cout << strQuery << "\n> ";
    }

    return out;
}

Rewrite the mainmenu() function

First we will rename mainmenu() to main_menu(). Second we will use our new get_input() function. Notice that in C++, two string literals next to each other (such as "[1] Login" "\n") are automatically concatenated into one string literal by the compiler. Even if the string literals are on separate lines, as long as there are no semicolons or anything else between them, then the compiler will concatenate them. Using this strategy make your main_menu() class easier to read. Notice that we only need two switch cases. All other cases result in nothing being done.

void main_menu()
{
    int choice = get_input <int>(
        "Hello, Would you like to log in or register?" "\n" 
        "[1] Login" "\n" 
        "[2] Register" "\n" 
        "[3] Exit");

    switch (choice)
    {
    case 1:
        login();
        break;
    case 2:
        register_user();
        break;
    }
}

Rewrite the login() function

This is a very small change. I add a newline to it.

void login()
{
    std::cout << "You are being logged in!" "\n";
}

Rewrite the registerme() function: Part 1

First, let's give it a better name. Let's rename this function to register_user(). This function does 3 things. It:

  1. Gets a username.
  2. Gets a password.
  3. Saves the password to a file.

To make this function more readable, we will create 3 helper functions to perform each of these tasks.

Get the username

We will create a helper function that will get the username. We will use get_input() to avoid all of the ugly recursion that you have.

std::string get_username()
{
    std::string username = get_input <std::string>("Please enter a username.");
    std::cout << "Username: \"" << username << "\"\n";

    while (get_input <int>("Confirm? [0|1]") != 1) {
        username = get_input <std::string>("Please enter a username.");
        std::cout << "Username: \"" << username << "\"\n";
    }

    return username;
}

Get the password

We just have to make sure the passwords match. Once again, the get_input() function makes this clean.

std::string get_password()
{
    std::string password1 = get_input <std::string> ("Please enter your password.");
    std::string password2 = get_input <std::string> ("Please re-enter your password.");

    while (password1 != password2) {
        std::cout << "Error! Passwords do not match." "\n";
        password1 = get_input <std::string>("Please enter your password.");
        password2 = get_input <std::string>("Please re-enter your password.");
    }

    return password1;
}

Save the username and password

Now we need a function that can save the username and password. This one is pretty similar to your writetofile() function, except it is easier to read. It does require passing both the username and the password though.

void save_user(const std::string &username, const std::string &password)
{
    std::string filename = username + ".txt";
    std::ofstream file(filename);
    file << password << "\n";
}

Rewrite the registerme() function: Part 2

Now with those 3 helper functions implemented, we can write the new registerme() function (which we renamed register_user()). This turns your complicated function into a simple function that is easy to read.

void register_user()
{
    std::string username = get_username();
    std::string password = get_password();
    save_user(username, password);
}

Rewrite the main() function

Your main() function stays pretty much the same. Just call main_menu() instead of mainmenu() since we renamed the function.

All of it together

#include <fstream>
#include <iostream>
#include <limits>
#include <string>

template <typename T>
T get_input(const std::string &strQuery);

std::string get_username();
std::string get_password();
void save_user(const std::string &username, const std::string &password);

void login();
void register_user();
void main_menu();

int main()
{
    main_menu();
}

template <typename T>
T get_input(const std::string &strQuery)
{
    std::cout << strQuery << "\n> ";
    T out = T();

    while (!(std::cin >> out)) {
        std::cin.clear();
        std::cin.ignore(std::numeric_limits <std::streamsize>::max(), '\n');
        std::cout << "Error!" "\n";
        std::cout << strQuery << "\n> ";
    }

    return out;
}

std::string get_password()
{
    std::string password1 = get_input <std::string> ("Please enter your password.");
    std::string password2 = get_input <std::string> ("Please re-enter your password.");

    while (password1 != password2) {
        std::cout << "Error! Passwords do not match." "\n";
        password1 = get_input <std::string>("Please enter your password.");
        password2 = get_input <std::string>("Please re-enter your password.");
    }

    return password1;
}

std::string get_username()
{
    std::string username = get_input <std::string>("Please enter a username.");
    std::cout << "Username: \"" << username << "\"\n";

    while (get_input <int>("Confirm? [0|1]") != 1) {
        username = get_input <std::string>("Please enter a username.");
        std::cout << "Username: \"" << username << "\"\n";
    }

    return username;
}

void login()
{
    std::cout << "You are being logged in!" "\n";
}

void main_menu()
{
    int choice = get_input <int>(
        "Hello, Would you like to log in or register?" "\n"
        "[1] Login" "\n"
        "[2] Register" "\n"
        "[3] Exit");

    switch (choice)
    {
    case 1:
        login();
        break;
    case 2:
        register_user();
        break;
    }
}

void register_user()
{
    std::string username = get_username();
    std::string password = get_password();
    save_user(username, password);
}

void save_user(const std::string &username, const std::string &password)
{
    std::string filename = username + ".txt";
    std::ofstream file(filename);
    file << password << "\n";
}
\$\endgroup\$
  • \$\begingroup\$ Great code review! Just one question, why do you use a 'confirmation' variable in your get_username() function? The loop can simply be writen as 'while (get_input<int>("Confirm? [0|1]") != 1) {...}', making the variable unnecessary^^ \$\endgroup\$ – Kodnot Mar 30 '16 at 11:11
  • \$\begingroup\$ @Kodnot Good catch, thanks! I updated the answer. \$\endgroup\$ – ncalmbeblpaicr0011 Mar 30 '16 at 17:03
4
\$\begingroup\$

First of all, many will argue that you should not use using namespace std. It may cause problems if you write your own function that shares a name with one of the STL functions.

You use the exit() function to end your program. It would be better to have your program return normally than to force the exit. If you were doing something more involved that required memory allocation/deletion, you could inadvertently end your program without freeing memory, causing leaks.

I'd recommend taking a look at the Google style docs for C++. They will help the overall look of your code. The things I would predominantly point out are your spacing/indentation and braces. Generally, you should keep braces on new lines by themselves to improve readability, or at the least put a space between the brace and the other content on the same line. Every time you make a new code block via braces, you should indent the content of that block one tab more than the surrounding content. For example:

int main()
{
    Foo();
    if (TheSkyIsBlue)
    {
        Bar();
    }
}

It seems that you do this more or less consistently, but try to make it the same throughout. Furthermore, it is good practice to limit the length of your lines. Traditionally, the limit was 80 characters, but pick something that enables most viewers to see your code without horizontal scrolling.

You also have semicolons after several else keywords, while(conditional) statements, etc. in your code, which ought to make it unable to be compiled.

\$\endgroup\$
  • \$\begingroup\$ Google's C++ Guide may not be the best thing to learn from for modern C++. \$\endgroup\$ – ncalmbeblpaicr0011 Mar 29 '16 at 20:33
  • \$\begingroup\$ @ncalmbeblpaicr0011 Any specific things you dislike about it, or do you consider most if not all of it to be obsolete/poor practice? For my reference, do you have an alternative comprehensive style guide you prefer? \$\endgroup\$ – Michael Brandon Morris Mar 30 '16 at 3:42
  • \$\begingroup\$ I disagree with its stance on exceptions, trailing return type syntax, function overloading (it says use AppendString() and AppendInt() rather than Append()), and other small things that can make a big difference. It seems like I am not the only one who disagrees with the style guide. Some of its suggestions are good, others are tailored purely for Google's code base. I think it's an okay guide to follow as long as you don't see it as the word of God. \$\endgroup\$ – ncalmbeblpaicr0011 Mar 30 '16 at 17:46
  • 1
    \$\begingroup\$ I don't follow any particular style guide, but here are recommendations on other SE sites: 1, 2. \$\endgroup\$ – ncalmbeblpaicr0011 Mar 30 '16 at 17:48

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.