Skip to content

Review1#1

Open
azbyx wants to merge 2 commits into
masterfrom
review1
Open

Review1#1
azbyx wants to merge 2 commits into
masterfrom
review1

Conversation

@azbyx
Copy link
Copy Markdown
Owner

@azbyx azbyx commented Jan 24, 2021

No description provided.

Comment thread sources/model.cpp
keeperObjects.push_back(factoryObjects.makeObjects[typePrimitive::CIRCLE]->create(location, radius));
}

void Model::removeLast() {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

круто, что можно удалить последний созданный элемент. а если нужно удалить не последний элемент, как быть?

Comment thread sources/view.cpp
#include "view.h"
#include <stdexcept>

View::View(IModelSptr model, IPainterSptr painter) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread sources/controller.cpp
void Controller::commandRemoveShape() {

controllerModel->removeLast();
controllerModel->notifyUpdate();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

возможно notifyUpdate лучше делать внутри модели, потому что контроллер не должен знать когда необходимо обновлять представление

Copy link
Copy Markdown

@ruscoder2013 ruscoder2013 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

В целом всё написано хорошо и понятно

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.

3 participants