Czy uznanie za błędne praktyki umieszczenia numeru błędu w nazwie metody w celu tymczasowego obejścia?

Mój współpracownik, który jest starszym facetem, blokuje mnie podczas przeglądu kodu, ponieważ chce, żebym nazwał metodę „PerformSqlClient216147Workaround”, ponieważ jest to obejście jakiegoś defektu ###. Teraz moja propozycja nazwy metody jest podobna do PerformRightExpressionCast, która ma tendencję do opisywania tego, co faktycznie robi metoda. Jego argumenty są następujące: „Cóż, ta metoda jest używana tylko jako obejście tego przypadku i nigdzie indziej”.

Czy umieszczenie numeru błędu w nazwie metody tymczasowego obejścia byłoby uważane za złą praktykę?

Nie nazwałbym tej metody tak, jak sugerował twój współpracownik. Nazwa metody powinna wskazywać, co robi metoda. Nazwa PerformSqlClient216147Workaroundtaka nie wskazuje, co robi. Jeśli już, użyj komentarzy opisujących metodę, aby wspomnieć, że jest to obejście. Może to wyglądać następująco:

/**
 * Cast given right-hand SQL expression.
 *
 * Note: This is a workaround for an SQL client defect (#216147).
 */
public void CastRightExpression(SqlExpression rightExpression)
{
    ...
}

Zgadzam się z MainMa, że numery błędów / defektów nie powinny pojawiać się w samym kodzie źródłowym, ale raczej w komentarzach kontroli źródła, ponieważ są to metadane, ale nie jest straszne, jeśli pojawiają się w komentarzach do kodu źródłowego. Numery błędów / defektów nigdy nie powinny być używane w nazwach metod.

Nic nie jest bardziej trwałe niż tymczasowa poprawka, która działa.

Czy jego sugestia wygląda dobrze za 10 lat? Kiedyś powszechną praktyką było komentowanie każdej zmiany z naprawioną wadą. Niedawno (podobnie jak w ciągu ostatnich 3 dekad) komentowanie tego stylu jest powszechnie akceptowane jako ograniczenie możliwości utrzymania kodu - i to za pomocą samych komentarzy, a nie nazw metod.

To, co proponuje, jest przekonującym dowodem na to, że twoje systemy kontroli jakości i kontroli jakości są znacznie wadliwe. Śledzenie wad i napraw defektów należy do systemu śledzenia defektów, a nie do kodu źródłowego. Śledzenie zmian w kodzie źródłowym należy do systemu kontroli źródła. Odsyłacze między tymi systemami umożliwiają śledzenie defektów w kodzie źródłowym .....

Kod źródłowy jest dostępny na dziś, nie wczoraj i nie jutro (ponieważ nie wpisujesz w źródle tego, co planujesz zrobić w przyszłym roku) ...

Więc to jest tymczasowe rozwiązanie? Następnie użyj nazwy sugerowanej przez recenzenta, ale oznacz metodę jako przestarzałą, aby użycie jej generowało ostrzeżenie za każdym razem, gdy ktoś kompiluje kod.

Jeśli nie, zawsze możesz powiedzieć, że 216147to nie ma sensu w kodzie, ponieważ kod nie jest powiązany z systemem śledzenia błędów (jest to raczej system śledzenia błędów, który jest powiązany z kontrolą źródła). Kod źródłowy nie jest dobrym miejscem na odniesienia do zgłoszeń błędów i wersji, a jeśli naprawdę musisz tam umieścić te odniesienia, zrób to w komentarzach.

Pamiętaj, że nawet w komentarzach sam numer błędu nie jest zbyt cenny. Wyobraź sobie następujący komentarz:

public IEnumerable<Report> FindReportsByDateOnly(DateTime date)
{
    // The following method replaces FindReportByDate, because of the bug 8247 in the
    // reporting system.
    var dateOnly = new DateTime(date.Year, date.Month, date.Day);
    return this.FindReportByDate(dateOnly);
}

private IEnumerable<Report> FindReportsByDate(DateTime date)
{
    Contract.Requires(date.Hour == 0);
    Contract.Requires(date.Minute == 0);
    Contract.Requires(date.Second == 0);

    // TODO: Do the actual work.
}

Wyobraź sobie, że kod został napisany dziesięć lat temu, że właśnie dołączyłeś do projektu i że kiedy zapytałeś, gdzie możesz znaleźć informacje o błędzie 8247, twoi koledzy powiedzieli, że na stronie internetowej znajduje się lista błędów raportowanie oprogramowania systemowego, ale strona została przebudowana pięć lat temu, a nowa lista błędów ma różne numery.

Wniosek: nie masz pojęcia, o co chodzi w tym błędzie.

Ten sam kod mógł zostać napisany w nieco inny sposób:

public IEnumerable<Report> FindReportsByDateOnly(DateTime date)
{
    // The reporting system we actually use is buggy when it comes to searching for a report
    // when the DateTime contains not only a date, but also a time.
    // For example, if looking for reports from `new DateTime(2011, 6, 9)` (June 9th, 2011)
    // gives three reports, searching for reports from `new DateTime(2011, 6, 9, 8, 32, 0)`
    // (June 9th, 2011, 8:32 AM) would always return an empty set (instead of isolating the
    // date part, or at least be kind and throw an exception).
    // See also: http://example.com/support/reporting-software/bug/8247
    var dateOnly = new DateTime(date.Year, date.Month, date.Day);
    return this.FindReportsByDate(dateOnly);
}

private IEnumerable<Report> FindReportsByDate(DateTime date)
{
    Contract.Requires(date.Hour == 0);
    Contract.Requires(date.Minute == 0);
    Contract.Requires(date.Second == 0);

    // TODO: Do the actual work.
}

Teraz masz jasny obraz problemu. Nawet jeśli wydaje się, że link hipertekstowy na końcu komentarza jest martwy pięć lat temu, nie ma to znaczenia, ponieważ nadal możesz zrozumieć, dlaczego FindReportsByDatezostał zastąpiony FindReportsByDateOnly.

Uważam wtedy, że są PerformSqlClient216147Workaroundbardziej pouczające PerformRightExpressionCast. W nazwie funkcji nie ma żadnych wątpliwości co do tego, co robi, dlaczego to robi i jak uzyskać więcej informacji na jej temat. Jest to wyraźna funkcja, która będzie bardzo łatwa do przeszukiwania w kodzie źródłowym.

Dzięki systemom śledzenia błędów ten numer jednoznacznie identyfikuje problem, a kiedy podnosisz ten błąd w systemie, zapewnia on wszystkie potrzebne szczegóły. Jest to bardzo mądra rzecz do zrobienia w kodzie źródłowym i pozwoli zaoszczędzić czas przyszłym programistom, próbując zrozumieć, dlaczego wprowadzono zmianę.

Jeśli widzisz wiele z tych nazw funkcji w kodzie źródłowym, to problem nie tkwi w konwencji nazewnictwa. Problem polega na tym, że masz wadliwe oprogramowanie.

Sugestia twojego współpracownika mają wartość; zapewnia identyfikowalność poprzez powiązanie zmian w kodzie z powodem, udokumentowanym w bazie danych błędów pod tym numerem zgłoszenia, dlaczego zmiana została wprowadzona.

Sugeruje to jednak, że jedynym powodem istnienia tej funkcji jest obejście błędu. Że jeśli problem nie byłby problemem, nie chciałbyś, aby oprogramowanie to robiło. Przypuszczalnie chcesz , aby oprogramowanie działało, więc nazwa funkcji powinna wyjaśniać, co robi i dlaczego domena problemowa wymaga tego; nie dlaczego baza danych błędów tego potrzebuje. Rozwiązanie powinno wyglądać jak część aplikacji, a nie jak opaska.

Myślę, że zarówno ty, jak i on, wyłożyliśmy to wszystko na proporcje.

Zgadzam się z twoim argumentem technicznym, ale na koniec dnia nie zrobi to aż tak dużej różnicy, szczególnie jeśli jest to tymczasowa poprawka, którą można usunąć z bazy kodu w ciągu kilku dni / tygodni / miesięcy.

Myślę, że powinieneś włożyć swoje ego z powrotem do jego pudła i pozwolić mu mieć własną drogę. (Gdyby też słuchał, powiedziałbym, że musicie iść na kompromis. Oba ego wracają do swoich pudełek.)

Tak czy inaczej, ty i on mamy lepsze rzeczy do zrobienia.

Czy umieszczenie numeru błędu w nazwie metody tymczasowego obejścia byłoby uważane za złą praktykę?

Tak.

Idealnie byłoby, gdyby twoja klasa najlepiej odwzorowała / wdrożyła koncepcję w przepływie / stanie twojej aplikacji. Nazwy interfejsów API tej klasy powinny odzwierciedlać to, co robią ze stanem klasy, aby kod klienta mógł z łatwością korzystać z tej klasy (tj. Nie podawać nazwy, która dosłownie nic nie znaczy, chyba że o niej przeczytasz).

Jeśli część publicznego API tej klasy zasadniczo mówi „wykonaj operację Y opisaną w dokumencie / lokalizacji X”, to zdolność innych osób do zrozumienia API będzie zależeć od:

1. wiedząc, czego szukać w zewnętrznej dokumentacji

2. wiedzą, gdzie szukać zewnętrznej dokumentacji

3. oni poświęcają czas i wysiłek i faktycznie patrzą.

Z drugiej strony, nazwa metody nawet nie wspomina, gdzie „lokalizacja X” jest dla tej wady.

Zakłada tylko (bez żadnego realistycznego powodu), że każdy, kto ma dostęp do kodu, ma również dostęp do systemu śledzenia defektów i że system śledzenia będzie istniał tak długo, jak długo będzie istniał stabilny kod.

Przynajmniej jeśli wiesz, że wada będzie zawsze dostępna w tej samej lokalizacji i to się nie zmieni (np. Numer wady Microsoft, który był pod tym samym adresem URL przez ostatnie 15 lat), powinieneś podać link do problem w dokumentacji interfejsu API.

Mimo to mogą istnieć obejścia innych defektów, które wpływają na więcej niż interfejs API jednej klasy. Co wtedy zrobisz

Posiadanie interfejsów API z tym samym numerem defektu w wielu klasach ( data = controller.get227726FormattedData(); view.set227726FormattedData(data);tak naprawdę niewiele mówi, a jedynie sprawia, że ​​kod jest bardziej niejasny.

Możesz zdecydować, że wszystkie inne defekty zostaną rozwiązane za pomocą nazw opisowych dla operacji ( data = controller.getEscapedAndFormattedData(); view.setEscapedAndFormattedData(data);), z wyjątkiem przypadku defektu 216147 (który łamie zasadę „najmniej zaskoczenia” - lub jeśli chcesz to ująć w ten sposób, to zwiększa pomiar „WTF / LOC” twojego kodu).

TL; DR: Zła praktyka! Idź do swojego pokoju!

Głównymi celami programisty powinien być działający kod i przejrzystość wypowiedzi.

Nazywanie metody obejścia (.... Obejście). Wydaje się, że udało się osiągnąć ten cel. Mam nadzieję, że na pewnym etapie problem zostanie naprawiony, a metoda obejścia zostanie usunięta.

Dla mnie nazwanie metody po błędzie sugeruje, że błąd nie został rozwiązany lub nie zidentyfikowano przyczyny źródłowej. Innymi słowy, sugeruje, że wciąż jest nieznane. Jeśli pracujesz nad błędem w systemie innej firmy, to obejście jest rozwiązaniem, ponieważ znasz przyczynę - po prostu nie może lub nie może tego naprawić.

Jeśli część interakcji z SqlClient wymaga wykonania prawidłowego rzutowania wyrażenia, kod powinien brzmieć „performRightExpressionCast ()”. Zawsze możesz skomentować kod, aby wyjaśnić dlaczego.

Ostatnie 4 i pół roku spędziłem na utrzymywaniu oprogramowania. Jedną z rzeczy, która sprawia, że ​​kod jest mylący w zrozumieniu podczas wskakiwania, jest kod napisany w sposób, który wynika wyłącznie z historii. Innymi słowy, nie byłoby tak, gdyby był napisany dzisiaj. Nie chodzi mi o jakość, ale o punkt widzenia.

Mój współpracownik powiedział kiedyś: „Naprawiając defekt, zrób kod tak, jak powinien”. Sposób, w jaki interpretuję to „Zmień kod tak, aby wyglądał, gdyby ten błąd nigdy nie istniał”.

Konsekwencje:

1. Zwykle w rezultacie mniej kodu.
2. Prosty kod
3. Mniej odniesień do błędów w kodzie źródłowym
4. Mniejsze ryzyko przyszłej regresji (po pełnej weryfikacji zmiany kodu)
5. Łatwiejsze do analizy, ponieważ programiści nie muszą obciążać się historią uczenia się, która nie jest już aktualna.

Kod źródłowy nie musi mi mówić, jak osiągnął swój obecny stan. Kontrola wersji może mi powiedzieć historię. Potrzebuję kodu źródłowego, aby był w stanie niezbędnym do pracy. To powiedziawszy, sporadyczny komentarz „// błąd 12345” nie jest złym pomysłem, ale jest nadużywany.

Więc decydując, czy nazwać metodę „PerformSqlClient216147Workaround”, zadaj sobie następujące pytania:

1. Gdybym wiedział o błędzie 216147 przed napisaniem kodu, jak bym sobie z tym poradził?
2. Jakie jest obejście? Jeśli ktoś, kto nigdy wcześniej nie widział tego kodu, mógł go obejrzeć, czy byłby w stanie go stosować? Czy sprawdzenie systemu śledzenia błędów jest konieczne, aby wiedzieć, jak działa ten kod?
3. Jak tymczasowy jest ten kod? Z mojego doświadczenia wynika, że ​​tymczasowe rozwiązania zwykle stają się trwałe, jak wskazuje @mattnz.