6
\$\begingroup\$

I'm currently working on my final project of my first semester of C++ (and programming in general). What my professor wants us to do is to make a mock social media program that has some basic functions such as:

  • Sign up
  • Sign in
  • Follow user
  • Write a post
  • View Activity – (Activity from friends)
  • View Profile – (Username, followers, people you follow, posts from you)
  • Sign Out
  • Exit

On the prompt it says that he:

"expects to see knowledge of the material, in particular Object Oriented Design - Classes".

I assume that means that we should be using classes properly.

That brings me to my general question about the code I have, which is whether or not I'm fulfilling that "requirement" of demonstrating knowledge of using classes (or objects or whatever he's probably expecting).

project02.cpp

#include <iostream>
#include <fstream>
#include <string>
#include <cstdlib>
#include "project02.h"

using namespace std;

ofstream fout;
ifstream fin;

void LoginScreen()
{
    cout << "Please select from the following options by entering the corresponding number:" << endl << endl;
    cout << "1. Sign up" << endl;
    cout << "2. Log in" << endl;
    cout << "3. User list" << endl;
    cout << "4. Exit" << endl << endl;
    cout << "Please make your selection (enter 1, 2, 3, or 4): ";
}

void UserInfo::signUp()
{


    int offset;
    string line;
    bool isTaken;

    string fName;
    string lName;
    string bYear;
    string screenName;

    cout << "Enter your first name: ";
    cin >> fName;
    setFirstName(fName);

    cout << "Enter your last name: ";
    cin >> lName;
    setLastName(lName);

    cout << "Enter your birth year: ";
    cin >> bYear;
    setBirthYear(bYear);

    cout << "Create your screen name (no special characters or spaces): ";
    do
    {
        cin >> screenName;
        isTaken = false;
        fin.open("RegisteredUsers.txt");
        while (!fin.eof())
        {
            getline(fin, line);
            if ((offset = line.find(screenName, 0)) != string::npos)
            {
                isTaken = true;
                cout << "The username " << screenName << " is already taken, please choose a different username: ";
            }
        }
        fin.close();
    } while (isTaken = true && isTaken != false);

    setScreenName(screenName);

    cout << endl;
    cout << "Registered with the following information:" << endl;
    cout << endl;
    cout << "Full name: " << getFirstName() << " " << getLastName() << endl;
    cout << "Birth year: " << getBirthYear() << endl;
    cout << "Screen name: " << getScreenName() << endl;
    cout << "You may now log in to your newly created account." << endl;
    cout << endl;

    string infoFileName = screenName + "_info.txt";
    string followerFileName = screenName + "_follow.txt";
    string activityFileName = screenName + "_activity.txt";

    registerUser(fout, screenName);

    fout.open(infoFileName.c_str());
    writeUserInfo(fout, fName, lName, bYear, screenName);
    fout.close();
}

void writeUserInfo(ofstream & fout, string & first, string & last, string & year, string & screen)
{
    fout << first + " " + last + " " + year + " " + screen + " ";
}

void registerUser(ofstream & fout, string screen)
{
    fout.open("RegisteredUsers.txt", ios::app);
    fout << " " + screen;
    fout.close();
}

void listUsers()
{
    ifstream fin;
    string screen;
    cout << "The following users are registered: " << endl << endl;
    fin.open("RegisteredUsers.txt");
    while(!fin.eof())
    {
        fin >> screen;
        cout << screen << endl;
    }
    fin.close();
}

void UserInfo::signIn()
{
    int offset;
    string line;
    bool exists;
    string screenName;

    cout << "Please enter your screen name to sign in: ";
    do
    {
        cin >> screenName;
        exists = true;
            fin.open("RegisteredUsers.txt");
            while (!fin.eof())
            {
                getline(fin, line);
                if ((offset = line.find(screenName, 0)) == string::npos)
                {
                    exists = false;
                    cout << endl;
                    cout << "User " << screenName << " does not exist!" << endl << endl;
                    cout << "Please enter an existing username to sign in: ";
                }
            }
            fin.close();
    } while (exists = false || exists != true);
    cout << "You are now logged in as " + screenName << endl;
}

void UserInfo::displayProfile()
{

}

void UserInfo::setFirstName(string fName)
{
    _firstName = fName;
}

string UserInfo::getFirstName()
{
    return _firstName;
}

void UserInfo::setLastName(string lName)
{
    _lastName = lName;
}

string UserInfo::getLastName()
{
    return _lastName;
}

void UserInfo::setBirthYear(string bYear)
{
    _birthYear = bYear;
}

string UserInfo::getBirthYear()
{
    return _birthYear;
}

void UserInfo::setScreenName(string screenName)
{
    _screenName = screenName;
}

string UserInfo::getScreenName()
{
    return _screenName;
}

project02.h

#ifndef PROJECT02_H
#define PROJECT02_H

using namespace std;

void LoginScreen();

void writeUserInfo(ofstream & fout, string & first, string & last, string & year, string & screen);
void registerUser(ofstream & fout, string screen);
void listUsers();

class UserInfo
{
    public:
        string getFirstName();
        void setFirstName(string first);
        string getLastName();
        void setLastName(string last);
        string getBirthYear();
        void setBirthYear(string year);
        string getScreenName();
        void setScreenName(string sn);
        void signUp();
        void signIn();
        void displayProfile();
    private:
        string _firstName;
        string _lastName;
        string _birthYear;
        string _screenName;
};

#endif // PROJECT02_H

project02main.cpp

#include <iostream>
#include <fstream>
#include <string>
#include <cstdlib>
#include "project02.h"

using namespace std;

int main()
{
    char input;
    UserInfo inputInfo;

    cout << "Welcome to MyFace, a social media network where you can post your thoughts and see what your friends are up to." << endl << endl;

    do
    {
        LoginScreen();
        cin >> input;
        cout << endl;
        if (input == '1' && input != '2' && input != '3')
        {
            inputInfo.signUp();
        }
        else if (input == '2' && input != '3')
        {
            inputInfo.signIn();
        }
        else if(input == '3')
        {
            listUsers();
            cout << endl;
            system("PAUSE");
            cout << endl;
            LoginScreen();
        }
        else if (input != '1' && input != '2' && input != '3' && input != '4')
        {
            cout << "Invalid choice! Redirecting to login page..." << endl << endl;
        }
        else if (input == '4')
        {
            cout << "Thank you for using MyFace." << endl;
        }
    } while (input != '4');

    return 0;
}

As you can see, it doesn't yet include all the functionalities and signIn() doesn't actually do anything yet as that's where I left off. Anyway, I'm still just a beginner but please let me know if you see any potential problems or if I'm not using classes properly, or if you just have any general comments!

I don't actually have a whole lot of time to finish this so if something doesn't NEED to be fixed then I probably won't want to fix it, especially if it involves heavy restructuring or is otherwise time consuming.

\$\endgroup\$
  • 1
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. Feel free to post a new question with the revised code. It's good practice to wait for a couple of hours before doing so, to give other answerers the time to respond as well. \$\endgroup\$ – Mast Dec 7 '15 at 8:36
  • \$\begingroup\$ string::npos is just a simple static const size_type npos = -1;. You already declared ifstream fin;, use it instead of declaring again this (obviously, you must close the stream each time). while (exists = false || exists != true); has no sense - it means: while (exists = false || exists = false);. Same thing for the condition isTaken in the other while loop. Don't use system() functions, it brings your application less portable. Use '\n' instead of std::endl. In some case you can use sprintf with std::string.c_str(). You should also handle the exceptions. \$\endgroup\$ – user91060 Dec 7 '15 at 11:02
  • \$\begingroup\$ Make every member function const if they don't modify the object's state. Take not-cheap-to-copy objects like strings by const-ref instead of by-value. \$\endgroup\$ – Juho Dec 7 '15 at 19:56
  • 1
    \$\begingroup\$ @Pr0kram3r You should note the OP probably confused assignment = and equality comparison == operators there. \$\endgroup\$ – πάντα ῥεῖ Dec 7 '15 at 20:58
  • \$\begingroup\$ Thanks guys. I think I'm a bit weak in understanding what @mrm said but I'll look into it when I get the chance after my finals. Also I think I'm using the wrong = because == would work for some reason but really it just means that my code needs some fixing. \$\endgroup\$ – BaloneyOs Dec 7 '15 at 21:28
5
\$\begingroup\$

Namespaces

using namespace std; is considered bad practice. Short code is not a requirement in C++, clear code is preferred.

Return

return 0; is a legacy from C. In C++, it's no longer required to write this manually at the end of main. The compiler will take care of returning 'normal' if no errors where thrown or other returns (like -1) are encountered.

Naming

A project with a name like project02 will be hard to find if you need parts of it later. Try giving it a more meaningful name.

The following is confusing:

else if (input != '1' && input != '2' && input != '3' && input != '4')

Why is that not simply the latest else statement? If none of the other ifs are true, the input is and should be invalid.

\$\endgroup\$
  • \$\begingroup\$ Namespaces: yeah I did learn about that somewhat recently and I'll try to change this once this semester is over. Return: I didn't know that, but I'll keep that in mind as well. Naming: the professor wanted it to be that name. The else if statement: thanks for pointing that out. It must have been a remnant of code that wouldn't work otherwise before, but I fixed it now! \$\endgroup\$ – BaloneyOs Dec 7 '15 at 8:29
  • \$\begingroup\$ Any glaring issues with my utilization of objects/classes/etc? \$\endgroup\$ – BaloneyOs Dec 7 '15 at 8:32
  • 1
    \$\begingroup\$ using namespace std; is perfectly fine used within a translation unit, as long you know there are no clashes. The point where it's absolutely discouraged is using the statement in header files, because you're going to loose control over it (you cannot know in which context your header file will be included). \$\endgroup\$ – πάντα ῥεῖ Dec 7 '15 at 22:44
  • 1
    \$\begingroup\$ @πάνταῥεῖ He's using it everywhere, hence the note. Using it like this is definitely not fine and learning not to do it now will save him from un-learning bad-practices later. \$\endgroup\$ – Mast Dec 8 '15 at 0:11
  • 1
    \$\begingroup\$ This is the most poor answer I saw. 1) using namespace is considered bad if used in an header file. 2) He can perfectly use return 0;, no one cares if it's implied for the compiler. It's always preferred to not omit that. 3) Naming is not an improvement. \$\endgroup\$ – user91060 Dec 8 '15 at 13:03
5
\$\begingroup\$

I can spot a number of things in your code (suspected it's already working as intended), which should (actually need) to be improved:

1. Usage of global variables

ofstream fout;
ifstream fin;

These should not be global variables, but either local variables in the functions where you use them, or encapsulated in another class that manages all of the user registrations.

2. Usage of eof() in a loop condition

while (!fin.eof())

Rather use

    while(getline(fin, line))

Using a test for eof() directly in a loop condition is almost always considered wrong.

Just test if the input operation was good, as shown above.

3. Simplification of main() loop

The if/else if cascade in your main input loop should be simplified using a switch

    bool exit = false;
    do {
        LoginScreen();
        cin >> input;
        switch(input) {
        case '1':
            inputInfo.signUp();
            break;
        case '2':
            inputInfo.signIn();
            break;
        case '3':
            listUsers();
            cout << endl;
            system("PAUSE");
            break;
        case '4':
            exit = true;
            break;
        default:
            cout << "Invalid choice! Redirecting to login page..." << endl;
            break;
        }
    } while (!exit);

4. Overuse of strayed superfluous endl in output

Just as stated above, but that may be a matter of taste.

5. Constant use of redundant condition tests

Like here

while (exists = false || exists != true);

or here

while (isTaken = true && isTaken != false);

Either of these conditions already implies to be the other becoming true.
(Note there's also a possible error: to test for equality use ==, I don't believe the assignment = is used intentionally in these cases, correct me if I'm wrong about this please).

6. Usage of system("PAUSE"); isn't portable

Rather use something like

 cout << "Press ENTER to continue" << endl;
 getchar();

7. Usage of using namespace std; in header files

You should never use this statement (or any other using namespace xyz;) in header files. It's OK in translation units, if you're sure what you're doing.
But where header files are included is out of your control, and the using namespace statement may lead to unexpected namespace clashes.

Rather be explicit using scoped qualifiers (like e.g. std::string) in your header files, and remove the using namespace std;.


I'm pretty sure there's still more room for improving your code (regarding your requirements). E.g. introducing more classes that interact with each other as mentioned in my first point, like a RegistrationManager class, that actually will be responsible for management of the files to store already signed up users and finding the corresponding records there.


As a side note: In very 1st place make sure it works as intended when asking at Code Review.

\$\endgroup\$
  • \$\begingroup\$ Thank you! I'll look into fixing some of those. And yeah for sure the code runs and works as intended so far. \$\endgroup\$ – BaloneyOs Dec 7 '15 at 21:25
  • \$\begingroup\$ @BaloneyOs Are you really sure about those assignments in the conditions? \$\endgroup\$ – πάντα ῥεῖ Dec 7 '15 at 21:27
  • \$\begingroup\$ Hm they're working for me, or at least their associated functions, signIn() and creating a new username seem to work just fine. Strange thing is that for some reason it wouldn't work if I didn't include the second parameter but that could also be a coding issue as well. I'll roll with what I have just for the time being though and maybe come back to fix it later if I have spare time after finals and before project due date. \$\endgroup\$ – BaloneyOs Dec 7 '15 at 21:33
  • \$\begingroup\$ Oh yeah also for some reason cin.get(dummy) isn't waiting for user input. It just jumps to the next operation. \$\endgroup\$ – BaloneyOs Dec 7 '15 at 21:35
  • 1
    \$\begingroup\$ @πάνταῥεῖ just use a simple getchar();, no need of std::cin.get(); \$\endgroup\$ – user91060 Dec 8 '15 at 12:28

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.