Здравствуйте, GhostCoders, Вы писали:

GC>Данный код был написан моим сотрудником. Мне этот код не нравится, но он мне отвечает что я субъективен и его код неплох.
Плохое форматирование, проблемы с конвенциями именования.
Bad smells of code прямо по фаулеру: дубликаты, магические числа, длинные метода.
Очень плохо с архитектурой: нарушение SRP, не выделены классы которые прямо-таки напрашиваются на создание (вроде Point)Думаю, не помешало бы использовать пару структурных паттёрнов.
Ещё мне кажется, что человек не знает о назначении ASSERT.
Из плюсов, человек знает как про double и его проблемы.

Здравствуйте, Zhendos, Вы писали:

Z>т.к. что и куда нужно возвращать из контекста не ясно,
Z>то просто не ловим исключения, пусть они идут наверх
Ну ок, ок. Напрягает то, что сущностей X, Y, Z как-то не выделено.

Здравствуйте, _Dreamer, Вы писали:

_D>На мой взгляд, код ужасен.
Плюсую.
_D>Чтобы не быть субъективным, нужно принять стандарт оформления кода и встроить линтер на прекоммит.
Дико плюсую.
_D>Принять решение о написании тестов. Тогда никто не будет писать таких огромных классов.
Заплюсовываю!
_D>Использовать VCS, их куча на выбор, тогда не будет никаких закомментированных участков кода.
Там скорее всего есть, встречается такое, что в ветках комменты.
_D>Организовать код-ревью если не каждого коммита в master, то хотя бы важных фич.
Просто дико заплюсовываю. Нигде не видел системы codereview, там где работал, кроме одного проекта QT'a.
Когда был контрибьютером, просто офигел от того, как четко и правильно работают люди.
_D>Тогда Вы сможете аргументировать.
Согласен, поддерживаю, всецело отдаю свой голос за.

Здравствуйте, _Dreamer, Вы писали:

встроить линтер на прекоммит.

Так и называется, "Линтер"? В svn такое есть?

Здравствуйте, -n1l-, Вы писали:

N>>>

N>>>            do {    // pseudo-cycle to destruct tempRasters BEFORE doc-switching & CloseDocument()
N>>>                KompasIteratorHolder tempApprox(CreateIterator(ALL_OBJ, 0));    // GetViewReference(0)));
N>>>                if(!tempApprox) break;
N>>>                elem = MoveIterator(tempApprox, 'F');
N>>>            } while (0);
N>>>

N>>>вот это что за цикл на одно действие?

К>>Это прикольнейшая идиома. Это не цикл, а goto end. В роли goto используется break.

N>И типа if тут никак? Чем это хуже?

N>

N>KompasIteratorHolder tempApprox(CreateIterator(ALL_OBJ, 0));
N>if(tempApprox){
N> elem = MoveIterator(tempApprox, 'F');
N>}

N>

Не вникал в остальной код, но здесь разное время жизни tempApprox. В первом случае он уничтожается в конце блока, в твоем — живет себе дальше. Но твой вариант можно в еще один блок пихнуть.

Здравствуйте, Kernan, Вы писали:

K>Здравствуйте, Zhendos, Вы писали:

K>try-catch забыл. Я не особо смотрел на код, но вроде после do-while было что-то ещё.

Эти С++-ники такие С++-ники. Кто-то думает что с try{}catch будет производительнее?
В try{}catch произойдёт вызов __cxa_throw в котором вызовется __Unwind_RaiseException,
а там, мерзопакостный

_Unwind_RaiseException(struct _Unwind_Exception *exc)
{
..................................................
  /* Phase 1: Search.  Unwind the stack, calling the personality routine
     with the _UA_SEARCH_PHASE flag set.  Do not modify the stack yet.  */
  while (1)
    {
      _Unwind_FrameState fs;

      /* Set up fs to describe the FDE for the caller of cur_context.  The
     first time through the loop, that means __cxa_throw.  */
      code = uw_frame_state_for (&cur_context, &fs);

      if (code == _URC_END_OF_STACK)
    /* Hit end of stack with no handler found.  */
    return _URC_END_OF_STACK;

      if (code != _URC_NO_REASON)
    /* Some error encountered.  Usually the unwinder doesn't
       diagnose these and merely crashes.  */
    return _URC_FATAL_PHASE1_ERROR;

      /* Unwind successful.  Run the personality routine, if any.  */
      if (fs.personality)
    {
      code = (*fs.personality) (1, _UA_SEARCH_PHASE, exc->exception_class,
                    exc, &cur_context);
      if (code == _URC_HANDLER_FOUND)
        break;
      else if (code != _URC_CONTINUE_UNWIND)
        return _URC_FATAL_PHASE1_ERROR;
    }

      /* Update cur_context to describe the same frame as fs.  */
      uw_update_context (&cur_context, &fs);
    }

  /* Indicate to _Unwind_Resume and associated subroutines that this
     is not a forced unwind.  Further, note where we found a handler.  */
  exc->private_1 = 0;
  exc->private_2 = uw_identify_context (&cur_context);

  cur_context = this_context;
  code = _Unwind_RaiseException_Phase2 (exc, &cur_context);
  if (code != _URC_INSTALL_CONTEXT)
    return code;

  uw_install_context (&this_context, &cur_context);
}

Но кроме этого ещё куча работы и куча вызовов.
Может лучше сразу while(1){ ... break/return } ?

Здравствуйте, GhostCoders, Вы писали:

GC>Данный код был написан моим сотрудником. Мне этот код не нравится, но он мне отвечает что я субъективен и его код неплох.

Проводим, значит, код ревью

GC>

GC>// SewingDlg.h
GC>

У вас coding conventions вообще есть? где оформленный заголовок с именем файла, автором, датой создания, названием проекта?

GC>

GC>#ifdef max
GC>#undef max
GC>#endif

GC>#ifdef min
GC>#undef min
GC>#endif
GC>

Вот это нужно комментировать.

GC>

GC>class KompasObjectHolder
GC>

Стоп. Безотносительно того, что было уже сказано по поводу частокола конструкторов: Где комментарии? Где разъяснение, зачем это нужно. Где обоснование невозможности генеразизации класса (шаблоны)? И, чтобы уж добить, зачем оно вообще надо, если есть smart pointers?

GC>

GC>class KompasIteratorHolder : public KompasObjectHolder
GC>

Все то же самое плюс — почему в одном заголовочном файле объявлено более одного класса?

GC>

GC>class CSewingDlg : public CDialog       // диалоговое окно CSewingDlg
GC>

Офигенный комментарий. Спасибо, кэп.

GC>

GC>    CSewingDlg(CWnd* pParent = NULL);   // стандартный конструктор
GC>

Офигенный комментарий. Спасибо, кэп.

Дальнейшие поскипанные объявления методов тоже не проходят ревью по поводу отсутствия сколько-нибудь внятной документации. Об универсальном всемогуторе поговорим позже.

GC>

GC>reference CSewingDlg::Draw2DElement(reference element, bool closed)
GC>{
GC>    if (ReturnResult()) ResultNULL();
GC>    if (!ExistObj(element)) return 0;

GC>    switch (GetObjParam(element, 0, 0, ALLPARAM))
GC>    {
GC>    case CONTOUR_OBJ:
GC>        return DrawContour(element);        // ONLY APPROXIMATION NOW
GC>    case LINESEG_OBJ:
// Вызовы отрисовок для остальных типов фигур в ужасе поскипаны
GC>        break;
GC>    }
GC>    return 0;
GC>}
GC>

А это что вообще такое? Такое слово, как инкапсуляция, Вашему подчиненному вообще знакомо?
Это не C++ код, это страшное проявление C в его худшем виде, которое нынче еще поискать надо.

На этом в нашей компании код ревью бы прекратился за его дальнейшей бессмысленностью.

Здравствуйте, landerhigh, Вы писали:

GC>>

GC>>// SewingDlg.h
GC>>

L>У вас coding conventions вообще есть? где оформленный заголовок с именем файла, автором, датой создания, названием проекта?
для всего этого есть система контроля версий.
есть смысл разве что писать копирайт, и то не очень-то нужно.

GC>>

GC>>#ifdef max
GC>>#undef max
GC>>#endif

GC>>#ifdef min
GC>>#undef min
GC>>#endif
GC>>

L>Вот это нужно комментировать.
не нужно. те кто пишут под винду и так знают про эту проблему. другое дело что лучше NOMINMAX

GC>>

GC>>class KompasObjectHolder
GC>>

L>Стоп. Безотносительно того, что было уже сказано по поводу частокола конструкторов: Где комментарии? Где разъяснение, зачем это нужно.
кажется имя класса и так говорит о том зачем это нужно.

GC>>

GC>>class KompasIteratorHolder : public KompasObjectHolder
GC>>

L>Все то же самое плюс — почему в одном заголовочном файле объявлено более одного класса?
а почему бы и нет? особенно если классы используются вместе

Здравствуйте, smeeld, Вы писали:

S>Здравствуйте, Kernan, Вы писали:

K>>Здравствуйте, Zhendos, Вы писали:

K>>try-catch забыл. Я не особо смотрел на код, но вроде после do-while было что-то ещё.

S>Эти С++-ники такие С++-ники. Кто-то думает что с try{}catch будет производительнее?
S>В try{}catch произойдёт вызов __cxa_throw в котором вызовется __Unwind_RaiseException,
S>а там, мерзопакостный

Непонятно с чем вы спорите. В коде с "do {} while" обрабатываются ошибки выделения памяти,
т.е. вероятность выброса исключения очень мала,
код для бросания исключений и их обработки в частности ваш "__Unwind_RaiseException",
будет вызван только в случае нештатной ситуации, иными словами
в данном случае, с использованием современных C++ компиляторов,
измененный код будет работать не медленнее чем первоночальный,
а возможно и быстрее.

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

Здравствуйте, Zhendos, Вы писали:

Выше брезгливо морщились от выхода из последовательности операций, при
наступлении ошибки на каком-то этапе, с помощью do{ ...break..}while(1)
и говорилось про необходимость использовать это try{ throw x }catch();
А это как бы менее производительно, чем вариант с while{ break};
И вызов __Unwind_RaiseException произойдёт при всяком throw, ибо как есть стандарт.

Здравствуйте, Andrew.W Worobow, Вы писали:

AWW>мало коментариев.

В хорошем коде комментариев быть не должно.

Здравствуйте, Abyx, Вы писали:


L>>У вас coding conventions вообще есть? где оформленный заголовок с именем файла, автором, датой создания, названием проекта?
A>для всего этого есть система контроля версий.

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

A>есть смысл разве что писать копирайт, и то не очень-то нужно.

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

GC>>>

GC>>>#ifdef max
GC>>>#undef max

GC>>>

L>>Вот это нужно комментировать.
A>не нужно. те кто пишут под винду и так знают про эту проблему. другое дело что лучше NOMINMAX

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

GC>>>

GC>>>class KompasObjectHolder
GC>>>

L>>Стоп. Безотносительно того, что было уже сказано по поводу частокола конструкторов: Где комментарии? Где разъяснение, зачем это нужно.
A>кажется имя класса и так говорит о том зачем это нужно.

Комментарий должен быть. Он должен обосновывать необходимость этого холдера, в т.ч. содержать объяснение, почему аффтар не стал использовать умный указатель вместо этого, а также рассказывать, с какого такого бодуна объект вдруг стал называться ссылкой и зачем ему может понадобиться внешний удалятор. Любое инженерное решение должно быть обосновано. У меня сразу возникает вопрос — зачем нужно убивать объект, который является членом класса, неким внешним функтором. Если бы автор потрудился это описать в комментарии, он бы примерно на полпути, скорее всего, догадался бы, что инкапсуляция — не просто красивое слово, которое непонятно как правильно транслителировать на русском языке, а весьма правильный подход к проектированию подобных классов. Ну или бы четко описал, почему это должно быть сделано именно так, а не иначе.

GC>>>

GC>>>class KompasIteratorHolder : public KompasObjectHolder
GC>>>

L>>Все то же самое плюс — почему в одном заголовочном файле объявлено более одного класса?
A>а почему бы и нет? особенно если классы используются вместе

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

Здравствуйте, GhostCoders, Вы писали:

GC>Данный код был написан моим сотрудником. Мне этот код не нравится, но он мне отвечает что я субъективен и его код неплох.

Не стоит тащить на форум огромный код для его разбора. Достаточно небольшого фрагмента.

Разбирать код по принципу "я евангелист и этот код не вписывается в моё евангелие" не стоит.

Если код делается в команде, то должны быть выработаны командные требования по различным аспектам, включая оформление кода. Если этого сделано не было, то жаловаться на плохое оформление не стоит.

Прежде чем давать оценку, стоит честно ответить на вопросы: 1) этот код решает проставленную задачу?, 2) есть ли в нем ошибки?, 2) не жрет ли он неоправданно много ресурсов?

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

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

/*-----------------------------------------------------------------------------------*/

class KompasObjectHolder
{
protected:
    reference m_reference;
public:
    KompasObjectHolder() : m_reference(0) {}
    KompasObjectHolder(reference object_reference) : m_reference(object_reference) {}
    KompasObjectHolder(KompasObjectHolder& prev_holder) : m_reference(prev_holder.m_reference) { prev_holder.m_reference = 0; }
    KompasObjectHolder& operator =(KompasObjectHolder& prev_holder) {
        if (!m_reference) m_reference = prev_holder.m_reference, prev_holder.m_reference = 0; return *this; }
    ~KompasObjectHolder()
    {
        if (m_reference) DeleteObj(m_reference);
        m_reference = 0;
    }
    operator reference() { return m_reference; }
    operator bool() { return m_reference != 0; }
};

Я пишу вот так.

template <class T> 
T Replace_null(T &t) { T ret(t); t=T(); return ret; }

/* class KompasObjectHolder */

class KompasObjectHolder // Этот класс лучше превратить в шаблон 
 {
  protected:

   reference ptr; // лучше писать явно указатель на объект  KompasObject *

  public:

   // constructors

   KompasObjectHolder() : ptr(0) {}

   KompasObjectHolder(reference ptr_) : ptr(ptr_) {}

   KompasObjectHolder(KompasObjectHolder &obj) : ptr(obj.ptr) { obj.ptr=0; } 

   KompasObjectHolder(KompasObjectHolder &obj) : ptr(Replace_null(obj.ptr)) {} // Здесь следует использовать такую функцию

   KompasObjectHolder & operator = (KompasObjectHolder &obj) 
    {
     if( !ptr ) ptr = obj.ptr, obj.ptr = 0; 

     if( !ptr ) ptr=Replace_null(obj.ptr); // И здесь тоже
                                           // Кстати говоря, эта логика поведения мне не совсем понятна   

     return *this; // не экономь на строчках! 
    }

   ~KompasObjectHolder()
    {
     if( ptr ) DeleteObj(ptr); // Нормальная функция DeleteObj должна принимать нулевой указатель

     ptr=0; // Это не нужно
    }

   // methods

   operator reference() const { return ptr; } // здесь нужен const

   operator bool() const { return ptr!=0; } // и здесь тоже, более того, этот оператор не нужен и даже вреден, его функции будет выполнять первый оператор, будут проблемы с разрешением перегрузки
 };

Вот этот фрагмент порадовал.

        reference c_group = NewGroup(1); 
        EndGroup();
        SelectGroup(c_group, 2, 0,0,0,0);

        reference n_group = 0;
        if (ExistGroupObj(c_group))
        {
            KompasIteratorHolder contour_iterator(CreateIterator(ALL_OBJ, c_group));

            reference cur_obj_reference = MoveIterator(contour_iterator, 'F'); 
            while (cur_obj_reference)
            {
                int type = GetObjParam(cur_obj_reference, 0, 0, ALLPARAM);
                perimeter += ksGetCurvePerimeter (cur_obj_reference, ST_MIX_MM);
                cur_obj_reference = MoveIterator(contour_iterator, 'N');
            }
        }
        if (c_group) DeleteObj(c_group);

Если уж писать object holder, то следует их использовать. И методы им давать содержательные.

GroupHolder c_group(NewGroup(1));

....

reference cur_obj_reference = contour_iterator.move('F') ;

....

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

Копай Нео, копай -- летать научишься. © Matrix. Парадоксы

Здравствуйте, GhostCoders, Вы писали:

GC>Здравствуйте, bnk, Вы писали:

bnk>>Код конечно не торт, но подобный ппц вполне ожидаем в продукте с N-летней историей.
GC> Этот код был написан в течении последних двух месяцев практически с нуля. Так сказать, свежачок....

Почерк динозавра просматривается
IMHO, ничего особо страшного, если не планируется этот код кому-либо отдавать или долго развивать-поддерживать, т.е. "выстрелил и забыл".

Здравствуйте, Шахтер, Вы писали:

Ш>Вообщем, начинать надо с проработки базовых вещей. Проект сделан неряшливо. И я думаю, что вина здесь не только автора данного кода.

А что Вы скажете про код из опенсорсов?

Здравствуйте, -n1l-, Вы писали:

N>Здравствуйте, _Dreamer, Вы писали:

N>встроить линтер на прекоммит.

N>Так и называется, "Линтер"? В svn такое есть?

ну если погуглить "cpp lint", то один из первых находится Cpplint + еще что-то.
и да, оно легко прикручивается к svn, много всего настраивается (вкл\выкл) и в основном заточено на google style, который не всем нравится и мне в частности тоже, поэтому кучу фич нужно было выключать, и да, как и описано, довольно часто возникают false positive и приходится затыкать глупыми комментами // NOLINT.

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

Здравствуйте, GhostCoders, Вы писали:

я взглянул только на заголовочный файл и мои глаза закровоточили. Работа с графикой должна быть отделена от работы с юаем, т.е. должна идти работа с абстрактным графическим контекстом — хотя бы обёрткой над hbitmap, всё что должен делать диалог — обрабатывать ввод пользователя и звать методы этого контекста, в onPaint блитить к себе.

Здравствуйте, GhostCoders, Вы писали:
Меня очень смущают "наполнители"

GC>reference CSewingDlg::DrawEllipse(reference element)
GC>{
GC>    EllipseParam epEllipseParam;
GC>    if (GetObjParam(element, &epEllipseParam, sizeof(EllipseParam), ALLPARAM))
GC>    {
 GC>        double x = epEllipseParam.xc,
GC>               y = epEllipseParam.yc;
GC>        double angle = epEllipseParam.ang;
GC>        double a = epEllipseParam.a;
GC>        double b = epEllipseParam.b;

GC>        const double perimeter =  ksGetCurvePerimeter (element, ST_MIX_MM); 

GC>        epEllipseParam.style = 1;

GC>        return ksEllipse(&epEllipseParam);
GC>    }
GC>    return 0;
GC>}

Таких "наполнителей" в коде дохрена.

А также куча сгенерированного кода. Свой и генерируемый код — нужно разделять, ибо поддержка IDE может перестанет работать. Я бы на вашем месте генерируемый код тут не постил.

Да и нотации типа m_, в новом коде я не приемлю, уже есть нормальные IDE.

Здравствуйте, vpchelko, Вы писали:

V>Да и нотации типа m_, в новом коде я не приемлю, уже есть нормальные IDE.

помимо нормальных ide код может оказаться в блокноте, вьювере, письме, распечатан, передан по скайпу, запосчен на форум и стековерфлоу. Чем m_ помешал-то?

Здравствуйте, vpchelko, Вы писали:

V>Здравствуйте, GhostCoders, Вы писали:

V>Меня очень смущают "наполнители"
V>

GC>>        y = epEllipseParam.yc;
GC>>        double angle = epEllipseParam.ang;
GC>>        double a = epEllipseParam.a;
GC>>        double b = epEllipseParam.b;
V>

V>Таких "наполнителей" в коде дохрена.

Наверное чтобы в дебаггере было удобно смотреть (возможно что в вотч эти самые x, y, a, b добавлены)
Хотя да, общая неряшливось присутствует. Явно у чувака с чувством прекрасного напряженка.

V>Да и нотации типа m_, в новом коде я не приемлю, уже есть нормальные IDE.

"m_" ничем не хуже "_" разве что на один символ длиннее, к тому же там mfc
Про поддержку IDE — она позволяет забыть про m_lpstr (венгерскую нотацию), но не про члены класса.