Ivanova475 / cplusplus-tasks

0 stars 0 forks source link

homework2-forest #2

Closed gitluup closed 5 years ago

gitluup commented 5 years ago

https://github.com/Ivanova475/cplusplus-tasks/blob/36f833253d8e722289ee29567a13887e809f2ca1/Forest/Forest/matrix.h#L25 Это должно быть в приватной секции

https://github.com/Ivanova475/cplusplus-tasks/blob/36f833253d8e722289ee29567a13887e809f2ca1/Forest/Forest/matrix.h#L18 Этот конструктор можно убрать, так как создать пустуб матрицу можно будет и с помощью оставшегося конструктора, передав ему 2 параметра: 0, 0

https://github.com/Ivanova475/cplusplus-tasks/blob/36f833253d8e722289ee29567a13887e809f2ca1/Forest/Forest/forest.h#L10 Лучше как-то обозначить, что этот параметр имеет отношение ко времени. Например, добавить _time в конце имени

https://github.com/Ivanova475/cplusplus-tasks/blob/36f833253d8e722289ee29567a13887e809f2ca1/Forest/Forest/forest.h#L24 Этот конструктор лучше убрать по аналогии с Matrix

https://github.com/Ivanova475/cplusplus-tasks/blob/36f833253d8e722289ee29567a13887e809f2ca1/Forest/Forest/forest.h#L29 Этот метод не предполагается вызывать извне класса, поэтому он должен быть приватным (да, методы тоже можно помещать в приватную секцию)

https://github.com/Ivanova475/cplusplus-tasks/blob/36f833253d8e722289ee29567a13887e809f2ca1/Forest/Forest/forest.h#L9 Создавать переменные в заголовочном файле - очень плохая идея. Так делать нельзя. Могут быть те же самые проблемы, которые возникают при написании функции в заголовочных файлах (помнишь про то, что заголовочники инклюдятся во все файлы подряд, одна функция может попасть в несколько объектных файлов и произойдёт ошибка на этапе линковки?).

https://github.com/Ivanova475/cplusplus-tasks/blob/36f833253d8e722289ee29567a13887e809f2ca1/Forest/Forest/forest.h#L13 Как-то лучше обозначить, что эти переменные относятся именно к состоянию дерева. Например, добавить state в конец (таким образом, не grow, а grow_state). Я вот не сразу понял, что они означают, а про параметры типа fire_time я сразу понял.

https://github.com/Ivanova475/cplusplus-tasks/blob/36f833253d8e722289ee29567a13887e809f2ca1/Forest/Forest/forest.cpp#L13 Очень, очень, очень много кода и копипаста. Сразу видно, что здесь написано как-то не так по двум причинам: 1) много копипаста 2) метод занимает больше, чем 1-1.5 экрана монитора (1.5 - это уже очень много) Посмотри, как это у меня было сделано (в функции ProcessForest): https://github.com/codepictor/various-tasks/blob/master/2/linux/forest/forest.c

https://github.com/Ivanova475/cplusplus-tasks/blob/36f833253d8e722289ee29567a13887e809f2ca1/Forest/Forest/forest.cpp#L162 В методе ForestUpdate мною замечена непоследовательность. В каких-то местах ты пишешь else на отдельной строке, а в других - на той же строке, на которой находится закрывающая фигурная скобка.

С производительностью у меня всё норм.

gitluup commented 5 years ago

https://github.com/Ivanova475/cplusplus-tasks/blob/f8ca6240761558c038fb7ab7ab61076dc87b9bbf/Forest/Forest/forest.cpp#L39 Здесь у тебя else на отдельной строке, а во всех остальных случаях в этом файле - на одной строке с закрывающей фигурной скобкой

https://github.com/Ivanova475/cplusplus-tasks/blob/f8ca6240761558c038fb7ab7ab61076dc87b9bbf/Forest/Forest/main.cpp#L18 В этом месте хорошо видно, что метод должен называться не ForestUpdate, а просто Update. И так везде поняно, что он применяется к Forest

https://github.com/Ivanova475/cplusplus-tasks/blob/f8ca6240761558c038fb7ab7ab61076dc87b9bbf/Forest/Forest/matrix.cpp#L39 Имеет смысл принимать третий аргумент по константной ссылке

gitluup commented 5 years ago

OK