Comments 28
покритикуем реализацию?
не только ООП, но раз уж Вы упомянули «дублирование», «рефакторинг», «код с душком», etc, то явно академичность статьи выходит чуть дальше за рамки ООП
а я как раз прочёл Р. Мартина, Фаулера, GoF (а ща читаю Физерса)
0. Ctrl +D в Lazarus'е форматирует код (неформатированный код — отстой; правильные привычки нужно формировать с самого начала)
1.
Метод стирания следует назвать
2. При этом стирание происходит непосредственно перед удалением объекта, поэтому место ему в деструкторе
2.1 Для стирания нужен TCanvas, поэтому передаём TImage в конструкторе (тем более, что нам нужны его ширина и высота, которые имеют мало смысла, если рисовать мы будем на каком-то другом канвасе), и запоминаем его (использование интерфейса «дружеского класса» не нарушает инкапсуляцию)
3. Управление списком созданных объектов нужно отдать классу TPetals/TPetalList, а не рулить глобальным индексом.
3.1 При этом у Вас утечка памяти в
3.2. Он же (TPetals) рулит созданием нового экземпляра
при этом
4. Кроме того, в строке
происходит SEGV, если Timer сработает быстрее, чем выполнится FormCreate (у меня на Linux)
поэтому надо выключить таймер в Design time
и включать его во время исполнения
с учётом вышесказанного обработчик таймера будет выглядеть примерно так
не только ООП, но раз уж Вы упомянули «дублирование», «рефакторинг», «код с душком», etc, то явно академичность статьи выходит чуть дальше за рамки ООП
а я как раз прочёл Р. Мартина, Фаулера, GoF (а ща читаю Физерса)
0. Ctrl +D в Lazarus'е форматирует код (неформатированный код — отстой; правильные привычки нужно формировать с самого начала)
1.
procedure Draw (Canvas: TCanvas; Erase: boolean = FALSE);
При вызове метода из программы без использования второго параметра объект рисуется, а при использовании параметра Erase — объект стирается:
Метод стирания следует назвать
Erase, и в нём уже вызывать «рисование с параметром»2. При этом стирание происходит непосредственно перед удалением объекта, поэтому место ему в деструкторе
2.1 Для стирания нужен TCanvas, поэтому передаём TImage в конструкторе (тем более, что нам нужны его ширина и высота, которые имеют мало смысла, если рисовать мы будем на каком-то другом канвасе), и запоминаем его (использование интерфейса «дружеского класса» не нарушает инкапсуляцию)
3. Управление списком созданных объектов нужно отдать классу TPetals/TPetalList, а не рулить глобальным индексом.
3.1 При этом у Вас утечка памяти в
тут не удаляются уже созданные объекты (да, знаю, при завершении программы, в общем-то пофиг, но «привычки...»)procedure TForm1.FormClose(Sender: TObject; var CloseAction: TCloseAction); begin Petals := nil; end;
3.2. Он же (TPetals) рулит созданием нового экземпляра
при этом
может выглядеть как (удаляем дублирование)if Random(2) = 1 then Petals[CurPetal] := TPetal.Create(img.Width, img.Height) else Petals[CurPetal] := TOverlappedPetal.Create(img.Width, img.Height);
type
TCustomPetalClass = class of TCustomPetal;
...
var
LPetalClassToCreate: TCustomPetalClass;
...
if Random(2) = 1 then
LPetalClassToCreate := TPetal
else
LPetalClassToCreate := TOverlappedPetal
Petals[CurPetal] := LPetalClassToCreate.Create(img.Width, img.Height);
4. Кроме того, в строке
if Assigned(Petals[CurPetal]) then
происходит SEGV, если Timer сработает быстрее, чем выполнится FormCreate (у меня на Linux)
поэтому надо выключить таймер в Design time
и включать его во время исполнения
procedure TForm1.FormCreate(Sender: TObject);
begin
PetalC := 70;
SetLength(Petals, PetalC);
CurPetal := 0;
timer1.Enabled := True;
end;
с учётом вышесказанного обработчик таймера будет выглядеть примерно так
type
TForm1 =
private
FPetals: TPetals;
...
end;
...
procedure TForm1.Timer1Timer(Sender: TObject);
begin
FPetals.DrawNext;
img.Canvas.TextOut(10, 10, IntToStr(FPetals.Count) + ' ');
end;
Здравствуйте.
Большое спасибо за комментарии и то, что потратили время на такой дельный разбор. Я на самом деле этот учебный (для себя) проект делал пару лет назад, и тогда, если бы я дорабатывал его до идеала, я бы тоже решил запрятать полностью всю работу в классы. Потом забросил.
Теперь вот решил допилить, учел практически полностью Ваши замечания, новый файл в ветке lesson4.
Я только не понял в чем разница — передавать свойство Canvas или весь объект Image, я пока оставил Canvas. И отдельный метод для стирания писать не стал, т.к. считаю это избыточным, ведь нужна всего одна строчка, а ее я запрятал в аналог деструктора (метод kill), то есть в конечном итоге, как и предлагалось, обработчик таймера делает лишь DrawNext ))
Большое спасибо за комментарии и то, что потратили время на такой дельный разбор. Я на самом деле этот учебный (для себя) проект делал пару лет назад, и тогда, если бы я дорабатывал его до идеала, я бы тоже решил запрятать полностью всю работу в классы. Потом забросил.
Теперь вот решил допилить, учел практически полностью Ваши замечания, новый файл в ветке lesson4.
Я только не понял в чем разница — передавать свойство Canvas или весь объект Image, я пока оставил Canvas. И отдельный метод для стирания писать не стал, т.к. считаю это избыточным, ведь нужна всего одна строчка, а ее я запрятал в аналог деструктора (метод kill), то есть в конечном итоге, как и предлагалось, обработчик таймера делает лишь DrawNext ))
сорре за придирчивость )) но чисто для полноты картины:
и всё сломали )) код не компилируется (Unit1 not found) (заменили main на какой-то Unit1)
если пофиксить это, то при закрытии формы возникает AV/SEGV
ИМХО, тут речь не о Вас, пишущем код, а о том, что считает тот, кто читает ваш код ;) кстати, это можете быть и Вы (через полгода-год-два...)
Один умный человек сказал: «вы всегда работаете в команде: „вы“ и „вы через две недели“».
вот именно — «запрятал» )))
а ведь код должен быть понятным
зачем? если есть «стандарный способ»
который вызывается при уничтожении объекта
и опять глобальная переменная (P) ))) а не поле класса TForm1
…
прошу прощения, если утомил, но дьявол, как известно, в мелочах, а настоящим программистам важны мелочи
Теперь вот решил допилить, учел практически полностью Ваши замечания
и всё сломали )) код не компилируется (Unit1 not found) (заменили main на какой-то Unit1)
если пофиксить это, то при закрытии формы возникает AV/SEGV
И отдельный метод для стирания писать не стал, т.к. считаю это избыточным,
ИМХО, тут речь не о Вас, пишущем код, а о том, что считает тот, кто читает ваш код ;) кстати, это можете быть и Вы (через полгода-год-два...)
Один умный человек сказал: «вы всегда работаете в команде: „вы“ и „вы через две недели“».
ведь нужна всего одна строчка, а ее я запрятал
вот именно — «запрятал» )))
а ведь код должен быть понятным
в аналог деструктора (метод kill),
зачем? если есть «стандарный способ»
destructor Destroy; override;
который вызывается при уничтожении объекта
и опять глобальная переменная (P) ))) а не поле класса TForm1
…
прошу прощения, если утомил, но дьявол, как известно, в мелочах, а настоящим программистам важны мелочи
Всё поправил)
Интересно, а у меня одного лазарус в убунте ведет себя странно? То окна не найдешь, то новый проект сохраняет в старом (вот поэтому и вылез unit1).
Ошибка при закрытии выпала, видимо, от того, что пока удаляются объекты, успевал сработать еще раз таймер. Выключил его при закрытии.
Теперь затираемые розы не «портят» перекрытые.
А насчет деструктора — в него нельзя передать параметр, поэтому сделал метод kill.
Интересно, а у меня одного лазарус в убунте ведет себя странно? То окна не найдешь, то новый проект сохраняет в старом (вот поэтому и вылез unit1).
Ошибка при закрытии выпала, видимо, от того, что пока удаляются объекты, успевал сработать еще раз таймер. Выключил его при закрытии.
Теперь затираемые розы не «портят» перекрытые.
А насчет деструктора — в него нельзя передать параметр, поэтому сделал метод kill.
:)
Теперь без косяков
потому я и говорил
но говоря про TPetals как объект, управляющий списком объектов TPetals, я имел в виду
что-то вроде
добавление нового «лепестка»
это удобнее, чем манипуляция массивом с проверками, типа
и к тому же
а меньше кода = чище код = понятней код = проще сопровождение
Теперь без косяков
А насчет деструктора — в него нельзя передать параметр, поэтому сделал метод kill.
потому я и говорил
2.1 Для стирания нужен TCanvas, поэтому передаём TImage в конструкторе (тем более, что нам нужны его ширина и высота, которые имеют мало смысла, если рисовать мы будем на каком-то другом канвасе), и запоминаем его (использование интерфейса «дружеского класса» не нарушает инкапсуляцию)
но говоря про TPetals как объект, управляющий списком объектов TPetals, я имел в виду
что-то вроде
TPetals = class(TObjectList)
добавление нового «лепестка»
P.Add(...)это удобнее, чем манипуляция массивом с проверками, типа
if Assigned(fPetals[Count]) then
и к тому же
TPetals.Free, fSize, fCount становятся не нужны вовсеа меньше кода = чище код = понятней код = проще сопровождение
Ну теперь, кажется, всё) lesson4-1
Таки-пришлось TImage передавать.
Таки-пришлось TImage передавать.
На видео видно, как розы постоянно портятся, цвета близкие к чёрному надо бы из рандома исключить.
Здорово! Наверно такого мне не хватало когда изучали в институте ООП с delphi.
А существует что-нибудь подобное для других языков?
А существует что-нибудь подобное для других языков?
Есть видеокурс по C#, там на основе ООП делается простенькая игра «змейка»; я после него и вспомнил про этот свой проект.
Интересные примеры на C# и HAML здесь, а также понятное толкование сути наследования в классах
книжка
еще книжка
Microsoft Windows Presentation Foundation: базовый курс (много примеров графики в проекте клона блокнота, в дальнейшем парсера HAML)
TL;DR. Осторожно, красная капсула далее.
Это не ООП. И вся статья про то, как мимикрировать под ООП, не задействовав сути идеи и ее преимуществ.
> мы наглядно и полноценно продемонстрировали все принципы объектно-ориентированного программирования… инкапсуляция… наследование… полиморфизм.
Нет, не все. Нет самого главного — абстракции.
> (в комментах) Я только не понял в чем разница — передавать свойство Canvas или весь объект Image
В этом ВСЯ разница. В данном случае — НАДО передавать Canvas, потому что Canvas в Delphi — это более универсальная сущность, чем Image. Передадите Image — сможете рисовать только на Image, передадите Canvas — на Image и на других типах, имеющих Canvas.
И это все равно будет не ООП. Чтобы сделать это ООП (именно в этом месте) — создайте интерфейс с методами рисования и передавайте его в то, что хочет отрисоваться (фигуру) и реализуйте в том, на чем хотите отрисоваться — в данном случае в Image на базе методов того же Canvas'а. Читайте про Dependency Inversion Principle.
> инкапсуляция
Image.Canvas — вот вы и разрушили всю инкапсуляцию. Читайте про что-нибудь вроде «avoid getters», в т.ч. у упомянутых здесь Мартина и компании. Something.Anything — это намного хуже, чем геттеры.
> полиморфизм представлен, например, так: есть базовый абстрактный класс Кошачьи, от которого порождены многочисленные наследники
Про наследование лучше просто забудьте. Ищите по кивордам «composition over inheritance».
P.S. Настоятельно рекомендую ознакомиться хотя бы со страницами с результатами поиска по ключевым словам выше (например avoid getters). Вы будете удивлены тем, насколько далеко то, что предлагает стандартный дельфийский подход, от ООП.
Это не ООП. И вся статья про то, как мимикрировать под ООП, не задействовав сути идеи и ее преимуществ.
> мы наглядно и полноценно продемонстрировали все принципы объектно-ориентированного программирования… инкапсуляция… наследование… полиморфизм.
Нет, не все. Нет самого главного — абстракции.
> (в комментах) Я только не понял в чем разница — передавать свойство Canvas или весь объект Image
В этом ВСЯ разница. В данном случае — НАДО передавать Canvas, потому что Canvas в Delphi — это более универсальная сущность, чем Image. Передадите Image — сможете рисовать только на Image, передадите Canvas — на Image и на других типах, имеющих Canvas.
И это все равно будет не ООП. Чтобы сделать это ООП (именно в этом месте) — создайте интерфейс с методами рисования и передавайте его в то, что хочет отрисоваться (фигуру) и реализуйте в том, на чем хотите отрисоваться — в данном случае в Image на базе методов того же Canvas'а. Читайте про Dependency Inversion Principle.
> инкапсуляция
Image.Canvas — вот вы и разрушили всю инкапсуляцию. Читайте про что-нибудь вроде «avoid getters», в т.ч. у упомянутых здесь Мартина и компании. Something.Anything — это намного хуже, чем геттеры.
> полиморфизм представлен, например, так: есть базовый абстрактный класс Кошачьи, от которого порождены многочисленные наследники
Про наследование лучше просто забудьте. Ищите по кивордам «composition over inheritance».
P.S. Настоятельно рекомендую ознакомиться хотя бы со страницами с результатами поиска по ключевым словам выше (например avoid getters). Вы будете удивлены тем, насколько далеко то, что предлагает стандартный дельфийский подход, от ООП.
В данном случае — НАДО передавать Canvas, потому что Canvas в Delphi — это более универсальная сущность, чем Image. Передадите Image — сможете рисовать только на Image, передадите Canvas — на Image и на других типах, имеющих Canvas.
а Вы правы, да. Это я посоветовал не сильно подумав (сам ещё только меняю привычки проектирования классов)
ну, а интерфейсы я не стал «трогать»…
Спасибо. Я как раз продолжаю сам изучать теорию ооп.
Но в данном конкретном примере, для начинающих, так сказать, не будет ли это избыточным усложнением? То есть, когда в школе учат формулам скорости, веса, ведь никто сходу не говорит, что нужно учитывать теорию относительности взамен простых вычислений s=vt.
Но в данном конкретном примере, для начинающих, так сказать, не будет ли это избыточным усложнением? То есть, когда в школе учат формулам скорости, веса, ведь никто сходу не говорит, что нужно учитывать теорию относительности взамен простых вычислений s=vt.
Zapped,velvetcat, набросал заготовку с «интерфейсом» рисовальщика. На мой взгляд, чрезмерно усложнено по сравнению с предыдущим вариантом.
Ну или я не так понял.
Ну или я не так понял.
немножко облегчил код, добавил метод Erase в интерфейс рисовальщика, чтоб и затереть и сразу удалить объект
эээ… боюсь, вы не поняли интерфейсы
почитайте, например
а следуя DIP, код мог бы выглядеть примерно так
N.B.:
модуль
также
они связаны опосредовано через
таким образом мы в любой момент можем заменить классы TPetal и TPetals другими, и нам не надо будет менять
так же мы можем протестировать в unit-тестах классы TPetals/TPetal, не используя TForm, а просто «подсунув» stub/mock интерфейса IPetalDrawer.
почитайте, например
а следуя DIP, код мог бы выглядеть примерно так
N.B.:
модуль
main ничего не знает про (= не зависит от) petalclassтакже
petalclass и ничего не знает про mainони связаны опосредовано через
petalintfтаким образом мы в любой момент можем заменить классы TPetal и TPetals другими, и нам не надо будет менять
mainтак же мы можем протестировать в unit-тестах классы TPetals/TPetal, не используя TForm, а просто «подсунув» stub/mock интерфейса IPetalDrawer.
Большое спасибо за труд)
Как раз сижу и вникаю, как в книги по проектированию, так и в ваш код. Применил его на своем другом учебном проекте. Всё же, возвращаясь к методу обучения основам ООП… Ну вот, чтобы вникнуть в ваш код, мне пришлось его повторить, чтоб понять кто кого куда и зачем). И это имея уже представление о том, что делают те или иные классы, судя по названиям. А в тот момент, когда я только начинал изучать ООП, мне трудно было даже понять реализацию полиморфизма и смысл этого. Понял, только на ходу, делая вот этот проект с рисульками. Вряд ли на первых порах обучения стоит сразу же в простейшие проекты вставлять сложные шаблоны проектирования. Это и усложняет их, и делает трудными для чтения и понимания.
Как раз сижу и вникаю, как в книги по проектированию, так и в ваш код. Применил его на своем другом учебном проекте. Всё же, возвращаясь к методу обучения основам ООП… Ну вот, чтобы вникнуть в ваш код, мне пришлось его повторить, чтоб понять кто кого куда и зачем). И это имея уже представление о том, что делают те или иные классы, судя по названиям. А в тот момент, когда я только начинал изучать ООП, мне трудно было даже понять реализацию полиморфизма и смысл этого. Понял, только на ходу, делая вот этот проект с рисульками. Вряд ли на первых порах обучения стоит сразу же в простейшие проекты вставлять сложные шаблоны проектирования. Это и усложняет их, и делает трудными для чтения и понимания.
Sign up to leave a comment.
Безболезненная прививка объектного мышления