Текущий архив: 2009.10.11;
Скачать: CL | DM;

Вниз

Вложенный цикл   Найти похожие ветки 

← →
Дмитрий Белькевич   (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;
Скачать: CL | DM;

Наверх

Память: 0.6 MB
Время: 0.016 c