0
    #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;
}
8
  • Что значит улучшить?
    – nick_gabpe
    Commented 29 сент. 2017 в 15:04
  • А какие участки кода недотсточно опмтимизированы?
    – Unick
    Commented 29 сент. 2017 в 15:16
  • это на код ревью нужно отправлять. тут хватает добра:) например, условие tr1.name == tr2.name почти никогда не будет работать. А оптимизировать можно кол-во параметров функций:)
    – KoVadim
    Commented 29 сент. 2017 в 15:16
  • 1
    Для начале неплохо бы сделать из этого нормальный С++, прежде чем задумываться об оптимизации. Использовать std::string, перестать передавать в функцию член класса указатель на сам класс, сделать члены класса private Commented 29 сент. 2017 в 15:31
  • 1
    Понимаете, после tr1.inTrain(&tr1); и тому подобных вещей... Этот код надо не оптимизировать, а переписывать с нуля.
    – Harry
    Commented 29 сент. 2017 в 18:23

1 ответ 1

2

Выделю, как мне кажется, основные моменты.

Что плохо

  • весь класс открыт внешнему миру (все объявлено в секции 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;
}

Ваш ответ

Нажимая «Отправить ответ», вы соглашаетесь с условиями пользования и подтверждаете, что прочитали политику конфиденциальности.

Всё ещё ищете ответ? Посмотрите другие вопросы с метками или задайте свой вопрос.