4
\$\begingroup\$

I have an assignment due Friday to make a family tree. Started to code in January. My reviews from the teacher for this code was that the use of pointers was horrendous and it wasn't OOP enough. This code compiles and works. What can I do better to make the code more object oriented?

The best commentary was "It's horrendous to see pointers in the wild like this". At the same time, he taught us, so my assignment partner and I don't know what else to do. It seems like every time we ask for help, the code gets more complicated than necessary and we go further away from C++. I know that I have to create a node that takes in a person, but is there anything else?

This code is made to let a user put in him/herself and to add parents, grandparents etc. You can edit, delete and search.

class person
{
private:
public:
    std::string firstName;  // variable to hold the firstname of the person
    std::string lastName;   // variable to hold the lastname of the person
    int yearOfBirth;        // variable to hold the year of birth
    int age;                // variable to hold the age of person
    int alive;              // variable to hold data
    int sex;                // variable to hold the gender of the person

    person *left;
    person *right;
    person();
    ~person();
    void getData();
};

person::person() // constructor outside the class
{
    left = right = nullptr;
    firstName = "";
    lastName = "";
    yearOfBirth = 0;
    age = 0;
    alive = 0;
    sex = 0;
}

person::~person() = default; //destructor of person::person (check if this code runs and are okey!!)


void ignoreLine()
{
    std::cin.clear();
    std::cin.ignore(INT_MAX, '\n');
}

void person::getData() // get information about a person
{
    char gender;

    std::cout << "\nFirstname of the person: " << std::endl;
    std::cin >> firstName;
    std::cout << "Lastname of the person: " << std::endl;
    std::cin >> lastName;
    std::cout << "Age of " << firstName << ": " << std::endl;
    while (!(std::cin >> age) or (age < 0) or std::cin.fail()) // check if the inputs only contains positives int.
    {
        std::cin >> age;
        std::cout << "Only positives number allowed: " << std::endl;
        ignoreLine();
    }
    std::cout << "What is the sex of " << firstName << " (m/f): " << std::endl;
    std::cin >> gender;
    switch (gender) // all other int, char etc, than m/M is considered as female. Maybe change later, but good for now.
    {
        case 'm':
        case 'M':
            sex = 1;
            break;
    }
    ignoreLine(); // "pause" the program until the user press "Enter"
}



class familyTree
{
private:
public:

    person *root;
    person *search(const std::string&);
    person *traverseLeft(person *, const std::string&);
    person *traverseRight(person *, const std::string&);

    familyTree();
    void addNewPerson();
    void addMother(person *, person *);
    void addFather(person *, person *);

    static void show(person *);

    void printInOrder(person *, int);
};

familyTree::familyTree()
{
    root = nullptr;
}

void familyTree::addMother(person *a, person *b) // to add b as the mother of a
{
    if (a->left == nullptr)
    {
        a->left = b;
    }

    else
    {
        addMother(a->left, b);
    }
}

void familyTree::addFather(person *a, person *b)// to add b as the father of a
{
    while (a->right != nullptr)
        a = a->right;

    a->right = b;
}

person *familyTree::traverseLeft(person *ptr, const std::string& person)
{
    ptr = ptr->left;

    while (ptr != nullptr)
    {
        if ((ptr->firstName) == person) {
            return ptr;
        }

        else if (traverseRight(ptr, person) != nullptr)
        {
            return traverseRight(ptr, person);
        }
        else
        {
            ptr = ptr->left;
        }
    }
    return nullptr;
}

person *familyTree::traverseRight(person *ptr, const std::string& person)
{

    ptr = ptr->right;

    while (ptr != nullptr)
    {
        if ((ptr->firstName) == person)
        {
            return ptr;
        }
        else if (traverseLeft(ptr, person) != nullptr)
        {
            return traverseLeft(ptr, person);
        }
        else
            ptr = ptr->right;
    }
    return nullptr;
}


void familyTree::addNewPerson() 
{
    auto *temp = new struct person; 
    temp->getData();

    std::string personChild;

    if (root == nullptr)
    {
        std::cout << "\nFirst person added to the family!" << std::endl;
        root = temp;
    }

    else // if there exists a person in the tree, add the new person as a relative to a previous
    {
        std::cout << "Enter the name of the person " << temp->firstName << " is the parent to: " << std::endl;
        std::cin >> personChild;

        if (familyTree::search(personChild))  //tests if the person entered is found int the tree, if yes it continues.
        {
            int opt;

            std::cout << "\nWhat is the family relation? " << std::endl;
            std::cout << "(1) if " << temp->firstName << " is the Mother to " << personChild << std::endl;
            std::cout << "(2) if " << temp->firstName << " is the Father to " << personChild << std::endl;
            std::cout << "Enter: " << std::endl;
            std::cin >> opt;
            ignoreLine(); // "pause" the program until the user press "Enter"

            while (std::cin.fail() or opt < 1 or opt > 2)
            {
                std::cout << "\nOnly a number of 1 or 2 is accepted!" << std::endl;
                std::cout << "Enter: " << std::endl;
                std::cin >> opt;
                ignoreLine(); // "pause" the program until the user press "Enter"
            }

            switch (opt)
            {
                case 1:
                    addMother(search(personChild), temp);
                    std::cout << temp->firstName << " is now added as the mother!" << std::endl;
                    break;
                case 2:
                    addFather(search(personChild), temp);
                    std::cout << temp->firstName << " is now added as the father!" << std::endl;
                    break;
            }
        }

//        else
//        {
//          maybe make a code that loops the "enter name" until it's correct? Or just go straight back to main menu?
//        }
    }
}

void familyTree::printInOrder (person *person, int space) //print its persons using in order traversal
{
    auto count = 7; //using count for how much space between generations (horizontal)
    space += count;

    if (person != nullptr)
    {
        if(person->left != nullptr)
        {
            printInOrder(person->left, space);
        }

        std::cout << std::endl;
        for (int i = count; i < space; i++)
            std::cout << " ";
        std::cout << person->firstName << "\n";

        if(person->right != nullptr)
        {
            printInOrder(person->right, space);
        }
    }

    else
    {
        std::cout << "No tree exist!" << std::endl;
        ignoreLine(); // "pause" the program until the user press "Enter"
        return;
    }
}

person *familyTree::search(const std::string& per) // to search for a person
{
    person *ptr = root;

    if ((ptr->firstName) == per)
    {
        return ptr;
    }

    else if (traverseRight(root, per) != nullptr)
    {
        return traverseRight(root, per);
    }

    else if (traverseLeft(root, per) != nullptr)
    {
        return traverseLeft(root, per);
    }

    else
    {
        std::cout << "\nNo person found with the given name" << std::endl;
        ignoreLine(); // "pause" the program until the user press "Enter"
        return nullptr;
    }
}

void familyTree::show(person *ptr) // to show the information of a particular person
{
    std::string sex = "Female";
    if (ptr->sex)
    {
        sex = "Male";
    }

    std::cout << "\nName: " << ptr->firstName << " " << ptr->lastName << std::endl;
    std::cout << "Age: " << ptr->age << std::endl;
    std::cout << "Sex: " << sex << std::endl;
    std::cin.ignore();
}

void showMainMenu() // hold the output for the main menu
{
    std::cout << "Welcome" << std::endl;
    std::cout << "Please enter a number for your choice below:\n" << std::endl;
    std::cout << "(1) Add new person to tree" << std::endl;
    std::cout << "(2) Show information for a person" << std::endl;
    std::cout << "(3) Print complete family-tree" << std::endl;
    std::cout << "(4) Used for testing new choices" << std::endl;
    std::cout << "(0) Quit" << std::endl;
    std::cout << "\nYour choice: " << std::endl;
}

int main() 
{
    familyTree fT; // used to access/init. familytree class.

    int option, exit = 0;
    std::string temp, str;

    while (exit == 0)
    {
        showMainMenu();
        std::cin >> option;

        while (std::cin.fail())
        {
            ignoreLine();
            std::cout << "\nOnly a number between 0 and 10 is allowed: ";
            std::cin >> option;
        }

        switch (option)
        {
            case 1:
                fT.addNewPerson();
                break;
            case 2:
                std::cout << "Enter name of person to show information: ";
                std::cin >> temp;
                fT.show(fT.search(temp));
                break;
            case 3:
                fT.printInOrder(fT.root, 0);
                break;
            case 4:
                /* n/a */
                break;
            case 0:
                exit = 1;
                break;
        }
        std::cout << "\nPress enter to continue.." << std::endl;
        ignoreLine();
    }
    return 0;
}
\$\endgroup\$

1 Answer 1

5
\$\begingroup\$

This is by no means exhaustive, more a list of general highlights on what to focus on rather that deep analysis of the implementation.

Some general pieces of advice first:

  1. Use some coding conventions that easily allows identification of types, variables etc. What is person here: person *familyTree::traverseRight(person *ptr, const std::string& person)? Well, it ambiguous, or at least context-dependent. Writing like this is huge disservice to future maintainers of the code.
  2. Raw pointers are generally evil especially when taking modern C++ into account. However, using smart ones for trees, lists etc. can lead to stack overflow upon destruction of the data structure if that one is too big, due to destructors being called recursively. So it's either manual resource management with all its drawbacks, or correctness by default at the expense of limited size. This is for the author to decided. I can't tell that for you.
  3. Incorporate some testing framework, be it Catch2, Google Test, BoostTest; write unit tests to protect the existing functionalities. Even superficially tested code becomes tremendously easier to review and maintain.
  4. (not entirely review related) Familiarize yourself with version control and develop a habit of committing often, it saves a lot of time especially when testing new features as reverting to previous revision gets easier. That sentence:

the code gets more complicated than necessary and we go further away from C++. suggests you have troubles tracking what has been changed. Any VCS should make that way easier.

As for the code itself: First, I'd try to define responsibilities of classes and split them accordingly. Class person has mixed responsibilities: it's both the person as well as a tree node.

Let's extract the person first: If it's stripped of its node responsibilities it becomes way easier to maintain; all its special operations (move, copy, its respectable assignments etc.) become more automatic. If (de)serialization should be part of the class's member function or a freestanding one is an open question. I tend to prefer the freestanding ones (by overloading stream operators << and >>), but that's a matter of style, convention, taste and needs... you name it. I skipped that for brevity.

Also, consider using more suitable datatypes, e.g. boolean for alive, enumeration for sex, maybe unsigned instead of int for year (unless you're reaching BC; or maybe even choose a completely distinct type? I don't know, depends on your exact case).

Personally I'd go +- this way.

struct Person
{
    enum struct Sex{male, female};

    std::string firstName{};  // variable to hold the firstname of the person
    std::string lastName{};   // variable to hold the lastname of the person
    unsigned yearOfBirth{};        // variable to hold the year of birth
    unsigned age{};                // variable to hold the age of person
    bool alive{};              // variable to hold data
    Sex sex{};                // variable to hold the gender of the person
};

If person should be default-constructible - hard to say out of the blue. Yes, should you plan to read it from streams. Otherwise - I'd say no.

The next step would be fitting that into the tree. BTW, as far as I understand family trees, they don't have to be binary ones, but let's assume the whole assignment is about binary trees so it's been designed like that wittingly ;)

Also, I'm not sure if you're supposed to use templates or not. Assuming a non-templated version:

struct FamilyTreeNode
{
    Person person{};
    FamilyTreeNode* left;
    FamilyTreeNode* right;
};

Note I'm using raw pointers here. No problem with using the smart ones, at the price of potential stack-related problems.

And that can be enclosed inside a family tree:

struct FamilyTree
{
private:
    FamilyTreeNode* root;
    //I wouldn't make it public, but your call
    //also, maybe root doesn't necessarily has to be heap-allocated, but let's stick to that to keep it consistent...
 ////the rest

Now, as for family tree's interface: I would need to dig through that thoroughly and it is also highly dependent on ones needs. Hard for me to say if your preference is to operate directly on people (Person class) or maybe tree's nodes. I'd probably go the node way, making it somewhat C++- iterator-ish.

Also, keep const-correcntess in mind. E.g. method: person *search(const std::string&); should probably have its const overload, i.e. const person *search(const std::string&); const (or FamilyTreeNode instead of person in this case) should need to look for a person only for printing.

\$\endgroup\$
2
  • \$\begingroup\$ Personally, I'd stick with a signed int for yearOfBirth and age, mostly they seem like values likely to be involved in a subtraction computation at some point. You might as well leave them signed so that you don't have to do more work later to prevent absurdities like "Alice, 36, is 4,294,967,285 years older than Bob, 47" \$\endgroup\$ May 16 at 17:02
  • 2
    \$\begingroup\$ I wouldn't worry about the stack usage from smart pointers too much for this particular problem. The tree will be mostly balanced since most people will have both a mother and a father, and there will only be on the order of a hundred generations since AD 1. \$\endgroup\$
    – G. Sliepen
    May 16 at 21:02

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.