Я хотел бы знать, если следующий код является "хорошим"


Я бы серьезная, тупой и холодный суждение на этот код. Будет ли это считаться "хорошим" кодом? В общем, это хорошо написано? Это разумный дизайн?

#include <iostream>   // C++ IO library code to include.
#include <string>     // C++ string functions.
using namespace std ; // Use standard C++ names for functions.


//====== class declaration.

class Tpacket_list
{public: // functions that are available to code outside the class.
    // put the command line into the list.
    Tpacket_list(int param_index, string param) ;  
    void print_list() ; // print the list.
    Tpacket_list* find( string target) ;
    int    tell_id() ;
    string tell_payload() ;

 protected:                //  Data in a class is normally hidden from other code.
    string payload ;       // C++ strings are classes so this is aggregation.
    int id ;               // Such strings are more powerful and easier to use
    Tpacket_list *next ;   //   than the older C string routines, also slower!
} ;


Tpacket_list *list_head ;  // Start of linked list, set to 1st item.
Tpacket_list *work_ptr ;   // Work pointer to work along list.

//====== Class Body =========================================================

//------ Constructor gets called when the class is created. It puts in the
//       payload and sets up the chain of pointers.All this is hideen from
//       the user of the class.
Tpacket_list::Tpacket_list(int param_index, string param) 
{//--- save information for this parameter.
   id = param_index ;
   payload = param ;
   cout << "    Adding parameter # " << id << ", value = " << payload << endl ;
 //--- form list.
   next = list_head ;
   list_head = this ;
}

//------ Print the list.  Note how the function calls the print_list function
//       in the next item in the link list.
//       Function prints from the current link list item to the end.
void Tpacket_list::print_list()
{//---
   cout << "    id = " << id << ",  payload = " << payload << ", length " << payload.length() << " characters."<< endl ;
   if ( next != NULL) // if there is a next item, get it to print itself.
     next->print_list() ;
}


//------ Find item in the list.
Tpacket_list* Tpacket_list::find( string target)
{//---
   if ( payload == target)
      return ( this) ;          // return pointer to current object.
   if ( next == NULL)
      return( NULL) ;           // end of list, not match, return NULL.
   //--- if got here there is a valid next.
   return( next->find( target)) ;
}

//------ Tell of internal data.
int    Tpacket_list::tell_id() 
 { return( id) ;
 }

string Tpacket_list::tell_payload()
 { return( payload) ;
 }

//====== Start program here =================================================

int main(int argc, char *argv[]) // argc = number strings on command line.
                                 // argv is array of strings each holding 
                                 //   a parameter.

{//--- Initialize link list with command parameters.
   cout << endl << "  Form link list of command line parameters." << endl ;
   list_head = NULL ;
   int i = 1 ;
   while ( i < argc)
    { new Tpacket_list(i, argv[i]) ;
      i++ ;
    }

 //--- print out the list.
   cout << "  Print paramters from head of list the end of list." << endl ;
   if ( list_head != NULL)
        list_head->print_list() ;
   else cout << "   List is empty." << endl ;

 //--- search for parameters
   cout << "  Find first 0 parameter (working last to first parameter, list head to tail)." << endl ;
   Tpacket_list *found ;
   if ( list_head != NULL)
     { found = list_head->find("0") ;
       if ( found != NULL)
            cout << "    Found target on parameter # " << found->tell_id()  << endl ;
       else cout << "    No \"0\" found." << endl ;
     }

   cout << endl ;
   return(0) ;
}

P. S. Это не мой код, это на самом деле лектора.



608
5
c++
задан 29 сентября 2011 в 09:09 Источник Поделиться
Комментарии
5 ответов

Для общего качества, я бы дал около 2 или 3 (по шкале 1-10). Честно, даже 3 будет довольно щедрым.

#include <iostream>   // C++ IO library code to include.
#include <string> // C++ string functions.
using namespace std ; // Use standard C++ names for functions.

с помощью пространства имен std; обычно неодобрительно. Есть несколько причин для использования пространства имен, но делают это во всем мире так (особенно с СТДпространства имен) это просто плохой идеей.

//====== class declaration.

Я раздвоения на этом. С одной стороны, бессмысленно, подобные комментарии, как правило, хуже, чем бесполезно. В то же время, учитывая, что это написано исключительно для учебных целей, он может быть простительно включают несколько вещей, которые мы обычно отвергаем, описывая основы того, как работает c++. Это будет не в реальном коде, но в данных обстоятельствах это понятно и (я думаю) разумный.

class Tpacket_list
{public: // functions that are available to code outside the class.

Твой учитель, кажется, трудно работать в опровержение обычных мудрость о форматировании. Большинство людей понять, что точного форматирования вы выберете менее важна, чем постоянно использовать его. Это один из немногих я видел, что действительно, кажется, чтобы сделать код более трудным для чтения.

    // put the command line into the list.
Tpacket_list(int param_index, string param) ;
void print_list() ; // print the list.
Tpacket_list* find( string target) ;
int tell_id() ;
string tell_payload() ;

Комментарий здесь вводит в заблуждение в лучшем случае. Код, чтобы поставить командную строку в список в другом месте (где-то в главном).

 protected:                //  Data in a class is normally hidden from other code.
string payload ; // C++ strings are classes so this is aggregation.
int id ; // Such strings are more powerful and easier to use
Tpacket_list *next ; // than the older C string routines, also slower!
} ;

Здесь мы имеем реальную проблему дизайна. Он объединяет две идеи, которые должны быть полностью отделены: связанный список, и узел в связанном списке. Работать вообще хорошо, связный список почти на две:

class list {    
struct Node {
int id;
std::string param;
Node *next;

Node(int param_index, std::string const &p) :
id(param_index), param(p)
{}
}

public:
list(list const &);
// ...
private:
node *list_head;
};

В этом случае узел - это просто тупой сведения; список класса имеет все реальные "знания" о том, как делать полезные вещи. Также обратите внимание, что в реальности, это почти наверняка будет шаблон класса вместо фактического класса. Вместо жесткого кодирования в инт и СТД::строка как типы хранимых данных, мы действительно хотим, чтобы те, шаблон параметров, поэтому мы можем создать связанный список каких-либо данных, мы хотим.

Tpacket_list *list_head ;  // Start of linked list, set to 1st item.
Tpacket_list *work_ptr ; // Work pointer to work along list.

"Папа, он определил глобальные!"
"Сын, что я тебе говорил об использовании языка, как это?"

//====== Class Body =========================================================

//------ Constructor gets called when the class is created. It puts in the
// payload and sets up the chain of pointers.All this is hideen from
// the user of the class.

Я повторю свой прежний комментарий о комментарии для акцента: в то время как (с трудом), приемлемых при данных обстоятельствах, это было явно не место в реальном коде.

Tpacket_list::Tpacket_list(int param_index, string param) 
{//--- save information for this parameter.
id = param_index ;
payload = param ;
cout << " Adding parameter # " << id << ", value = " << payload << endl ;
//--- form list.
next = list_head ;
list_head = this ;
}

Этот код имеет достаточно проблем. Прежде всего, вы должны, как правило, предпочитают инициализация членов в списке инициализации:

Tpacket_list::Tpacket_list(int param_index, string param) :
id(param_index),
payload(param)

ОТО, в предыдущем комментарии, этот код действительно должен быть в узле::вместо узла. С точки зрения связанного списка, этот код не должен быть конструктор на всех. После обычных конвенций для C++, это может быть функция с именем push_front (а это обычно только принимают один тип данных в качестве входных данных-хранить в инт и СТД::строка, Мы поместили их вместе в структуру, какой-то-это с std::пара, если ничего другого.

//------ Print the list.  Note how the function calls the print_list function
// in the next item in the link list.
// Function prints from the current link list item to the end.
void Tpacket_list::print_list()
{//---
cout << " id = " << id << ", payload = " << payload << ", length " << payload.length() << " characters."<< endl ;
if ( next != NULL) // if there is a next item, get it to print itself.
next->print_list() ;
}

ИМО, само наличие этой функции-это проблема. Для распечатки содержимого списка, мы действительно хотите применить алгоритм (например, с std::копия) в список (или некоторые из них). Для этого, наш список должен определить начала() и конца(), которые возвращают список итераторов. Итератор типа следует определить (по крайней мере) оператор++ для обхода списка. В нашем случае итератор типа будет довольно тонкая обертка вокруг указателя на узел:

class list {
class Node { /* ... */ };

// warning: typing this in off the top of my head. It probably has at least a few
// problems.
class iterator {
Node *pos;
public:
Node operator*() { return *pos; }
iterator &operator++() {
if (NULL == pos)
return NULL;
pos = pos -> next;
return *this;
}
bool operator==(iterator const &other) {
return pos == other.pos;
}
};

Таким образом, когда мы хотим напечатать список, мы можем использовать те же алгоритмы (и фразеологизмов в целом), что мы можем с любым из других контейнеров в стандартной библиотеке.

//------ Find item in the list.
Tpacket_list* Tpacket_list::find( string target)
{//---
if ( payload == target)
return ( this) ; // return pointer to current object.
if ( next == NULL)
return( NULL) ; // end of list, not match, return NULL.
//--- if got here there is a valid next.
return( next->find( target)) ;
}

Опять, наверное, бессмысленно-вместо того, чтобы найти функции, обеспечить соответствующие типу итератора, так что можно использовать с std::найти (среди прочего).

//------ Tell of internal data.
int Tpacket_list::tell_id()
{ return( id) ;
}

string Tpacket_list::tell_payload()
{ return( payload) ;
}

Я не могу сказать, как эти много, но они единственное, что до сих пор это не очевидно/явно не так...

//====== Start program here =================================================

int main(int argc, char *argv[]) // argc = number strings on command line.
// argv is array of strings each holding
// a parameter.

{//--- Initialize link list with command parameters.
cout << endl << " Form link list of command line parameters." << endl ;
list_head = NULL ;
int i = 1 ;
while ( i < argc)
{ new Tpacket_list(i, argv[i]) ;
i++ ;
}

Здесь мы видим плохое воздействие на плохой дизайн. Этот код претендует на создание (и, кстати, утечка) АГДС-1 отдельная Tpacket_listС. В реальности, он создает единый список ссылок, потому что конструктор делает неявное push_front на (в единственном числе) глобального связанного списка, представленного неприятные мировой. Для удобства использования мы явно хотят что-то вроде этого:

    linked_list<std::pair<int, std::string> > list;

for (int i=1; i<argc; i++)
list.push_back(std::make_pair(i, argv[i]));

//--- print out the list.
cout << " Print paramters from head of list the end of list." << endl ;
if ( list_head != NULL)
list_head->print_list() ;
else cout << " List is empty." << endl ;

Затем, чтобы распечатать список, у нас был бы код, как:

if (list.empty())
std::cout << "List is empty.\n";
else
std::copy(list.begin(), list.end(),
std::ostream_iterator<std::pair<int, std::string>(std::cout, "\n"));

и конечно:

 //--- search for parameters
cout << " Find first 0 parameter (working last to first parameter, list head to tail)." << endl ;
Tpacket_list *found ;
if ( list_head != NULL)
{ found = list_head->find("0") ;
if ( found != NULL)
cout << " Found target on parameter # " << found->tell_id() << endl ;
else cout << " No \"0\" found." << endl ;
}

...поиск в списке будет осуществляться с использованием std::найти вместо того, чтобы писать наши собственные (топорная) подделка.

Как окончательное Примечание, поскольку связанный список динамически распределяет узлы, мы почти наверняка должны предоставить dtor, который будет удалять эти узлы. Мы также хотим предоставить копию конструктор, который будет копировать узлы, или объявить конструктор копирования в частном порядке1 для того, чтобы никто копировать список ссылок на все.


1 в C++11, мы можем использовать =удалить синтаксис, чтобы удалить его, но это может быть немного преждевременно, чтобы учить, что как обычный C++ с использованием только пока.

12
ответ дан 29 сентября 2011 в 02:09 Источник Поделиться

Приведенный ниже код не является совершенным. Но мы должны учитывать, что мы пытаемся донести основные понятия к группе людей, которые не понимают полном объеме с++ еще. Таким образом, определенные сочетания будут сделаны на этой сложности. В течение года вы можете вновь посетить этот код и видеть постепенные улучшения в своем классе увеличивает свое знание языка.

Например я прокомментировала места, где я бы ожидал увидеть потоки/смарт-указатели/константный corectnes/итераторы/исключения. Но было бы бессмысленно включать эти понятия в программе преподается для абсолютных новичков. Нужно учить тому, что впоследствии дооснастить код с новой концепцией.


Резюме:

Код функции обертывания в объект узла, который, вероятно, должен быть заключен в высокий уровень строительства (как объект списка). Есть несколько подсказок в коде, которые свидетельствуют о том, что это работа в процессе обучения понятиями (так можно простить).

Так много комментариев на реальном коде.
Они загромождали вещи и сделать его трудно читать (но порядке, так как это является частью учебного курса).

Это просто лень
Не используя пространства имен , если это вообще возможно

using namespace std ;


Данные в классе обычно скрыты от других код.Строки C++ классы, так это агрегация. Такие струны более мощный и проще в использовании, чем старше, c строка процедуры, также медленнее!

Я не согласен с медленнее. Строку C++ может использовать несколько байтов, но если основное использование будет быстрее, потому что размер, если предварительно вычисленные (вы были бы удивлены, сколько раз размер c-строка обновляется).

class Tpacket_list
{public:

Это действительно некрасиво. Поставить общественность на отдельной строке (сделать его читабельным).

void print_list() ;

Распечатать список где? Обычно, когда вы печатаете то, что вы печатаете на поток (с std::соиь). Как правило, вы хотите, чтобы убедиться, что вы можете пройти в различного типа поток, так что он может перейти в файл или быть сериализован в строку для транспорта. Так что я бы ожидал распечатать заявление, чтобы взять объект потока, так что поток может быть распечатана.

Также в объекты C++ обычно печатаются через << оператора. Но это приемлемо, чтобы положить метод печати в классе в качестве помощника оператора. Оператор поток может быть то, что придет последним в курсе.

    Tpacket_list* find( string target) ;

Указатели-это очень плохая идея. Они не передают право собственности на возвращенный объект (так кто отвечает за их удаление). Я предполагаю, что проблема в том, что если вы сможете найти нужный объект вам нужен способ, чтобы указать, что он не был включен, и вы можете использовать null, чтобы указать на это. Контейнеры в C++ обычно возвращают итераторы на найти. Неспособность найти элемент возвращает конец() (который является итератор на один конец контейнера).

Также предпочитают передавать параметры const ссылка (для предотвращения ненужного копирования).

    int    tell_id() ;
string tell_payload() ;

Похоже, последние три метода должны быть помечены как const.
Все они, кажется, не мутирует методов. Таким образом, они все Конст.

    protected:                

Защищен обеспечивает никакой защиты (это делает, но это иллюзия, которая легко прокалывается). Предпочитают использовать частные (особенно в данном случае). С помощью публичных/защищенных ты в том числе все эти элементы в общий интерфейс (это понятие ОО не концепта C++) объекта. Что-нибудь в общедоступном интерфейсе должен быть сохранен и в будущем, таким образом, крепко связывая вас к этой реализации.

    Tpacket_list *next ;

Сочетая бизнес-данных и ресурсов в единый объект нарушает принцип разделения задач. Делая свой объект не как управление ресурсами (транспортная обработка действий список) и бизнес-логики (пакетная передача данных) вы могли бы сделать его более компактным, но вы делаете код более сложным, чтобы справиться с этой ситуацией. В результате вы будете платить за ваш компактный данных с более код.

Tpacket_list *list_head ;  // Start of linked list, set to 1st item.
Tpacket_list *work_ptr ; // Work pointer to work along list.

ОК. Я собираюсь игнорировать это и предположить, что это работа в прогрессе. В противном случае ваше приложение может иметь только один список.

Tpacket_list::Tpacket_list(int param_index, string param)
{
id = param_index ;
payload = param ;

Предпочитают использовать список инициализации.

void Tpacket_list::print_list()
{
if ( next != NULL)
next->print_list() ;

Рекурсия-это ловкий трюк, чтобы преподавать в образовательных целях. Но в промышленном коде предпочитаю петлю. Рекурсия имеет потенциал дует свой стек. Петля всегда будет работать независимо от размера списка. Должно быть инкапсулировано в на более высоком уровне (владелец узлы списка).

3
ответ дан 29 сентября 2011 в 04:09 Источник Поделиться

Сказать правду, это плохой код, комментарии единственная хорошая вещь, хотя они могли бы быть лучше. Вот несколько способов, чтобы улучшить ваш код:


  • использовать несколько файлов: объявления классов и функций должно быть .H, а реализация должна быть в отдельном файле, было бы лучше также, чтобы поставить основной отдельно

  • существует большое форматирование должно быть сделано,

как для

if ( found != NULL)
cout << " Found target on parameter # " << found->tell_id() << endl ;
else cout << " No \"0\" found." << endl ;

может быть

if ( found != NULL)
cout << " Found target on parameter # " << found->tell_id() << endl ;
else
cout << " No \"0\" found." << endl ;

или даже лучше

if ( found != NULL) {
cout << " Found target on parameter # " << found->tell_id() << endl ;
} else {
cout << " No \"0\" found." << endl ;
}

так что вы можете позже добавить в него код без каких либо проблем

2
ответ дан 29 сентября 2011 в 11:09 Источник Поделиться

#include <iostream>   // C++ IO library code to include.
#include <string> // C++ string functions.
using namespace std ; // Use standard C++ names for functions.

//====== class declaration.

class Tpacket_list
{public: // functions that are available to code outside the class.

Лучше поставить на строке

    // put the command line into the list.
Tpacket_list(int param_index, string param) ;
void print_list() ; // print the list.
Tpacket_list* find( string target) ;
int tell_id() ;
string tell_payload() ;

скажи? обычно люди используют вам, или нет префикса. Используя скажу-это странно.

 protected:                //  Data in a class is normally hidden from other code.
string payload ; // C++ strings are classes so this is aggregation.
int id ; // Such strings are more powerful and easier to use
Tpacket_list *next ; // than the older C string routines, also slower!
} ;

Tpacket_list *list_head ; // Start of linked list, set to 1st item.
Tpacket_list *work_ptr ; // Work pointer to work along list.

Глобальные переменные не рекомендуется

//====== Class Body =========================================================

//------ Constructor gets called when the class is created. It puts in the
// payload and sets up the chain of pointers.All this is hideen from
// the user of the class.

Вы не должны сказать мне, что конструктор вызывается при создании класса ТЭН. Каждый, кто читает ваш код будет знать, что. Ваш комментарий должен действительно описать значение параметров не обсуждение того, что скрыто от пользователя.

Tpacket_list::Tpacket_list(int param_index, string param) 
{//--- save information for this parameter.
id = param_index ;
payload = param ;
cout << " Adding parameter # " << id << ", value = " << payload << endl ;
//--- form list.
next = list_head ;
list_head = this ;

Его обычно лучше управлять списком вне класса. Всегда когда-либо элемент в списке-это обычно плохая идея.

}

//------ Print the list. Note how the function calls the print_list function
// in the next item in the link list.
// Function prints from the current link list item to the end.
void Tpacket_list::print_list()
{//---
cout << " id = " << id << ", payload = " << payload << ", length " << payload.length() << " characters."<< endl ;
if ( next != NULL) // if there is a next item, get it to print itself.
next->print_list() ;
}

Эта функция является рекурсивной, но это обычно не хорошая идея. Это предел того, сколько элементов вы можете иметь в вашем списке и быть менее эффективными тогда итерационное решение.

//------ Find item in the list.
Tpacket_list* Tpacket_list::find( string target)
{//---
if ( payload == target)
return ( this) ; // return pointer to current object.

Вам не нужны скобки для возвращения заявления.

   if ( next == NULL)
return( NULL) ; // end of list, not match, return NULL.
//--- if got here there is a valid next.
return( next->find( target)) ;
}

//------ Tell of internal data.
int Tpacket_list::tell_id()
{ return( id) ;
}

string Tpacket_list::tell_payload()
{ return( payload) ;
}

//====== Start program here =================================================

int main(int argc, char *argv[]) // argc = number strings on command line.
// argv is array of strings each holding
// a parameter.

{//--- Initialize link list with command parameters.
cout << endl << " Form link list of command line parameters." << endl ;
list_head = NULL ;
int i = 1 ;
while ( i < argc)
{ new Tpacket_list(i, argv[i]) ;
i++ ;
}

Использовать цикл, а не цикл while здесь.

 //--- print out the list.
cout << " Print paramters from head of list the end of list." << endl ;
if ( list_head != NULL)
list_head->print_list() ;
else cout << " List is empty." << endl ;

//--- search for parameters
cout << " Find first 0 parameter (working last to first parameter, list head to tail)." << endl ;
Tpacket_list *found ;
if ( list_head != NULL)
{ found = list_head->find("0") ;

Я рекомендую не класть заявление на одной строке {1

       if ( found != NULL)
cout << " Found target on parameter # " << found->tell_id() << endl ;
else cout << " No \"0\" found." << endl ;
}

cout << endl ;
return(0) ;
}

2
ответ дан 29 сентября 2011 в 12:09 Источник Поделиться

Во-первых, в качестве оговорки, я не код на C/C++, поэтому я могу критиковать некоторые общие практики, которые считаются приемлемыми как это норма.

Тем не менее, несколько интересных вещей.

Комментарии

Комментарии должны быть использованы, чтобы описать, почему что-то делается. Сам код должен быть информативным, как это делать.

Tpacket_list *work_ptr ; // рабочий указатель для работы по список.

Например, эта переменная будет переименован, чтобы сделать его информативным? Возможно, *current_list_item. Возможно, что-то еще, что бы устранить необходимость для комментария.

Один Файл На Каждый Класс

Вообще говоря, вы хотите один файл на каждый класс. Я тоже считаю, что это обычная практика для программ C/C++, чтобы иметь один файл для заголовка и один для кода для этого заголовка. Может, что-то вроде Tpacket_list.ч и Tpacket_list.cpp, а также main.cpp.

Область

Почему ваши Tpacket_list переменные защищены? Почему они не частные?

Кроме того, list_head объявляется в главном, а затем в свободном доступе в глобальную переменную внутри класса Tpacket_list. Вместо этого, она считается лучшей практики для использования внедрения зависимостей. Передать эту переменную в класс, который требует доступ к нему.

Это сделает ваш код намного легче поддерживать в дальнейшем. Это открывает возможность модульного тестирования вашего класса.

Случайные Мысли


  • Вы можете просто вернуться пораньше, если нет списка?

  • Tpacket_list представляется как коллекции и предметы коллекции. Бы разделять эти понятия вне смысла? Если программа остается маленькой, я уверен, что это нормально. Если он растет, чтобы быть большим, это позволит получить значение.

  • Это стилистические, является ли или не вы используете фигурные скобки, содержимое если-бы то ни было высказываний, которые я хотел бы отметить, что я никогда не видел кого-то сделать ошибку с их на месте, но я видел ошибки там, где их не было.

2
ответ дан 29 сентября 2011 в 12:09 Источник Поделиться