Форум: "Начинающим";
Текущий архив: 2009.10.11;
Скачать: [xml.tar.bz2];
ВнизВложенный цикл Найти похожие ветки
← →
Дмитрий Белькевич (2009-08-04 15:41) [40]>Это что, goto неявно приведет к таким проверкам? И будет медленнее?
Я что-то не врубился.
Нет, это приведенный в [4] код, с двумя лишними проверками, будет медленнее.
← →
Kolan © (2009-08-04 16:46) [41]Про удобство модификации кода из [28] можно поспорить, там есть дублирующийся код, правда чисто теоретически, он слишком маленький, чтобы создать проблемы, поэтому спорить не буду.
← →
palva © (2009-08-04 20:43) [42]Покритикуйте вот такой код.
Нужно нарисовать кристалл на матрице. Если на матрице уже есть центр кристаллизации (звездочка), то использовать его, если нет, выбрать центр кристалла случайным образом.
{$APPTYPE CONSOLE}
type
TMatrix = array[0..4,0..4] of Char;
var
i, j: Integer;
a: TMatrix = (
(".", ".", ".", ".", "."),
(".", ".", ".", ".", "."),
(".", ".", ".", "*", "."),
(".", ".", ".", ".", "."),
(".", ".", ".", ".", ".")
);
procedure Crystal(var a: TMatrix);
var i, j, ii, jj: Integer;
label LBreak;
begin
for ii := 1 to 5 do for jj := 1 to 5 do
if a[ii, jj] = "*" then begin
i := ii; j := jj;
goto LBreak;
end;
Randomize;
i := Random(5); j := Random(5);
a[i, j] := "*";
LBreak:
if i > 0 then a[i - 1, j] := "*";
if i < 4 then a[i + 1, j] := "*";
if j > 0 then a[i, j - 1] := "*";
if j < 4 then a[i, j + 1] := "*";
end;
begin
Crystal(a);
for i := 0 to 4 do begin
for j := 0 to 4 do Write(a[i, j]);
WriteLn
end;
end.
Понятно, что рисование кристалла можно вынести в процедуру и сделать обращение к ней из внутреннего цикла, после чего сделать exit. После цикла написать еще одно обращение к этой процедуре. Это немного ухудшит эффективность, но goto исчезнет. Я бы, наверно, так и сделал. Но не знаю, требуется ли бороться за эффективность при рисовании фрактальных картин.
← →
Дмитрий Белькевич (2009-08-04 21:50) [43]
> и сделать обращение к ней из внутреннего цикла
Как я и говорил, лучше циклы выносить вместе:
procedure Crystal(var a: TMatrix);
var
i, j: Integer;
procedure CrystalInt(var a: TMatrix);
var
ii, jj: Integer;
begin
for ii := 1 to 5 do
for jj := 1 to 5 do
if a[ii, jj] = "*" then
begin
i := ii;
j := jj;
Exit;
end;
Randomize;
i := Random(5);
j := Random(5);
a[i, j] := "*";
end;
begin
CrystalInt(a);
if i > 0 then
a[i - 1, j] := "*";
if i < 4 then
a[i + 1, j] := "*";
if j > 0 then
a[i, j - 1] := "*";
if j < 4 then
a[i, j + 1] := "*";
end;
← →
Дмитрий Белькевич (2009-08-04 21:53) [44]Вообще - не то, что бы я категорически против goto. Но обычно - можно без. Да и label леньво описывать.
← →
Игорь Шевченко © (2009-08-04 22:08) [45]
procedure Crystal(var a: TMatrix);
var i, j, ii, jj: Integer;
begin
Randomize;
i := Random(5);
j := Random(5);
for ii := 1 to 5 do
for jj := 1 to 5 do
if a[ii, jj] = "*" then
begin
i := ii;
j := jj;
end;
if a[i, j] <> "*" then
a[i, j] := "*";
if i > 0 then a[i - 1, j] := "*";
if i < 4 then a[i + 1, j] := "*";
if j > 0 then a[i, j - 1] := "*";
if j < 4 then a[i, j + 1] := "*";
end;
← →
palva © (2009-08-04 22:21) [46]
> Игорь Шевченко © (04.08.09 22:08) [45]
Ну здесь отказ от break и уход от попытки прекратить полный просмотр матрицы при первом найденном центре.
← →
palva © (2009-08-04 22:28) [47]Хотя сам алгоритм определения места кристалла тоже уязвим, поскольку кристаллы имеют тенденцию появляться в верхней части фрактальной матрицы. Можно начинать искать со случайного места внутри матрицы, раз уж случайные i и j вычислены и, дойдя до конца продолжать поиск с начала (два воложенных цикла while)
Это замечание к goto не относится.
← →
Игорь Шевченко © (2009-08-04 22:40) [48]palva © (04.08.09 22:21) [46]
прошу прощения, break разумеется забыл. Но goto мне здесь кажется излишним.
← →
palva © (2009-08-04 22:46) [49]А может вместо двух вложенных for сделать один while с индивидуальным наращиванием ii и jj. На эффективности это вряд ли скажется, зато break будет работать как надо.
← →
Игорь Шевченко © (2009-08-04 23:38) [50]
> А может вместо двух вложенных for сделать один while с индивидуальным
> наращиванием ii и jj. На эффективности это вряд ли скажется,
> зато break будет работать как надо.
Не сразу сообразил - ты же из двух циклов выходишь.
Тогда как бы сделал я, чуть больше кода, но мне яснее:{$APPTYPE CONSOLE}
type
TMatrix = array[0..4,0..4] of Char;
TCrystalCenter = record x, y: Integer end;
var
i, j: Integer;
a: TMatrix = (
(".", ".", ".", ".", "."),
(".", ".", ".", ".", "."),
(".", ".", ".", "*", "."),
(".", ".", ".", ".", "."),
(".", ".", ".", ".", ".")
);
function CrystalCenter(M: TMatrix): TCrystalCenter;
var
I, J: Integer;
begin
Result.x := -1;
Result.y := -1;
for I := 0 to 4 do
for J := 0 to 4 do
if M[I, J] = "*" then
begin
Result.X := I;
Result.y := J;
Exit;
end;
end;
procedure Crystal(var a: TMatrix);
var
C: TCrystalCenter;
begin
C := CrystalCenter(a);
if (C.X = -1) or (C.Y = -1) then
begin
Randomize;
C.X := Random(5);
C.Y := Random(5);
a[C.X, C.Y] := "*";
end;
if C.X > 0 then
a[C.X - 1, C.Y] := "*";
if C.X < 4 then
a[C.X + 1, C.Y] := "*";
if C.Y > 0 then
a[C.X, C.Y - 1] := "*";
if C.Y < 4 then
a[C.X, C.Y + 1] := "*";
end;
begin
Crystal(a);
for i := 0 to 4 do
begin
for j := 0 to 4 do
Write(a[i, j]);
WriteLn
end;
end.
← →
Kolan © (2009-08-04 23:57) [51]О, palva, это уже похоже на реальную задачу и вот что я думаю про этот код:
Сначала я, как обычно, ничего не понял, скопировал в Делфи и запустил. Потом стал разбирать код и мне сразу показалось, что тут не хватает комментариев. И я написал их.procedure Crystal(var a: TMatrix);
var i, j, ii, jj: Integer;
label LBreak;
begin
{Определить есть ли центр кристализации}
for ii := 1 to 5 do for jj := 1 to 5 do
if a[ii, jj] = "*" then begin
i := ii; j := jj;
goto LBreak;
end;
{Если центра нет, то сделать любую точку центром.}
Randomize;
i := Random(5); j := Random(5);
a[i, j] := "*";
{Построить кристал.}
LBreak:
if i > 0 then a[i - 1, j] := "*";
if i < 4 then a[i + 1, j] := "*";
if j > 0 then a[i, j - 1] := "*";
if j < 4 then a[i, j + 1] := "*";
end;
Такие комментарии Фаулер называет дезодорантом, они просто прикрывают плахой запах, маскируют его. Весьма вероятно этот метод нужно разделить, как раз по комментариям.
Кроме отсутствовавших комментариев о необходимости рефакторинга говорит название. По-честному этот метод должен называтьсяНайтиЦентрКристализацииИКристализовать
или подобным образом.
Кстати, кажется вы опять назвали вункцию существительным
или вы имели в виду именно глагол?
Так как тут задача не так абстрактна, то можно сделать хороший рефакторинг. Я решил применить Метод поиска существительных к заданию.
Смотрите: Нужно нарисовать кристалл на матрице. Если на матрице уже есть центр кристаллизации (звездочка), то использовать его, если нет, выбрать центр кристалла случайным образом.
Тут полно существительных:
о кристалл,
о матрица,
о центр кристаллизации.
Все это очень похоже на объекты предметной области. Вот что получилось в итоге:
program Project1;
{$APPTYPE CONSOLE}
uses
SysUtils,
Types;
type
{Матрица. На матрице можно построить кристалл.}
TMatrix = array[0..4, 0..4] of Char;
{Кристал.}
TCrystal = class
private
{Матрица кристала. Без неё кристаллу негде было бы расти.}
FMatrix: TMatrix;
protected
{Найти центр крстализации. Если его нет — будет возвращено (-1, -1)}
function FindCrystallizationCenter(const Matrix: TMatrix): TPoint;
{Создать центр кристализации в произвольной точке.
Возвращает координаты созданного центра.}
function CreateRandomCrystallizationCenter(var Matrix: TMatrix): TPoint;
public
{Кристаллизовать кристалл :)}
procedure Crystallize;
{Кристалл просто сохраняет ссылку на переданную матрицу,
не портите её, пожалуйста.}
constructor Create(const Matrix: TMatrix);
property Matrix: TMatrix read FMatrix;
end;
{ TCrystal }
constructor TCrystal.Create(const Matrix: TMatrix);
begin
FMatrix := Matrix;
end;
function TCrystal.CreateRandomCrystallizationCenter(var Matrix: TMatrix): TPoint;
begin
Result.X := Random(Length(Matrix));
Result.Y := Random(Length(Matrix[Result.X]));
Matrix[Result.X, Result.Y] := "*";
end;
procedure TCrystal.Crystallize;
var
{Центр кристализации. Из этой точки возникает кристалл.}
CrystallizationCenter: TPoint;
begin
{Найти центр кристализации в матрице, вдруг он уже есть.}
CrystallizationCenter := FindCrystallizationCenter(FMatrix);
{*По-хорошему эта проверка должна быть встроена в TPoint. Точнее вместо TPoint
нужно сделать свою запись, соответствующую предметной области.}
if (CrystallizationCenter.X = -1) and (CrystallizationCenter.Y = -1) then
CrystallizationCenter := CreateRandomCrystallizationCenter(FMatrix);
{Кристализация.}
if CrystallizationCenter.X > 0 then FMatrix[CrystallizationCenter.X - 1, CrystallizationCenter.Y] := "*";
if CrystallizationCenter.X < 4 then FMatrix[CrystallizationCenter.X + 1, CrystallizationCenter.Y] := "*";
if CrystallizationCenter.Y > 0 then FMatrix[CrystallizationCenter.X, CrystallizationCenter.Y - 1] := "*";
if CrystallizationCenter.Y < 4 then FMatrix[CrystallizationCenter.X, CrystallizationCenter.Y + 1] := "*";
end;
function TCrystal.FindCrystallizationCenter(const Matrix: TMatrix): TPoint;
var
I, J: Integer;
begin
Result.X := -1;
Result.Y := -1;
for I := Low(Matrix) to High(Matrix) do
for J := Low(Matrix[J]) to High(Matrix[J]) do
if Matrix[I, J] = "*" then
begin
Result.X := I;
Result.Y := J;
Exit;
end;
end;
procedure WriteLnCrystal(const ACrystal: TCrystal);
var
I, J: Integer;
begin
if Assigned(ACrystal) then
begin
for I := Low(ACrystal.Matrix) to High(ACrystal.Matrix) do
begin
for J := Low(ACrystal.Matrix[J]) to High(ACrystal.Matrix[J]) do
Write(ACrystal.Matrix[I, J]);
WriteLn;
end;
end;
end;
var
Crystal: TCrystal;
Matrix: TMatrix = (
(".", ".", ".", ".", "."),
(".", ".", ".", ".", "."),
(".", ".", ".", "*", "."),
(".", ".", ".", ".", "."),
(".", ".", ".", ".", ".")
);
begin
try
ReportMemoryLeaksOnShutdown := True;
Crystal := TCrystal.Create(Matrix);
try
Crystal.Crystallize;
WriteLnCrystal(Crystal);
ReadLn;
finally
Crystal.Free;
end;
except
on E:Exception do
Writeln(E.Classname, ": ", E.Message);
end;
end.
Вместо вашей функции появился почти осязаемы кристалл, который можно кристаллизовать.
Конечно в коде полно недостатков. Матрица — статический массив и передаётся в класс по ссылке, хорошо бы сделать массив динамическим, и копировать его внутри.
ВместоTPoint
надо сделать другую запись, которая бы соответствовала понятию Координаты из предметной области и имела бы нужные методы.
Да, кода получилось больше и в данном пример это, скорее всего, не оправданно. Однако в реальной программе я сделал бы нечто подобное, потому что такой код проще поддерживать, он само документирован и его проще передать кому-то.
P. S. У меня уже глаза слипаются, если где-то наделал ошибок — простите, я просто показывал идею.
Кстати, можно продолжить. Некоторые методы вроде поиска и выставления случайного центра кристаллизации попахивают «Feature Envy», так что из лучше перенести использую Перенесение метода в другой класс, которого сейчас вообще нет, в Матрицу. Может завтра руки дойдут.
← →
Игорь Шевченко © (2009-08-05 00:01) [52]Kolan © (04.08.09 23:57) [51]
И не жалко тебе пальцев ? Столько лишнего написать...
← →
palva © (2009-08-05 00:34) [53]
> И не жалко тебе пальцев ?
Ну так это, как я понял, всё генерируется.
← →
Игорь Шевченко © (2009-08-05 01:26) [54]
> Ну так это, как я понял, всё генерируется.
Да, вместе с комментариями
← →
Kolan © (2009-08-05 10:48) [55]Для данного примера писать все это (а само оно, к сожалению, не генерируется) было очень лень, но в реальном проекте я бы пошел именно этим путем.
Да этот код сложнее изначального, но у него есть приятная особенность — его легко вспоминать. Дело в том, что мой мозг, ничего не может запомнить. Я могу забыть где что в проекте если прервусь на день, а за неделю я забываю вообще все, подчистую. Поэтому пишу так, чтобы, когда я снова буду разбираться в коде, который к этому времени будет как чужой, мне было легко.
Цель того, что я писал в этой ветки — показать как я анализирую и перерабатываю код, а принимать это во внимание или нет — ваше дело.
Еще про комментарии и код у меня есть две заметки:
http://www.ksoftware.ru/wiki/narrative_code
http://www.ksoftware.ru/wiki/hanging_comments
Страницы: 1 2 вся ветка
Форум: "Начинающим";
Текущий архив: 2009.10.11;
Скачать: [xml.tar.bz2];
Память: 0.59 MB
Время: 0.006 c