Покритикуйте пожалуйста исходник - О жизни

На мою критику исходного кода человек сказал что я явно придираюсь и не знаю чего хочу, а код его практически идеальный за исключением нескольких моментов. Нет слов.... смотрите сами, здесь всего один метод.

Мне даже интерессно кто сколько недостатков в этом коде найдет :). Каждвый недостатки пусть перечисляет в своей ветке.

procedure TFrmCMMain.PageControlMainChange(Sender: TObject);
var
  activeDetailFrame: TframeDetailBase;
begin
  inherited;

  if PageControlMain.ActivePage = tabGeneral then
    BuildToolButtons(acSEVISGeneral)
  else
    begin
      activeDetailFrame := FindActiveDetailFrame((Sender as TcxPageControl).ActivePage);
      if activeDetailFrame <> nil then
      begin
        //Caption := activeDetailFrame.Name;
        BuildToolButtons(activeDetailFrame.SevisActions);
      end;
    end;

  //Will open the needed datasets when the active page on the tab control is changed
  if ((Sender as TcxPageControl).ActivePage = tabOverview)AND (cDetailSS_ID.Text<>'') then begin
    frameOverview1.SSID := cDetailSS_ID.Text;
    frameOverview1.SSRecordTypeID := cDetailSS_RECORDTYPE.Text;
    frameOverview1.UpdateParameters;
    if not frameOverview1.cdsWorkAuth.Active then
      frameOverview1.cdsWorkAuth.Open
    else begin
      frameOverview1.cdsWorkAuth.Close;
      frameOverview1.cdsWorkAuth.Open;
      //frameOverview1.cdsWorkAuth.Refresh();
    end;



    //dsCountry
    if not cdsCountry.Active  then
      cdsCountry.Open;
    if not frameOverview1.cdsNote.Active then
      frameOverview1.cdsNote.Open
    else begin
      frameOverview1.cdsNote.Close;
      frameOverview1.cdsNote.Open;
    end;
    if not frameOverview1.cdsAppointmentProfile.Active then
      frameOverview1.cdsAppointmentProfile.Open
    else begin
      frameOverview1.cdsAppointmentProfile.Close;
      frameOverview1.cdsAppointmentProfile.Open;
    end;

    //cdsSchool usde as lookup so no refresh is needed
    if not frameOverview1.cdsSchool.Active then
      frameOverview1.cdsSchool.Open;

    if not frameOverview1.cdsDependents.Active then
      frameOverview1.cdsDependents.Open
    else begin
      frameOverview1.cdsDependents.Close;
      frameOverview1.cdsDependents.Open;
    end;
    if not LkpDM.lkpWorkAuthTypes.Active  then
      LkpDM.lkpWorkAuthTypes.Open;

  //    cdsAddress
    if not frameOverview1.cdsAddress.Active then
      frameOverview1.cdsAddress.Open
    else begin
      frameOverview1.cdsAddress.Close;
      frameOverview1.cdsAddress.Open;
    end;
    if not LkpDM.lkpWorkAuthTypes.Active  then
      LkpDM.lkpWorkAuthTypes.Open;

    //cdsContact
    if not frameOverview1.cdsContact.Active then
      frameOverview1.cdsContact.Open
    else begin
      frameOverview1.cdsContact.Close;
      frameOverview1.cdsContact.Open;
    end;

    //cdsOrganization usde as lookup so no refresh is needed
    if not frameOverview1.cdsOrganization.Active then
      frameOverview1.cdsOrganization.Open;

    if not LkpDM.lkpWorkAuthTypes.Active  then
      LkpDM.lkpWorkAuthTypes.Open;

//    frameOverview1.SSRecordTypeID := cListIMGP_VISATYPE.Text;

//    if not frameOverview1.cFrmDetail.Active then
//      frameOverview1.cFrmDetail.Open;
//    if not (LkpDM.LkpAddrType.Active) then
//      LkpDM.LkpAddrType.Open;
//    if not (LkpDM.LkpPhoneType.Active) then
//      LkpDM.LkpPhoneType.Open;
  end;

  if ((Sender as TcxPageControl).ActivePage = tabGeneral) then begin
  // manage controls, depending on VisaType
  // F
    if (IMGPVisaTypeCode = '01') or (IMGPVisaTypeCode = '04')  then begin
        eSS_SEVISID.Enabled := true;
        eSS_PREVSEVISID.Enabled := true;
        eSS_HOMECNTRYPOSITION.Enabled := false;
        eSS_HOMECNTRYPOSCODE.Enabled := false;
        eSS_HOMECNTRYEMPLOYER.Enabled := false;
        dbimcbUSEmplVisa.Enabled := false;
        eSS_USEMPLOYERNAME.Enabled := false;
        eSS_HIGHESTDEGREEEARNED.Enabled := false;
        dbimcbHighestDegLocation.Enabled := false;
        meSS_HIGHESTDEGYEAR.Enabled := false;
        eSS_HIGHESTDEGINST.Enabled := false;
        eSS_HIGHESTDEGINSTLOC.Enabled := false;
    end
    else begin
    // J
    if (IMGPVisaTypeCode = '03') or (IMGPVisaTypeCode = '06')  then begin
        eSS_SEVISID.Enabled := true;
        eSS_PREVSEVISID.Enabled := true;
        eSS_HOMECNTRYPOSITION.Enabled := true;
        eSS_HOMECNTRYPOSCODE.Enabled := true;
        eSS_HOMECNTRYEMPLOYER.Enabled := true;
        dbimcbUSEmplVisa.Enabled := false;
        eSS_USEMPLOYERNAME.Enabled := false;
        eSS_HIGHESTDEGREEEARNED.Enabled := false;
        dbimcbHighestDegLocation.Enabled := false;
        meSS_HIGHESTDEGYEAR.Enabled := false;
        eSS_HIGHESTDEGINST.Enabled := false;
        eSS_HIGHESTDEGINSTLOC.Enabled := false;
    end
    else begin

    // other
        eSS_SEVISID.Enabled := false;
        eSS_PREVSEVISID.Enabled := false;
        eSS_HOMECNTRYPOSITION.Enabled := false;
        eSS_HOMECNTRYPOSCODE.Enabled := true;
        eSS_HOMECNTRYEMPLOYER.Enabled := true;
        dbimcbUSEmplVisa.Enabled := true;
        eSS_USEMPLOYERNAME.Enabled := true;
        eSS_HIGHESTDEGREEEARNED.Enabled := true;
        dbimcbHighestDegLocation.Enabled := true;
        meSS_HIGHESTDEGYEAR.Enabled := true;
        eSS_HIGHESTDEGINST.Enabled := true;
        eSS_HIGHESTDEGINSTLOC.Enabled := true;
    end;
    end

  end;

  if ((Sender as TcxPageControl).ActivePage = tabAddressPhone) then begin
    framePhoneAddressFrame1.SSID := cDetailSS_ID.Text;
    framePhoneAddressFrame1.SSRecordTypeID := cDetailSS_RECORDTYPE.Text;
    if not (LkpDM.LkpAddrType.Active) then
      LkpDM.LkpAddrType.Open;
    if not (LkpDM.LkpPhoneType.Active) then
      LkpDM.LkpPhoneType.Open;
    if not framePhoneAddressFrame1.cFrmDetail.Active then
      framePhoneAddressFrame1.cFrmDetail.Open;
  end;

  if ((Sender as TcxPageControl).ActivePage = tabDependents ) then begin
    frameDependants1.SSID := cDetailSS_ID.Text;
    frameDependants1.SSRecordTypeID := cDetailSS_RECORDTYPE.Text;
    frameDependants1.IMGPVisaTypeCode := cListIMGP_VISATYPE.Text;

    if not (LkpDM.lkpNameSuffix.Active) then
      LkpDM.lkpNameSuffix.Open;
    if not frameDependants1.cdsCountry.Active then
      frameDependants1.cdsCountry.Open;
    if not frameDependants1.cFrmDetail.Active then
      frameDependants1.cFrmDetail.Open;
  end;

  if ((Sender as TcxPageControl).ActivePage = tabImmigrationProfiles ) then begin
      frameImmigrationProfiles1.SSID := cDetailSS_ID.Text;
      frameImmigrationProfiles1.SSRecordTypeID := cDetailSS_RECORDTYPE.Text;
      frameImmigrationProfiles1.IMGPVisaTypeCode := cListIMGP_VISATYPE.Text;

//    if not (LkpDM.lkpSubjectCode.Active) then
//      LkpDM.lkpSubjectCode.Open;
    if not (LkpDM.lkpVisitorCategory.Active) then
      LkpDM.lkpVisitorCategory.Open;
    if not (LkpDM.lkpSEVISStatus.Active) then
      LkpDM.lkpSEVISStatus.Open;
    if not (LkpDM.LkpVisaSponsor.Active) then
      LkpDM.LkpVisaSponsor.Open;
    if not (LkpDM.LkpVisaType.Active) then
      LkpDM.LkpVisaType.Open;
    if not frameImmigrationProfiles1.cFrmDetail.Active then
      frameImmigrationProfiles1.cFrmDetail.Open;
  end;

   if ((Sender as TcxPageControl).ActivePage = tabNotes ) then begin
    frameNotes1.SSID := cDetailSS_ID.Text;
    frameNotes1.SSRecordTypeID := cDetailSS_RECORDTYPE.Text;
   if not frameNotes1.cFrmDetail.Active then
      frameNotes1.cFrmDetail.Open;
   if not (LkpDM.lkpNoteType.Active) then
      LkpDM.lkpNoteType.Open;
  end;

   if ((Sender as TcxPageControl).ActivePage = tabFinancialProfiles ) then begin
    frameFinancialProfiles1.SSID := cDetailSS_ID.Text;
    frameFinancialProfiles1.SSRecordTypeID := cDetailSS_RECORDTYPE.Text;
    frameFinancialProfiles1.IMGPVisaTypeCode := cListIMGP_VISATYPE.Text;
   if not frameFinancialProfiles1.cFrmDetail.Active then
      frameFinancialProfiles1.cFrmDetail.Open;
   if not (LkpDM.lkpFundType.Active) then
      LkpDM.lkpFundType.Open;

  end;

  if ((Sender as TcxPageControl).ActivePage = tabDocuments ) then begin
    frameDocuments1.SSID := cDetailSS_ID.Text;
    frameDocuments1.SSRecordTypeID := cDetailSS_RECORDTYPE.Text;
  if not (LkpDM.LkpDocumentType.Active) then
      LkpDM.LkpDocumentType.Open; 
   if not frameDocuments1.cFrmDetail.Active then
      frameDocuments1.cFrmDetail.Open;
  end;

  if ((Sender as TcxPageControl).ActivePage = tabPermResidAppl ) then begin
    framePermResidAppl1.SSID := cDetailSS_ID.Text;
    framePermResidAppl1.SSRecordTypeID := cDetailSS_RECORDTYPE.Text;
    if not LkpDM.LkpProcOfGreenCard.Active  then
      LkpDM.LkpProcOfGreenCard.Open;
    if not LkpDM.LkpPermPosCategory.Active  then
      LkpDM.LkpPermPosCategory.Open;
    if not LkpDM.LkpPermPosType.Active  then
      LkpDM.LkpPermPosType.Open;
    if not LkpDM.LkpPermPosStatus.Active  then
      LkpDM.LkpPermPosStatus.Open;
   if not framePermResidAppl1.cFrmDetail.Active then
      framePermResidAppl1.cFrmDetail.Open;
  end;

  if ((Sender as TcxPageControl).ActivePage = tabAppointmentProfiles ) then begin
    frameAppProfiles1.SSID := cDetailSS_ID.Text;
    frameAppProfiles1.SSRecordTypeID := cDetailSS_RECORDTYPE.Text;
    if not frameAppProfiles1.cdsSchool.Active then
      frameAppProfiles1.cdsSchool.Open;
    if not frameAppProfiles1.cdsOrganization.Active then
      frameAppProfiles1.cdsOrganization.Open;
   if not LkpDM.lkpSalaryUnits.Active  then
      LkpDM.lkpSalaryUnits.Open;
   if not frameAppProfiles1.cFrmDetail.Active then
      frameAppProfiles1.cFrmDetail.Open;

  end;

  if ((Sender as TcxPageControl).ActivePage = tabWorkAuth ) then begin
   frameWorkAuthorizations1.SSID := cDetailSS_ID.Text;
   frameWorkAuthorizations1.SSRecordTypeID := cDetailSS_RECORDTYPE.Text;
   if not LkpDM.lkpWorkAuthTypes.Active  then
      LkpDM.lkpWorkAuthTypes.Open;
   if not LkpDM.lkpWorkAuthEmpType.Active  then
      LkpDM.lkpWorkAuthEmpType.Open;
   if not frameWorkAuthorizations1.cFrmDetail.Active then
      frameWorkAuthorizations1.cFrmDetail.Open;
  end;

   if ((Sender as TcxPageControl).ActivePage = tabAcademicProfiles ) then begin
    frameAcademicProfiles1.SSID := cDetailSS_ID.Text;
    frameAcademicProfiles1.SSRecordTypeID := cDetailSS_RECORDTYPE.Text;
   if not frameAcademicProfiles1.cFrmDetail.Active then
      frameAcademicProfiles1.cFrmDetail.Open;
   end;

   if ((Sender as TcxPageControl).ActivePage = tabNonSevis ) then
   begin
    frameNonSevis1.SSID := cDetailSS_ID.Text;
    frameNonSevis1.SSRecordTypeID := cDetailSS_RECORDTYPE.Text;

    if not LkpDM.LkpNONSEVISFORMS.Active  then
              LkpDM.LkpNONSEVISFORMS.Open;

    frameNonSevis1.cbForms.ItemIndex:=-1;
    if frameNonSevis1.cList.Active then frameNonSevis1.cList.Close;

   end;

  //Update button state is called to apply the autoedit and button state changes on tab change.
  //Since datasets are being opened after the first time user click on the tab Autoedit needs to be reset.
  UpdateButtonState;
end;

Добавлен тэг pascal для подсветки кода

Здравствуйте, anonim2006, Вы писали:

A>На мою критику исходного кода человек сказал что я явно придираюсь и не знаю чего хочу, а код его практически идеальный за исключением нескольких моментов. Нет слов.... смотрите сами, здесь всего один метод.

A>Мне даже интерессно кто сколько недостатков в этом коде найдет

. Каждвый недостатки пусть перечисляет в своей ветке.

1. Не используется тэг pascal.

... << RSDN@Home 1.1.4 stable SR1 rev. 568>>

Здравствуйте, Lloyd, Вы писали:

L>Здравствуйте, anonim2006, Вы писали:

A>>На мою критику исходного кода человек сказал что я явно придираюсь и не знаю чего хочу, а код его практически идеальный за исключением нескольких моментов. Нет слов.... смотрите сами, здесь всего один метод.

A>>Мне даже интерессно кто сколько недостатков в этом коде найдет

. Каждвый недостатки пусть перечисляет в своей ветке.

L>1. Не используется тэг pascal.

0. Отсутствует культурное форматирование.

А главное все понятно

Здравствуйте, anonim2006, Вы писали:

<... Skip ....>

На первый беглый взгляд присутствуют повторы одних и тех же операций.

И с пэйдж контролом улыбнуло где по имени, а где через сэндер...

Мое мнение после оптимизации и причесывания количество кода уменьшится раза в полтора...

А так вообще струдом просмотрел...

... << RSDN@Home 1.2.0 alpha rev. 654>> -= Windows XP 5.1.2600.131072 =- А в Winamp'e: Дубцова Ирина — Роман

Здравствуйте, anonim2006, Вы писали:

A>На мою критику исходного кода человек сказал что я явно придираюсь и не знаю чего хочу, а код его практически идеальный за исключением нескольких моментов. Нет слов.... смотрите сами, здесь всего один метод.

Главный — процедура занимает штук пять экранов и обозреть ее не представляется возможным

Интересно, ты у него спросил, согласен ли он на такой коллективный code review.
Тебе же с ним навернякак дальше работать.

Привлекать коллективный разум, чтобы надавить на коллегу — это вещь бесполезная.
Лучше почитай как вообще нормально проводят code review и что вообще делают с замечаниями по коду.

. Каждвый недостатки пусть перечисляет в своей ветке.
Очень давно на дельфи не писал, поэтому могу где-то и натупить.

Вопрос — в контексте данной процедуры PageControlMain.ActivePage== Sender as TcxPageControl).ActivePage?
Судя по названиям — да. Неоднородность.

Миллион последовательных if'ов требует логика приложения, или же человек не умеет пользоваться, как минимум, switch?
Плохое форматирование кода.

Общее впечатление — чрезмерное дублирование кода (жесткий копипейст с разных мест?). Огромный объем процедуры.
Кажется, что это классический пример антипаттерна "вся логика на обработчиках кнопок".

Ломать не строить, но, кажется, что после причесывания процедура уменьшится в несколько раз.

... << RSDN@Home 1.2.0 alpha rev. 643>>

. Каждвый недостатки пусть перечисляет в своей ветке.

[flame]
1) Написано на маздайном Дельфи. Реальные программисты пишут на С++, Хаскелле и Немерле.

[/flame]

2) Сверхдлинная функция. Резать к чёртовой матери, не дожидаясь перитонита! А именно:

3) Много повторяющихся действий вида
— if not smth.Active then smth.Open else begin smth.Close; smth.Open end; -- заменить на функцию reOpen(smth)
— if not smth.Active then smth.Open; -- заменить на функцию justOpen(smth)

4) Ветвление по значениям sender'а собрать в одном месте и опять же превратить обработку каждого конкретного случая в вызов функции

5) Ветвление по типам Visa — нельзя ли превратить из отображения тип->(кортеж булей) в кортеж отображений тип->буль?

type
  TVisaType : (visa_01_04, visa_03_06, visa_other);
var
  visaType : TVisaType;
begin
  .....
  if (IMGPVisaTypeCode = '01') or (IMGPVisaTypeCode = '04')      then visaType := visa_01_04
  else if (IMGPVisaTypeCode = '03') or (IMGPVisaTypeCode = '06') then visaType := visa_03_06
  else                                                                visaType := visa_other ;
  
  eSS_SEVISID.Enabled := (visaType <> visa_other);
  eSS_PREVSEVISID.Enabled := (visaType <> visa_other);
  eSS_HOMECNTRYPOSITION.Enabled := (visaType = visa_03_06);
  // и т.д.

У этого варианта есть недостаток: трудно добавлять новые типы. Всё зависит от предметной области.
В качестве альтернативы можно вместо отображения тип -> кортеж присваиваний завести отображение тип -> кортеж значений, а присваивание сделать централизовано.

type
  TElementID : (elem_SERVISID, elem_PREVSERVISID, elem_HOMECNTRYPOSITION, .....);
const
  visaElem : array [TVisaType] of array [TElementID] of boolean =
  (
    (true, true, true, .....),
    (true, true, false, .....),
    (false, false, false, .....),
    .....
  )
begin
  .....
  eSS_SERVISID.Enabled := visaElem[visaType][elem_SERVISID];
  eSS_PREVSEVISID.Enabled := visaElem[visaType][elem_PREVSERVISID];
  eSS_HOMECNTRYPOSITION.Enabled := visaElem[visaType][elem_HOMECNTRYPOSITION];
  .....

... << RSDN@Home 1.2.0 alpha rev. 655>>

Ссылки вида frameOverview1.cdsContact.Close; — просто АД.
В общем код трудночитаемый, отсутствует форматирование, именование объектов оставляет пожелать всего самого наилучшего автору кода — он сам через 2 месяца поймет свой код, да еще с таким малым количеством комментариев?
Аналогичный код я видел когда работал в МВ

Автор кода не в МВ случайно работает?

Здравствуйте, anonim2006, Вы писали:

A>Мне даже интерессно кто сколько недостатков в этом коде найдет

. Каждвый недостатки пусть перечисляет в своей ветке.

Бывает код хуже. Намного

Могу даже сказать, что в этом коде есть плюсы: объекты именованы, отступы соблюдаются.
Человеку не наплевать, по крайней мере.
А то представь себе бред из
unit1.pas, unit2.pas ... unit43.pas, в котором form1...form43, в которых edit1...edit11, ну и т.п.
Для примера:

procedure Form22.Button5Click;
var
    konec: integer;
begin
    if CheckBox12.Checked and (Form12.RadioGroup2.ItemIndex= 2)

    then
    begin
Form13.Button2.Enabled := true;

        Form13.Button3.Enabled := false;
        end;
end;

Это я не придумал. Отступы, пустые строки, наимененования — по памяти из жизни.
Мне сейчас проект попался на BC++ — ни одного отступа ВАЩЕ!

Сейчас играет <Hallucinogen — 1997 — Lone Deranger — Demention>

. Каждвый недостатки пусть перечисляет в своей ветке.

A>

A>

// ...
activeDetailFrame := FindActiveDetailFrame((Sender as TcxPageControl).ActivePage);
      if activeDetailFrame <> nil then
// ...
// Опять обращение Sender as TcxPageControl
if ((Sender as TcxPageControl).ActivePage = tabOverview)AND (cDetailSS_ID.Text<>'') then begin
// ...
// Снова...
if ((Sender as TcxPageControl).ActivePage = tabGeneral) then begin
// ...

По идее не хватает

else

Exit

'ом после

    begin
      activeDetailFrame := FindActiveDetailFrame((Sender as TcxPageControl).ActivePage);
      if activeDetailFrame <> nil then
      begin
        //Caption := activeDetailFrame.Name;
        BuildToolButtons(activeDetailFrame.SevisActions);
      end;
    end;

	От:	anonim2006
	Дата:	14.09.06 13:38
	Оценка:

	От:	Lloyd
	Дата:	14.09.06 13:43
	Оценка:	+5

	От:	alcotras
	Дата:	14.09.06 13:45
	Оценка:

	От:	casual
	Дата:	14.09.06 13:52
	Оценка:

	От:	Shtirliz
	Дата:	14.09.06 14:15
	Оценка:	+2

От:	DangerRSDN	http://danger-world.livejournal.com/
Дата:	15.09.06 07:34
Оценка:

	От:	Skleroz
	Дата:	03.10.06 10:15
	Оценка:

	От:	_Oleg_
	Дата:	03.10.06 13:03
	Оценка: