Skip to content

Revert "review2 created"#2

Open
senyacherenkov wants to merge 1 commit intomasterfrom
review2
Open

Revert "review2 created"#2
senyacherenkov wants to merge 1 commit intomasterfrom
review2

Conversation

@senyacherenkov
Copy link
Owner

ready for review

This reverts commit 2006598.
@mkvdv
Copy link

mkvdv commented Oct 18, 2018

Добрый день!
Как мне кажется, Вы не совсем понимаете MVC, либо очень сильно не доделали работу. Посмотрите в книжке Head First, там, как мне кажется, весьма хорошо он описывается.
Также, по коду мне показалось, что Вы не совсем хорошо понимаете динамический полиморфизм, как и зачем он используется.

Замечания по пунктам:

  • CMakeLists.txt нет в коммите, поэтому напишу сюда: проект нельзя собрать, не установив спец. переменную. Добавьте if($ENV{TRAVIS_BUILD_NUMBER})
  • У Вас все сущности лежат в глобальном пространстве имен. Так выше возможность нарваться на конфликт имен. Используйте namespace-s
  • Постарайтесь сгруппировать файлы проекте - отдельно исходники, отдельно тесты, отдельно README.md и Doxyfile. Иначе, когда всё в кашу, смотреть бывает не очень удобно =)
  • Во многих местах у Вас определение класс в хидер файле. В данном простом случае компилятор не ругается, но не надо так делать. Чуть сложнее будет код - и получите redefinition от компилятора, и придется выносить определение в .cpp
  • Во многих местах у Вас лишние копирования - передаете параметры по значению, до дальше не делаете move. Любо мувьте, либо передавайте по const ссылке.
  • Про полиморфизм опять: предполагается, что модель, вью и контроллер о друг друге знают только интерфейс, но не реализацию - у Вас это не используется. Те места, где у Вас используется наследование - не ясно, зачем оно там. Ну, то есть понятно, зачем вообще его использовать в подобных паттернах, но у Вас полиморфизм не используется по назначению.

Можно много говорить о деталях, но пока у Вас не до конца доработана архитектура приложения, смысла спускаться туда я не вижу (хотя кое-что прокомментировал в комментариях к коду).
Если есть вопросы/замечания/предложения - пишите. 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants