#include <iostream>
using namespace std;
class Train {
public:
char *name;
int number;
float time;
void inTrain(Train *train) {
out <<"Введите данные: "<<endl;
cout <<"Пункт назначания: "<<endl;
train->name = new char[25];
cin >>train->name;
cout <<"Номер поезда: "; cin >>train->number;
cout <<"Время отправления: "; cin >>train->time;
}
void outTrain(Train *train) {
cout <<"Пункт назначения: "<<train->name<<endl<<"Номер поезда: "<<train->number<<endl<<"Время отправления: "<<train->time<<endl;
}
};
int main() {
Train tr1,tr2,*train1, *train2;
train1 = &tr1;
train2 = &tr2;
int num;
while (1) {
cout << "Выберите действие: " <<endl<< "1 - Ввод данных" <<endl<< "2 - Вывод данных" <<endl<< "3 - Обработка данных" <<endl<< "4 - Выход" << endl;
cin >> num;
switch(num) {
case 1: tr1.inTrain(train1);
tr2.inTrain(train2);
break;
case 2: tr1.outTrain(train1);
tr2.outTrain(train2);
break;
case 3: if (tr1.name == tr2.name) cout <<"True"<<endl;
else cout<<"False"<<endl;
break;
case 4: return 0;
}
}
return 0;
}
1 ответ
Выделю, как мне кажется, основные моменты.
Что плохо
- весь класс открыт внешнему миру (все объявлено в секции public) Практически всегда данные должны быть приватными, а публичным должен быть интерфейс класса. Интерфейс - это методы, через которые пользователь класса будет с ним взаимодействовать. Данными в вашем случае будут поля name, number и time. Интерфейсом будут методы создания, получения информации и сравнения.
- методы inTrain и outTrain являются методами класса, но при этом принимают параметром указатель на объект класса Train, с которым и работают
- используются Си строки (char* name) и никак не ограничивается длина строки, следовательно массив name может переполниться, что приведет, скорее всего, к аварийному завершению приложения. К тому же для такого варианта использования name лучше объявлять ее явно в виде массива char name[25]; при таком объявлении не понадобится использовать оператор new. И сравнение Си строк таким образом tr1.name == tr2.name выполняет сравнение не содержимого строк, а сравнение адресов, по которым строки расположены в памяти.
- в switch отсутствует секция default. И приложение игнорирует неправильно заданную команду, ничего не сообщая пользователю.
- вместо стандартного значения true в цикле while используется 1, которая неявно приводится к true
- используется бесконечный цикл, что приводит к использованию двух точек выхода из приложения. В общем случае - это плохо, потому что затрудняет понимание кода.
- вся логика собрана в одной функции (main), что приводит к большой по размеру функции, выполняющей много совершенно разных действий. Такой код трудно понимать.
Как исправить
Для начала поправить класс Train
Сделать данные приватными, т.е. перенести их в секцию private, а интерфейс сделать публичным, т.е. оставить его в секции public. После этого ваш объект будет похож на
class Train{
public:
методы интерфейса
private:
данные
};
Исправить типы данных - name сделать строкой
std::string name;
теперь вам не придется беспокоится о выделени памяти для массива символов строки, о переполнении этого массива и сравнение строк будет выполняться именно по содержимому (для этого будет использоваться метод compare)
Перевести методы inTrain и outTrain на работу "со своим объектом", а не переданным им через параметр
void inTrain() {
cout <<"Введите данные: "<<endl;
cout <<"Пункт назначания: "<<endl;
cin >> name;
cout <<"Номер поезда: ";
cin >> number;
cout <<"Время отправления: ";
cin >> time;
}
void outTrain() {
cout
<< "Пункт назначения: " << name << endl
<< "Номер поезда: " << number << endl
<< "Время отправления: " << time << endl;
}
Для обработки данных вам потребуется получения получать пункт назначения поезда, для этого надо добавить функцию, которая будет возвращать это значение
std::string getName(){
return name;
}
в этом примере используется возврат строки по значению. Это плохо и в рабочем коде так делать не стоит. Здесь сделано так, чтобы не вдаваться в подробности.
после этого ваш класс будет выглядить так
class Train{
public:
void inTrain() {
cout <<"Введите данные: "<<endl;
cout <<"Пункт назначания: "<<endl;
cin >> name;
cout <<"Номер поезда: ";
cin >> number;
cout <<"Время отправления: ";
cin >> time;
}
void outTrain() {
cout
<< "Пункт назначения: " << name << endl
<< "Номер поезда: " << number << endl
<< "Время отправления: " << time << endl;
}
std::string getName(){
return name;
}
private:
std::string name;
int number;
float time;
};
Теперь поправить логику работы
Функции надо стараться делать как можно меньше и придерживаться правила одна функция - "одно действие". Т.е. если функция принимает команду пользователя, то она не должна ее еще и выполнять эту команду. При этом функциям следует давать имена, соответствующие выполняемым ими действиям. Логику вашего приложения можно свести к нескольким действиям: получение команды, выполнение команды "Ввод данных", выполнение команды "Вывод данных", выполнение команды "Обработка данных". Эти действия надо вынести в отдельные функции и перенести в эти функции код, который эти действия выполняет. Так же следует создать функцию обработки ввода неизвестной команды:
int getCommand(){
cout << "Выберите действие: " << endl
<< "1 - Ввод данных" << endl
<< "2 - Вывод данных" << endl
<< "3 - Обработка данных" << endl
<< "4 - Выход" << endl;
int num;
cin >> num;
return num;
}
void inputData(Train &tr1, Train &tr2){
tr1.inTrain();
tr2.inTrain();
}
void outputData(Train &train1, Train &train2){
tr1.outTrain();
tr2.outTrain();
}
void processData(Train &train1, Train &train2){
if (tr1.getName().compare(tr2.getName()) == 0){
cout << "True" << endl;
}else{
cout << "False" << endl;
}
}
void undefinedCmd(){
cout << "неизвестная команда" << endl;
}
В функции main следует оставить только логику, связывающую все в одно приложение, добавить обработку неизвестной команды и завершение работы через завершения цикла, вместо использования второй точки выхода.
int main() {
Train tr1,tr2;
bool run = true;
int cmd;
while (run) {
cmd = getCommand();
switch(cmd) {
case 1:
inputData(tr1, tr2);
break;
case 2:
outputData(tr1, tr2);
break;
case 3:
processData(tr1, tr2);
break;
case 4:
run = false;
break;
default:
undefinedCmd();
break;
}
}
return 0;
}
В итоге должно получиться так:
#include <iostream>
#include <string>
using namespace std;
class Train{
public:
void inTrain() {
cout <<"Введите данные: "<< endl;
cout <<"Пункт назначания: "<< endl;
cin >> name;
cout <<"Номер поезда: ";
cin >> number;
cout <<"Время отправления: ";
cin >> time;
}
void outTrain() {
cout
<< "Пункт назначения: " << name << endl
<< "Номер поезда: " << number << endl
<< "Время отправления: " << time << endl;
}
std::string getName(){
return name;
}
private:
std::string name;
int number;
float time;
};
int getCommand(){
cout << "Выберите действие: " << endl
<< "1 - Ввод данных" << endl
<< "2 - Вывод данных" << endl
<< "3 - Обработка данных" << endl
<< "4 - Выход" << endl;
int num;
cin >> num;
return num;
}
void inputData(Train &tr1, Train &tr2){
tr1.inTrain();
tr2.inTrain();
}
void outputData(Train &tr1, Train &tr2){
tr1.outTrain();
tr2.outTrain();
}
void processData(Train &tr1, Train &tr2){
if (tr1.getName().compare(tr2.getName()) == 0){
cout << "True" << endl;
}else{
cout << "False" << endl;
}
}
void undefinedCmd(){
cout << "неизвестная команда" << endl;
}
int main() {
Train tr1,tr2;
bool run = true;
int cmd;
while (run) {
cmd = getCommand();
switch(cmd) {
case 1:
inputData(tr1, tr2);
break;
case 2:
outputData(tr1, tr2);
break;
case 3:
processData(tr1, tr2);
break;
case 4:
run = false;
break;
default:
undefinedCmd();
break;
}
}
return 0;
}
tr1.name == tr2.name
почти никогда не будет работать. А оптимизировать можно кол-во параметров функций:)tr1.inTrain(&tr1);
и тому подобных вещей... Этот код надо не оптимизировать, а переписывать с нуля.