Форум программистов [Powered by Invision Power Board]

Версия для печати темы
Нажмите сюда для просмотра этой темы в оригинальном формате

Форум программистов > Форум группы: delphi > Покритикуйте класс и код

Автор: VladBD 3.4.2010, 13:46

RAD 2010, прога - помощник для игры Сапер
писал для того чтоб разобраться с некоторыми моментами.
просьба оценить класс и высказать критику

Автор: RomanEEP 15.4.2010, 20:27

Отлично написано!

Автор: cemick 21.4.2010, 14:12

Если хотите что бы по придерались, то выскажу свою субъективное ИМХО на основе внешнего вида кода, не вдаваясь в логику
и работу программы:

1. Все поля я бы обозначал префиксом F, это касается и

Код


    cEMPTY_RECT : TRect;
    aOffsetRange : array[0..7] of TPoint;

2. Почему это

Код


caBorder   = 99;
    caInit     = 9;
    caEmpty    = 0;
    caMine     = 10;
    caFlag     = 11;
    caQuestion = 12;
    caDigit1 = 1;
    caDigit2 = 2;
    caDigit3 = 3;
    caDigit4 = 4;
    caDigit5 = 5;
    caDigit6 = 6;
    caDigit7 = 7;
    caDigit8 = 8;

было не объявить перечислением?
3. Параметры методов принято начинать с префикса "A"
4. Всегда операции (:= = + * < и т.д) обрамлять пробелами
5. Вот это

Код


      if (X1<X) and (X<X2) and (Y1<Y) and (Y<Y2) then FMap[Y, X] := caInit 
      else FMap[Y, X] := caBorder;

выглядело лучше хотя бы вот так

Код


      if (X1<X) and (X<X2) and (Y1<Y) and (Y<Y2) then 
        FMap[Y, X] := caInit
      else 
        FMap[Y, X] := caBorder;

6. Забавное название метода procedure THelper.FMapInit;
7. Подобного рода конструкции

Код


clBlue   : begin FMap[Y, X] := caDigit1; changeCell := True; Break; end;

ужасны, лучше вот

Код


clBlue   : 
  begin 
    FMap[Y, X] := caDigit1; 
    changeCell := True; 
    Break; 
  end;

И что тут делает Break?
8.

Код


 if not(changeCell) and (Pos('990000000000', S)>0) then begin FMap[Y, X] := caInit; changeCell := True; end;
      if not(changeCell) and (Pos('033003300330', S)>0) then begin FMap[Y, X] := caDigit3; changeCell := True; end;
      if not(changeCell) and (Pos('077000077770', S)>0) then begin FMap[Y, X] := caDigit7; changeCell := True; end;
      if not(changeCell) and (Pos('088008800880', S)>0) then begin FMap[Y, X] := caDigit8; changeCell := True; end;
      if not(changeCell) and (Pos('777777777777', S)>0) then begin FMap[Y, X] := caMine; changeCell := True; end;
      if not(changeCell) and (Pos('033333777770', S)>0) then begin FMap[Y, X] := caFlag; changeCell := True; end;
      if not(changeCell) and (Pos('070007770770', S)>0) then begin FMap[Y, X] := caQuestion; changeCell := True; end;

тоже самое что и предыдущий плюс добавить else (т.е. если выполниться первое условие, то зачем проверять следующие?)
И вместо Pos я бы использовал AnsiContainsText.
9. В названиях подобных функци isInitAnyCells лучше не использовать вопрос
10. Незабываем про сообщения компилятора, и подчищать код (проект не собирал наверняка их не мало):

Код


procedure THelper.Run;
var
  aStep, screenStep : TPoint;
begin
  Prepare;
  while (Mode=hmRun) do
  begin
    // считываем карту
    Prepare;
    //
    if not(SetCells) then Exit;


  end;
end;

11. Если уж показываем код людям то больше комментариев
12. Не понятно зачем тут S

Код


function THelper.MapToString : String;
var
  I, J : Integer;
  S : String;
begin
  S := '';
  // инициализация
  for J := 0 to High(FMap) do
  begin
    for I := 0 to High(FMap[0]) do
      S := S + IntToStr(FMap[J, I])+'|';
    S := S+#13#10;
  end;
  Result := S;
end;

13. Есть такая функция Format

ps Большинство придирок наверно мелочны и получены простым пробеганием взгляда по коду