4
\$\begingroup\$

I am a beginner with C++, I am not learning OOP yet, I will start when I finish this project.
I'm working on "Educational Management System Project".
So I had trouble to code the part of login and sign up, I am not comfortable with this approach, I wonder if there is a solution to simplify this code and reducing if-statement and functions in authentication.cpp file.

ems.h

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

using namespace std;

struct Person;
struct Doctor;
struct TeacherA;
struct Student;
struct Course;
struct Assignment;
struct AssignmentSolution;

// Main data
struct Person
{
    int id;
    string username;
    string fullName;
    string email;
    string password;
};

struct Doctor
{
    Person info;
    vector <shared_ptr <Course>> courses;
};

struct TeacherA
{
    Person info;
    vector <shared_ptr <Course>> courses;
};

struct Student
{
    Person info;
    vector <shared_ptr <Course>> courses;
    vector <shared_ptr <AssignmentSolution>> assignmentSolutions;
};

struct Course
{
    string code;
    string title;
    shared_ptr <Doctor> lecturer;
    shared_ptr <TeacherA> assistant;
    vector <shared_ptr <Student>> registeredStudents;
    vector <shared_ptr <Assignment>> assignments;
};

struct Assignment
{
    string content;
    int maxMark;
    shared_ptr <Course> course;
    vector <shared_ptr <AssignmentSolution>> assignmentSolutions;
};

struct AssignmentSolution
{
    bool isMarked = false;
    string solution;
    int mark;
    string comment = "There is no comment";
    shared_ptr <Assignment> assignment;
    shared_ptr <Student> student;
};
extern vector <shared_ptr <Doctor>> doctors;
extern vector <shared_ptr <TeacherA>> teachersA;
extern vector <shared_ptr <Student>> students;
extern vector <shared_ptr <Course>> courses;

int mainMenu();
int startMethod(int role);
void signUp(int role);
void signIn(int role);
bool verifyDoctorData(string username, string password);
bool verifyStudentData(string username, string password);
void addUserInfo(int role, Person info);
int createID(int role);

ems.cpp

#include "ems.h"

vector <shared_ptr <Doctor>> doctors;
vector <shared_ptr <TeacherA>> teachersA;
vector <shared_ptr <Student>> students;
vector <shared_ptr <Course>> courses;

int main() {
    cout <<"* Welcome in Educational Management System Project *\n";

    mainMenu();
}

int mainMenu() {
    int role = -1;
    while (role) {
        cout << "\nPlease enter a choice: \n"
            << "\t[1] Doctor\n"
            << "\t[2] Teacher Assistant\n"
            << "\t[3] Student\n"
            << "\t[0] Exit\n"
            <<"Your choice: ";

        cin >> role;
        if (role) startMethod(role);
    }
}

int startMethod(int role) {
    int method = -1;
    while (method) {
        cout << "\nPlease enter a choice: \n"
            << "\t[1] Sign Up\n"
            << "\t[2] Sign In\n"
            << "\t[0] Back\n"
            <<"Your choice: ";
        cin >> method;

        if (method == 1)
            signUp(role);
        else if (method == 2)
            signIn(role);
    }
}

authentication.cpp

#include "ems.h"

void signUp(int role) {
    Person info;
    cout << "\nPlease enter the following information:\n";

    cout << "Full Name: ";
    getline(cin >> ws, info.fullName);

    cout << "E-mail: ";
    cin >> info.email;

    cout << "Username: ";
    cin >> info.username;

    cout << "Password: ";
    cin >> info.password;

    info.id = createID(role);

    if (role == 1 && !verifyDoctorData(info.username, info.password)) {
        addUserInfo(role, info);
    }
    else if (role == 3 && !verifyStudentData(info.username, info.password)) {
        addUserInfo(role, info);
    } else {
        cout << "This user already exist";
    }
}

void signIn(int role) {
    string username, password;

    cout << "Username: ";
    cin >> username;

    cout << "Password: ";
    cin >> password;

    if (role == 1 && verifyDoctorData(username, password)) {
        loginMenu(role, username);
    }
    else if (role == 3 && verifyStudentData(username, password)) {
        loginMenu(role, username);
    } else {
        cout << "Username or Password Incorect";
    }

}

bool verifyDoctorData(string username, string password) {
    for (auto doctor: doctors) {
        if (username == doctor->info.username && password == doctor->info.password) {
            return true;
        }
    }
    return false;
}

bool verifyStudentData(string username, string password) {
    for (auto student: students) {
        if (username == student->info.username && password == student->info.password) {
            return true;
        }
    }
    return false;
}

void addUserInfo(int role, Person info) {
    if (role == 1) {
        shared_ptr <Doctor> newDoctor (new Doctor);
        newDoctor->info = info;
        doctors.push_back(newDoctor);
    } else if (role == 3) {
        shared_ptr <Student> newStudent (new Student);
        newStudent->info = info;
        students.push_back(newStudent);
    }
}
\$\endgroup\$
  • 4
    \$\begingroup\$ Welcome to code review, you would get a better review if you posted the entire file so that we could properly review the code. \$\endgroup\$ – pacmaninbw Mar 23 at 17:42
  • 3
    \$\begingroup\$ @pacmaninbw I posted it, this was the first question here. \$\endgroup\$ – Abdelwahab Hussien Mar 23 at 18:43
  • \$\begingroup\$ @AbdelwahabHussien Why don't you use inheritance from Person for Teacher, Doctor and Student? \$\endgroup\$ – πάντα ῥεῖ Mar 23 at 19:21
  • 1
    \$\begingroup\$ Does the code work? If it doesn't it might be better to ask this question on stackoverflow.com rather then on code review. We only review working code. FYI, it seems that you should have a header file called authentication.h that contains the function prototypes for signUp(role); and signIn(role);. The functions createID(role) and addUserInfo(role, info); also seem to be missing. \$\endgroup\$ – pacmaninbw Mar 23 at 19:38
  • 2
    \$\begingroup\$ @πάνταῥεῖ I am not learning OOP yet, I will start when I finish this project. \$\endgroup\$ – Abdelwahab Hussien Mar 24 at 2:16
3
\$\begingroup\$

Here are some things that may help you improve your code.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. Know when to use it and when not to (as when writing include headers).

Provide complete code to reviewers

This is not so much a change to the code as a change in how you present it to other people. Without the full context of the code and an example of how to use it, it takes more effort for other people to understand your code. This affects not only code reviews, but also maintenance of the code in the future, by you or by others. One good way to address that is by the use of comments. Another good technique is to include test code showing how your code is intended to be used. For this code, I added these two functions to make it compileable:

int createID(int role) {
    static int id{1000};
    return ++id + role * 10000;
}

void loginMenu(int role, std::string& username)
{
    std::cout << "Successful login of " << username << " as role " << role << '\n';
}

Use objects

You have a Person structure and then separate functions that operate on Person data. With only a slight syntax change, you would have a real object instead of C-style code written in C++.
So to begin with, you might define a Person class like this:

class Person
{
public:
    Person(int id, std::string& username, std::string& fullName, std::string& email, std::string& password);
    bool isMatch(const std::string& name, const std::string& pw) const;
private:
    int id;
    std::string username;
    std::string fullName;
    std::string email;
    std::string password;
};

Now we can now create a derived class to define a Doctor:

class Doctor : public Person
{
public:
    Doctor(Person& p) : Person{p} {}
private:
    std::vector <std::shared_ptr <Course>> courses;
};

This derivation is the Object Oriented Programming (OOP) way to express the is-a relationship. That is, a Doctor is a Person so everthing that a person has or can do, a doctor has or can do.

Add behavior to objects for simpler code

Note too that we have defined the isMatch function for a Person. We might define it like this:

bool Person::isMatch(const std::string& name, const std::string& pw) const {
    return name == username && pw == password;
}

Now instead of this code:

bool verifyDoctorData(string username, string password) {
    for (auto doctor: doctors) {
        if (username == doctor->info.username && password == doctor->info.password) {
            return true;
        }
    }
    return false;
}

We can write this:

bool verifyDoctorData(std::string username, std::string password) {
    for (const auto& doctor: doctors) {
        if (doctor->isMatch(username, password)) {
            return true;
        }
    }
    return false;
}

However, even better is the following suggestion.

Use standard algorithms where practical

Do we really need to have separate verifyDoctorData and verifyStudentData? They're really nearly identical. What I'd do instead is to use std::none_of or std::any_of and the function mentioned above. So instead of this:

if (role == 1 && !verifyDoctorData(info.username, info.password)) {
    addUserInfo(role, info);
}

One could write this:

if (role == 1 && std::none_of(doctors.cbegin(), doctors.cend(), std::bind(&Person::isMatch, std::placeholders::_1, username, password))) {
    addUserInfo(role, info);
}

Now there is no need at all to write a verifyDoctorData function.

Use an enum where appropriate

In the code above, instead of role == 1, wouldn't it make more sense to write role == doctor? One could do that with an enum.

There's much more, but this should be enough to get you started and to convey to you that learning C++ and learning OOP are not separate phases.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks, this was helpful to me, and of course, I'm going to start learning oop that looks powerful. \$\endgroup\$ – Abdelwahab Hussien Mar 25 at 13:07
1
\$\begingroup\$

Well, hello. I was getting a bit bored today, so thank you for giving me a nice little exercise to work on. I edited the code, removing unnecessary lines and editing things here and there.

Since you specified that you don't yet know much about OOP I made sure not to take the easier approach, which in my mind is a vector of pointers to a virtual class named and then using it to fit in other higher derived classes in them. This is a hobby for me so I have no clue if I'm correctly communicating the concepts I'm trying to talk about, but I tried.

Part of the question was if there was a better way to reduce the number of if statements in the code that are needed by the UI to test the user's input and make selections based on them. The best way of doing that than I ever thought about is by using a map with the selections already in it as keys and having function pointers as values that can be called with a key.

There are suggestions I have for you, with a very shallow and not at all comprehensive explanation:

  • Avoid using namespace std;; bad practice
  • Learn OOP; It would have been more than beneficial in this case
  • (Personal Opinion) Start using the c io functions printf() and scanf(); format strings make code so much neater(It's a beautiful thing)
  • Try to make your program as simple as you possibly can; it makes things easier to read, edit and understand(You didn't need to have all those function declarations in the header file, and there were some unneeded std::shared_ptrs)
  • (Personal Opinion) Make a main.cpp file for main(); putting it in a random file is unconventional
  • Use the header guards; the errors that come with not using them are bothersome
  • Usually, C++ header files are given the extension .hpp; I 99% sure it helps the compiler distinguish between C headers and C++ headers, but I'm doubting myself a little on this one.
  • (For this program specifically) If you intend on using it to a degree, you should probably get write a function that can pass out the information and another to read it in; Who wants to Sign-Up to a service every time to Sign-In
  • Get in the habit of writing lists of things you can improve on.

Don't worry, when I started off I got banned from stack overflow because of my cluelessness(Feel free to check out the questions I used to ask, they're fun to laugh at). I'm still banned to this day. Everyone has things they can work on. For me, it would be getting unbanned.

Anyways, the insertion code is incomplete because I got bored, so you can't actually insert any information, but I gave you a starting point for your UI and I can't do everything for you(Basically I'm too lazy to finish, but you get to learn something and I get to relax, so it's a win-win). I might get bored of relaxing and come back and finish this, but this should sufficiently help you for now.

Here's the code:

main.cpp

#include "ems.hpp"

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

ems.hpp

#ifndef ems_hpp
#define ems_hpp

#include <vector>
#include <string>
#include <memory>

// Main data
struct Person
{
    int id;
    std::string username;
    std::string fullName;
    std::string email;
    std::string password;


};

struct Assignment;
struct AssignmentSolution
{
    bool isMarked = false;
    std::string solution;
    int mark;
    std::string comment = "There is no comment";
    std::shared_ptr <Assignment> assignment;
    std::shared_ptr <Person> student;
};

struct Course;
struct Assignment
{
    std::string content;
    int maxMark;
    std::shared_ptr <Course> course;
    std::vector <std::shared_ptr <AssignmentSolution>> assignmentSolutions;
};

struct Course
{
    std::string code;
    std::string title;
    std::shared_ptr <Person> lecturer;
    std::shared_ptr <Person> assistant;
    std::vector <std::shared_ptr <Person>> registeredStudents;
    std::vector <std::shared_ptr <Assignment>> assignments;
};


// Data
extern std::vector <Person> doctors;
extern std::vector <Person> teachers;
extern std::vector <Person> students;
extern std::vector <std::shared_ptr <Course>> courses;

// Current user
extern std::string currentUser;

// Functions
void runEducationalManagementSystemProject();

#endif /* ems_hpp */

ems.cpp

#include "ems.hpp"
// Externs
std::vector <Person> doctors;
std::vector <Person> teachers;
std::vector <Person> students;

std::string currentUser;

#include <cstdio>
#include <map>
#include <string.h>

bool exists(std::string uName){
    printf("Checking if \"%s\" exitsts", uName.c_str());
    for (int i = 0; i < doctors.size(); i++) {
        if (strcmp(doctors.at(i).username.c_str(),uName.c_str())) {
            return true;
        }
    }

    for (int i = 0; i < teachers.size(); i++) {
        if (strcmp(teachers.at(i).username.c_str(),uName.c_str())) {
            return true;
        }
    }

    for (int i = 0; i < students.size(); i++) {
        if (strcmp(students.at(i).username.c_str(),uName.c_str())) {
            return true;
        }
    }
    return false;
}

bool correctPassword(std::string uName, std::string password){
    for (int i = 0; i < doctors.size(); i++) {
        if (strcmp(doctors.at(i).username.c_str(),uName.c_str())) {
            if (strcmp(doctors.at(i).password.c_str(), password.c_str())) {
                return true;
            } else {
                return false;
            }
        }
    }

    for (int i = 0; i < teachers.size(); i++) {
        if (strcmp(teachers.at(i).username.c_str(),uName.c_str())) {
            if (strcmp(teachers.at(i).password.c_str(), password.c_str())) {
                return true;
            } else {
                return false;
            }
        }
    }

    for (int i = 0; i < students.size(); i++) {
        if (strcmp(students.at(i).username.c_str(),uName.c_str())) {
            if (strcmp(students.at(i).password.c_str(), password.c_str())) {
                return true;
            } else {
                return false;
            }
        }
    }

    exit(1); // No user exists even though exits returned true, stop the program from running in needless perpetual loop
    return false;
}

struct MainMenu {
    static std::string display;
    static std::map<unsigned int, void (*)()> options;
};
std::string MainMenu::display;
std::map<unsigned int, void (*)()> MainMenu::options;

void mainMenu(){
    while (true) {
        printf("%s",MainMenu::display.c_str());
        int option = 0;
        scanf("%d", &option);
        MainMenu::options[option](); // Calls the function that has been connected to the key int the setup
    }
}

void signUp() {
    Person newUser;

    char* buffer;

    printf("\nPlease enter the following information:\n");

    printf("Full Name: ");
    scanf("%s", buffer);
    newUser.fullName = std::string(buffer);

    printf("E-mail: ");
    scanf("%s", buffer);
    newUser.email = std::string(buffer);

    printf("Username: ");
    scanf("%s", buffer);
    newUser.username = std::string(buffer);
    while (exists(newUser.username)) {
        printf("The username %s is already taken\n", newUser.username.c_str());
        printf("Username: ");
        scanf("%s", buffer);
        newUser.username = std::string(buffer);
    }

    printf("Password: ");
    scanf("%s", buffer);
    newUser.password = std::string(buffer);

    students.push_back(std::move(newUser));
}

void signIn() {

    printf("\nPlease enter Sign-In details: \n\tUsername: ");
    char* buffer;
    scanf("%s", buffer);

    std::string newSignIn(buffer);

    int chances = 3;
    tryAgain:
    printf("\tPassword: ");
    scanf("%s", buffer);

    std::string password(buffer);

    if (exists(newSignIn)) {
        // Check if the password given matches that users password
        if (correctPassword(newSignIn, password)) {
            currentUser = newSignIn;
        } else {
            printf("Incorrect Password try again(%d)\n", chances);
            chances--;
            if (chances == 0) {
                printf("Sorry, you've used up all your chances\n");
                return;
            }
            goto tryAgain;
        }
    } else {
        printf("User does not exits, did not sign-in\n");
    }
}

void quit(){
    exit(0);
}

void setup(){
    // Main Menu setup
    MainMenu::display = "\nPlease enter a choice: \n\t[1] Sign-In\n\t[2] Sign-Up\n\t[0] Quit\n";
    MainMenu::options.insert(std::make_pair(1, &signIn));
    MainMenu::options.insert(std::make_pair(2, &signUp));
    MainMenu::options.insert(std::make_pair(0, &quit));
}

void runEducationalManagementSystemProject() {
    setup();
    mainMenu();
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Stream formatting is indeed painful to use, but the C functions are worse because they are type unsafe, cannot be customized, and risk buffer overflow (for scanf). Use the {fmt} library instead, which is also known to have the best performance among similar libraries and standardized in C++20. \$\endgroup\$ – L. F. Mar 26 at 1:13

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.