Potrzebuję uczynić mój kod bardziej czytelnym dla innych programistów w moim zespole

Pracuję nad projektem w delphi i tworzę instalator aplikacji, są trzy główne części.

1. Instalacja / dezinstalacja PostgreSQL
2. myapplication (konfiguracja myapplication jest tworzona za pomocą nsi) instalacja / deinstalacja.
3. Tworzenie tabel w Postgresie za pomocą skryptu (pliki wsadowe).

Każda rzecz działa dobrze i płynnie, ale jeśli coś się nie powiedzie, stworzyłem LogToFileger, który będzie LogToFile na każdym etapie procesu,
jak ten

LogToFileToFile.LogToFile('[DatabaseInstallation]  :  [ACTION]:Postgres installation started');

Funkcja LogToFileToFile.LogToFile()Spowoduje to zapisanie zawartości do pliku. Działa to ładnie, ale problem polega na tym, że zepsuł się kod, ponieważ trudno było odczytać kod, ponieważ można zobaczyć tylko LogToFileToFile.LogToFile()wywołanie funkcji wszędzie w kodzie

przykład

 if Not FileExists(SystemDrive+'\FileName.txt') then
 begin
    if CopyFile(PChar(FilePathBase+'FileName.txt'), PChar(SystemDrive+'\FileName.txt'), False) then
       LogToFileToFile.LogToFile('[DatabaseInstallation] :  copying FileName.txt to '+SystemDrive+'\ done')
       else
       LogToFileToFile.LogToFile('[DatabaseInstallation] :  copying FileName.txt to '+SystemDrive+'\ Failed');
 end;
 if Not FileExists(SystemDrive+'\SecondFileName.txt')      then
   begin
     if CopyFile(PChar(FilePathBase+'SecondFileName.txt'), PChar('c:\SecondFileName.txt'), False) then
       LogToFileToFile.LogToFile('[DatabaseInstallation] : copying SecondFileName.txt to '+SystemDrive+'\ done')
   else
       LogToFileToFile.LogToFile('[DatabaseInstallation] :  copying SecondFileName.txt to '+SystemDrive+'\ Failed');
 end;

jak widać, było wiele LogToFileToFile.LogToFile()połączeń,
zanim było

 if Not FileExists(SystemDrive+'\FileName.txt') then
    CopyFile(PChar(FilePathBase+'FileName.txt'), PChar(SystemDrive+'\FileName.txt'), False) 
 if Not FileExists(SystemDrive+'\SecondFileName.txt')      then
   CopyFile(PChar(FilePathBase+'SecondFileName.txt'), PChar('c:\SecondFileName.txt'), False)

tak jest teraz w całym moim kodzie.
jest trudny do odczytania.

czy ktoś może mi zasugerować dobry sposób na uporządkowanie połączeń do LogToFile?

lubić

1. Wcięcie wywołania „LogToFileToFile.LogToFile ()` w
   ten sposób

      if Not FileExists(SystemDrive+'\FileName.txt') then
        begin
            if CopyFile(PChar(FilePathBase+'FileName.txt'), PChar(SystemDrive+'\FileName.txt'), False) then
           {Far away--->>}                   LogToFileToFile.LogToFile(2,'[DatabaseInstallation] :  [ACTION]:copying FileName.txt to '+SystemDrive+'\ sucessful')
      else
           {Far away--->>}                   LogToFileToFile.LogToFile(2,'[DatabaseInstallation] :  [ACTION]:copying FileName.txt to '+SystemDrive+'\ Failed');
      end;
   

2. Oddzielna jednostka, taka jak LogToFileger
   Ta jednostka będzie zawierała wszystkie wiadomości LogToFile w switch casepodobny sposób

    Function LogToFilegingMyMessage(LogToFilegMessage : integer)
   
    begin
   case  LogToFilegMessage of
   
   1         :  LogToFileToFile.LogToFile(2,'[DatabaseInstallation] :  [ACTION]:copying FileName.txt to '+SystemDrive+'\ sucessful');
   2         :  LogToFileToFile.LogToFile(2,'[DatabaseInstallation] :  [ACTION]:copying FileName.txt to '+SystemDrive+'\ Failed');
      150        :  LogToFileToFile.LogToFile(2,'[somthing] :  [ACTION]: somthing important);
   
   end;
   

więc mogę po prostu wywołać LogToFilegingMyMessage (1) tam, gdzie jest to wymagane.

Czy ktoś może mi powiedzieć, które jest lepsze i czystsze podejście do LogToFileging w ten sposób?

Dodając rejestrowanie, wprowadziłeś dwie rzeczy:

1. Kod stał się większy, ponieważ dla prawie każdej akcji dodano wiersz rejestrujący tę akcję (lub jej niepowodzenie)
2. Same linie kłód wydają się być wzdęte i odbierają czytelność, ponieważ zajmują tyle miejsca.

Każdy z tych problemów ma swoje własne, stosunkowo proste rozwiązanie:

1. Podziel kod na mniejsze funkcje. Zamiast jednej gigantycznej funkcji, która zawiera wszystkie twoje kopie, a także komunikaty dziennika o błędzie / sukcesie, możesz wprowadzić funkcję „CopyFile”, która kopiowałaby dokładnie jeden plik i zapisywała swój wynik. W ten sposób twój główny kod składałby się z wywołań CopyFile i byłby łatwy do odczytania.

2. Możesz sprawić, by Twój logger był mądrzejszy. Zamiast przekazywać gigantyczny ciąg znaków, który ma wiele powtarzalnych informacji, możesz przekazać wartości wyliczeń, które uczynią wszystko bardziej zrozumiałym. Lub możesz zdefiniować bardziej wyspecjalizowane funkcje Log (), tj. LogFileCopy, LogDbInsert ... Cokolwiek często powtarzasz, rozważ uwzględnienie tego w jego własnej funkcji.

Jeśli wykonasz (1), możesz mieć kod, który wygląda następująco:

CopyFile( sOSDrive, 'Mapannotation.txt' )
CopyFile( sOSDrive, 'Mappoints.txt' )
CopyFile( sOSDrive, 'Mapsomethingelse.txt' )
. . . .

Następnie CopyFile () potrzebuje tylko kilku linii kodu, aby wykonać akcję i zapisać wynik, więc cały kod pozostaje zwięzły i łatwy do odczytania.

Trzymałbym się z dala od twojego podejścia nr 2, ponieważ oddzielasz informacje, które powinny pozostać razem w różnych modułach. Prosisz tylko, aby Twój główny kod zsynchronizował się z instrukcjami dziennika. Ale patrząc na LogMyMessage (5), nigdy się tego nie dowiesz.

AKTUALIZACJA (odpowiedź na komentarz): Nie znam dokładnego języka, którego używasz, więc ta część może wymagać nieco dostosowania. Wygląda na to, że wszystkie komunikaty w dzienniku identyfikują 3 rzeczy: komponent, działanie, wynik.

Myślę, że to prawie to, co sugeruje MainMa. Zamiast przekazywać rzeczywisty ciąg, zdefiniuj stałe (w C / C ++ / C #, będą one częścią typu wyliczania wyliczeniowego). Na przykład dla komponentów możesz mieć: DbInstall, AppFiles, Rejestr, Skróty ... Wszystko, co czyni kod mniejszym, ułatwi czytanie.

Byłoby to również pomocne, jeśli Twój język obsługuje przekazywanie parametrów zmiennych, nie jestem pewien, czy to możliwe. Na przykład jeśli akcja to „FileCopy”, możesz zdefiniować tę akcję, aby mieć dwa dodatkowe parametry użytkownika: nazwę pliku i katalog docelowy.

Twoje linie do kopiowania plików wyglądałyby mniej więcej tak:

Bool isSuccess = CopyFile(PChar(sTxtpath+'Mapannotation.txt'), PChar(sOSdrive+'\Mapannotation.txt'), False)
LogBook.Log( DbInstall, FileCopy, isSuccess, 'Mapannotation.txt', sOSDrive )

* Uwaga, nie ma również powodu, aby dwukrotnie kopiować / wklejać wiersz dziennika, jeśli można zapisać wynik operacji w osobnej zmiennej lokalnej i po prostu przekazać tę zmienną do Log ().

Widzisz tutaj motyw, prawda? Mniej powtarzalnego kodu -> bardziej czytelny kod.

Wygląda na to, że musisz wyodrębnić pojęcie „LoggableAction”. Widzę wzór w twoim przykładzie, w którym wszystkie połączenia zwracają wartość logiczną wskazującą sukces lub porażkę, a jedyną różnicą jest komunikat dziennika.

Minęły lata odkąd napisałem Delphi, więc jest to pseudo-kod inspirowany c #, ale pomyślałem, że chcesz czegoś takiego

void LoggableAction(FunctionToCallPointer, string logMessage)
{
    if(!FunctionToCallPointer)
    {  
        Log(logMessage).
    }
}

Wówczas twój kod telefoniczny staje się

if Not FileExists(sOSdrive+'\Mapannotation.txt') then
    LoggableAction(CopyFile(PChar(sTxtpath+'Mapannotation.txt'), "Oops, it went wrong")

Nie pamiętam składni Delphi dla wskaźników funkcji, ale bez względu na szczegóły implementacji, wydaje się, że szukasz jakiejś abstrakcji wokół procedury dziennika.

Jednym z możliwych podejść jest ograniczenie kodu za pomocą stałych.

if CopyFile(PChar(sTxtpath+'Mapannotation.txt'), PChar(sOSdrive+'\Mapannotation.txt'), False) then
   LogBook.Log(2,'[POSTGRESQL INSTALLATION] :  [ACTION]:copying Mapannotation.txt to '+sOSdrive+'\ sucessful')
   else
   LogBook.Log(2,'[POSTGRESQL INSTALLATION] :  [ACTION]:copying Mapannotation.txt to '+sOSdrive+'\ Failed');

stanie się:

if CopyFile(PChar(sTxtpath+'Mapannotation.txt'), PChar(sOSdrive+'\Mapannotation.txt'), False) then
   Log(2, SqlInstal, Action, CopyMapSuccess, sOSdrive)
   else
   Log(2, SqlInstal, Action, CopyMapFailure, sOSdrive)

który ma lepszy stosunek kodu dziennika / innego kodu podczas liczenia liczby znaków na ekranie.

Jest to bliskie temu, co zasugerowałeś w punkcie 2 twojego pytania, z tym wyjątkiem, że nie posunąłbym się tak daleko: Log(9257)jest oczywiście krótszy niż Log(2, SqlInstal, Action, CopyMapSuccess, sOSdrive), ale także dość trudny do odczytania. Co to jest 9257? Czy to jest sukces? Akcja? Czy jest to związane z SQL? Jeśli pracujesz nad tą bazą kodów przez ostatnie dziesięć lat, nauczysz się tych liczb na pamięć (jeśli istnieje logika, tj. 9xxx to kody sukcesu, x2xx są związane z SQL, itp.), Ale dla świeżego programisty, który odkryje baza kodów, krótkie kody będą koszmarem.

Możesz pójść dalej, mieszając dwa podejścia: użyj jednej stałej. Osobiście nie zrobiłbym tego. Albo twoje stałe będą rosły:

Log(Type2SuccessSqlInstallCopyMapSuccess, sOSdrive) // Can you read this? Really?

lub stałe pozostaną krótkie, ale niezbyt wyraźne:

Log(T2SSQ_CopyMapSuccess, sOSdrive) // What's T2? What's SSQ? Or is it S, followed by SQ?
// or
Log(CopyMapSuccess, sOSdrive) // Is it an action? Is it related to SQL?

Ma to również dwie wady. Będziesz musiał:

• Zachowaj osobną listę, łącząc informacje dziennika z odpowiednimi stałymi. Dzięki jednej stałej będzie szybko rosnąć.

• Znajdź sposób egzekwowania jednego formatu w swoim zespole. Na przykład, co jeśli zamiast tego T2SSQktoś zdecyduje się napisać ST2SQL?

Spróbuj wyodrębnić serię małych funkcji, aby obsłużyć wszystkie niechlujnie wyglądające rzeczy. Istnieje wiele powtarzających się kodów, które można bardzo łatwo zrobić w jednym miejscu. Na przykład:

procedure CopyIfFileDoesNotExist(filename: string);
var
   success: boolean;
begin
   if Not FileExists(sOSdrive+'\'+filename') then
   begin
      success := CopyFile(PChar(sTxtpath+filename), PChar(sOSdrive+filename), False);

      Log(filename, success);
   end;
end;

procedure Log(filename: string; isSuccess: boolean)
var
   state: string;
begin
   if isSuccess then
   begin
      state := 'success';
   end
   else
   begin
      state := 'failed';
   end;

   LogBook.Log(2,'[POSTGRESQL INSTALLATION] : [ACTION]:copying ' + filename + ' to '+sOSdrive+'\ ' + state);
end;

Sztuczka polega na sprawdzeniu każdego duplikatu w kodzie i znalezieniu sposobów na jego usunięcie. Używaj dużej ilości białych znaków i korzystaj z początku / końca na swoją korzyść (więcej białych znaków i łatwe do znalezienia / złożenia bloki kodu). To naprawdę nie powinno być zbyt trudne. Te metody mogą być częścią twojego rejestratora ... to naprawdę zależy od ciebie. Ale to wygląda na dobre miejsce do rozpoczęcia.

Powiedziałbym, że pomysł stojący za opcją 2 jest najlepszy. Myślę jednak, że kierunek, w którym podążyłeś, pogarsza sytuację. Liczba całkowita nic nie znaczy. Kiedy patrzysz na kod, zobaczysz, że coś jest rejestrowane, ale nie wiesz co.

Zamiast tego zrobiłbym coś takiego:

void logHelper(String phase, String message) {
   LogBook.Log(2, "[" + phase + "] :  [Action]: " + message);
}

Zachowuje to strukturę komunikatów, ale umożliwia elastyczność kodu. Możesz zdefiniować ciągi ciągłe według potrzeb dla faz i używać ich tylko jako parametru fazy. Pozwala to na zmianę faktycznego tekstu w jednym miejscu i na efekt. Inną zaletą funkcji pomocnika jest to, że ważny tekst jest w kodzie (tak jakby to był komentarz), ale tekst, który jest ważny tylko dla pliku dziennika, jest abstrakcyjny.

if (!FileExists(sOSdrive+'\Mapannotation.txt')) {
    if (CopyFile(PChar(sTxtpath+'Mapannotation.txt'), PChar(sOSdrive+'\Mapannotation.txt'), False)) {
       logHelper(POSTGRESQL, 'copying Mapannotation.txt to '+ sOSdrive +'\ sucessful')
    } else {
       logHelper(POSTGRESQL, 'copying Mapannotation.txt to '+ sOSdrive +'\ Failed');
    }
}

To nie jest coś, o czym wspomniałeś w swoim pytaniu, ale zauważyłem twój kod. Twoje wcięcie nie jest spójne. Przy pierwszym użyciu beginnie jest wcięty, ale po raz drugi tak jest. Robisz podobną rzecz else. Powiedziałbym, że jest to o wiele ważniejsze niż wiersze dziennika. Gdy wcięcie nie jest spójne, utrudnia skanowanie kodu i śledzenie przepływu. Wiele powtarzających się linii dziennika można łatwo odfiltrować podczas skanowania.

Co powiesz na coś wzdłuż tej linii:

LogBook.NewEntry( 2,'POSTGRESQL INSTALLATION', 'copying Mapannotation.txt to '+sOSdrive);

if CopyFile(PChar(sTxtpath+'Mapannotation.txt'), PChar(sOSdrive+'\Mapannotation.txt'), False) then
    LogBook.Success()
else
    LogBook.Failed();

Metoda NewEntry () budowałaby linię tekstu (w tym dodawanie [&] wokół odpowiednich pozycji) i wstrzymywała ją, aż zostaną wywołane metody success () lub failure (), które dopiszą wiersz do „success” lub „awaria”, a następnie wypisuje wiersz do dziennika. Możesz także zastosować inne metody, takie jak info (), gdy pozycja dziennika dotyczy czegoś innego niż sukces / porażka.