В пятницу у меня задание сделать генеалогическое древо. Начал кодить в январе. Мои отзывы учителя об этом коде заключались в том, что использование указателей было ужасным и недостаточно ООП. Этот код компилируется и работает. Что я могу сделать лучше, чтобы сделать код более объектно-ориентированным?
Лучшим комментарием было: «Ужасно видеть такие указатели в дикой природе». В то же время он учил нас, поэтому мы с моим партнером по заданию не знаем, что еще делать. Кажется, что каждый раз, когда мы обращаемся за помощью, код становится сложнее, чем необходимо, и мы все дальше уходим от C++. Я знаю, что мне нужно создать узел, который принимает человека, но есть ли что-нибудь еще?
Этот код создан для того, чтобы пользователь мог ввести себя и добавить родителей, бабушек и дедушек и т. д. Вы можете редактировать, удалять и искать.
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;
}
Шайеза
1 ответ
Это ни в коем случае не исчерпывающий, скорее список общих моментов, на которых следует сосредоточиться, а не глубокий анализ реализации.
Сначала несколько общих советов:
- Используйте некоторые соглашения о кодировании, которые позволяют легко идентифицировать типы, переменные и т. д. Что такое
personздесь:person *familyTree::traverseRight(person *ptr, const std::string& person)? Ну, это неоднозначно или, по крайней мере, контекстно-зависимо. Писать так — это огромная медвежья услуга будущим специалистам по сопровождению кода. - Необработанные указатели, как правило, зло, особенно если принять во внимание современный C++. Однако использование умных для деревьев, списков и т. д. может привести к переполнению стека при разрушении структуры данных, если она слишком велика, из-за рекурсивного вызова деструкторов. Так что либо ручное управление ресурсами со всеми его недостатками, либо корректность по умолчанию за счет ограниченного размера. Это решать автору. Я не могу сказать это вам.
- Включите какой-нибудь фреймворк для тестирования, будь то Catch2, Google Test, BoostTest; напишите модульные тесты для защиты существующих функций. Даже поверхностно протестированный код становится намного легче просматривать и поддерживать.
- (не полностью связанный с обзором) Ознакомьтесь с контролем версий и выработайте привычку часто совершать коммиты, это экономит много времени, особенно при тестировании новых функций, поскольку возврат к предыдущей версии становится проще. Это предложение:
код становится более сложным, чем необходимо, и мы уходим еще дальше от C++. предполагает, что у вас есть проблемы с отслеживанием того, что было изменено. Любая VCS должна упростить этот путь.
Что касается самого кода: во-первых, я бы попытался определить обязанности классов и разделить их соответствующим образом. Учебный класс person имеет смешанные обязанности: это и человек, и узел дерева.
Давайте сначала извлечем человека: если он лишен своих узловых обязанностей, его становится намного проще поддерживать; все его специальные операции (перемещение, копирование, его респектабельные назначения и т. д.) становятся более автоматическими. Должна ли (де) сериализация быть частью функции-члена класса или автономной, это открытый вопрос. Я предпочитаю автономные (перегружая потоковые операторы << а также >>), но это вопрос стиля, условностей, вкуса и потребностей… что угодно. Я пропустил это для краткости.
Кроме того, рассмотрите возможность использования более подходящих типов данных, например логических для aliveперечисление пола, может быть unsigned вместо int для года (если вы не достигнете BC; или, может быть, даже выберите совершенно другой тип? Я не знаю, зависит от вашего конкретного случая).
Лично я бы пошел +- таким образом.
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
};
Должен ли человек быть конструктивным по умолчанию — трудно сказать на ровном месте. Да, если вы планируете читать его из потоков. В противном случае — я бы сказал нет.
Следующим шагом будет установка этого в дереве. Кстати, насколько я понимаю генеалогические деревья, они не обязательно должны быть бинарными, но давайте предположим, что все задание посвящено бинарным деревьям, так что оно было спроектировано так намеренно 😉
Кроме того, я не уверен, должны ли вы использовать шаблоны или нет. Предполагая версию без шаблона:
struct FamilyTreeNode
{
Person person{};
FamilyTreeNode* left;
FamilyTreeNode* right;
};
Обратите внимание, что здесь я использую необработанные указатели. Нет проблем с использованием умных, за счет потенциальных проблем, связанных со стеком.
И это можно заключить в генеалогическое древо:
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
Теперь, что касается интерфейса генеалогического древа: мне нужно тщательно его изучить, и он также сильно зависит от потребностей. Мне трудно сказать, предпочитаете ли вы воздействовать непосредственно на людей (Person класс) или, может быть, узлы дерева. Я бы, вероятно, пошел по пути узла, сделав его чем-то вроде C++-итератора.
Кроме того, помните о константной корректности. Например, метод: person *search(const std::string&); вероятно, должен иметь свою константную перегрузку, т.е. const person *search(const std::string&); const (или же FamilyTreeNode вместо person в этом случае) нужно искать человека только для печати.
алагнер