Krok 1 | długie metody – 5 kroków do zagłady
Zaczynamy niewinnie. Bo co złego może być w tym, że jakaś funkcja ma więcej niż kilka linijek? Każdy tak pisze! Poza tym to tylko na chwilę. Jak będzie wiadomo co gdzie przenieść to podzielę tę funkcję.
Wpis jest częścią cyklu „5 kroków do zagłady”. W tym momencie dostępne wpisy to:
Seria dostępna jest też w formie wideo na moim kanale w serwisie Youtube.
Co można powiedzieć o tego typu funkcji?
public long SaveUser(string firstName, string lastName, string email, string login, string password)
{
if (string.IsNullOrEmpty(firstName))
throw new ArgumentException();
if (string.IsNullOrEmpty(lastName))
throw new ArgumentException();
if (string.IsNullOrEmpty(email))
throw new ArgumentException();
if( string.IsNullOrEmpty(login))
throw new ArgumentException();
if (string.IsNullOrEmpty(password))
throw new ArgumentException();
using (var context = new DbContext())
{
if (context.Users.Any(x => x.Login == login))
throw ArgumentException();
var hashedPassword = Hash.Create(password);
var newUser = new User()
{
Login = login,
Email = email,
Password = hashedPassword,
FirstName = firstName,
LastName = lastName
};
context.Users.Add(newUser);
context.SaveChanges();
_mailService.Send(email,
"Potwierdź email",
string.Format("Aby potwierdzić swój adres email kliknij w ten link: https://korpo-soluszyn.com/users/confirm/{0}", user.Id));
return user.Id;
}
}
Wygląda niewinnie. Każdy widzi takie regularnie w projektach. Pewnie większość z nas też nieraz takie napisała. Poza tym – hej! – nie jest ona taka długa! Trochę ponad 30 linii? To Ty Marku chyba długich funkcji nie wiedziałeś! Masz rację. To tutaj to jakaś pierdoła, a nie duża funkcja. O czym my tu w ogóle mówimy. Do takich funkcji to my w tym jednym projekcie co to nasza firma przejęła dążymy! A Ty próbujesz wmówić nam, że są za długie…
Pomijam w kodzie powyżej, jak również w każdym kolejnym w tym tekście, pewne nieistotne dla zagadnienia szczegóły takie jak obecność klas czy inicjowania dodatkowych obiektów naokoło. Zaciemniały by one obraz i jeszcze bardziej rozciągały kod.
Problem
Zapewne część z Was zarzuci mi, że te wyjątki to są nie bardzo. Ale nie o to tutaj chodzi. Można było zwracać true/false. Albo jakiś status. Nieistotne.
Ważne w tym wszystkim jest to, że ta metoda JEST ZA DŁUGA. Wracając parę zdań wcześniej mówiliśmy, że ma przecież trochę ponad 30 linii. Więc gdzie jej do długich metod?
Prawda jest jednak taka, że to nie tylko ilość linii definiuje długość czy złożoność metody. Tak naprawdę po uwagę trzeba też wziąć to ile RÓŻNYCH operacji ona wykonuje. Skoro jest ich więcej to znak, że metoda jest za duża. Zobaczymy więc jakie operacje wykonuje nasza funkcja.
Lista zadań
Dodałem w kodzie komentarze oznaczającej kolejnymi cyframi kolejne operacje wykonywane w metodzie:
public void SaveUser(string firstName, string lastName, string email, string login, string password)
{
// === 1 ===
if (string.IsNullOrEmpty(firstName))
throw new ArgumentException();
if (string.IsNullOrEmpty(lastName))
throw new ArgumentException();
if (string.IsNullOrEmpty(email))
throw new ArgumentException();
if( string.IsNullOrEmpty(login))
throw new ArgumentException();
if (string.IsNullOrEmpty(password))
throw new ArgumentException();
// === 2 ===
using (var context = new DbContext())
{
// === 3 ===
if (context.Users.Any(x => x.Login == login))
throw ArgumentException();
// === 4 ===
var hashedPassword = Hash.Create(password);
// === 5 ===
var newUser = new User()
{
Login = login,
Email = email,
Password = hashedPassword,
FirstName = firstName,
LastName = lastName
};
// === 6 ===
context.Users.Add(newUser);
context.SaveChanges();
// === 7 ===
_mailService.Send(email,
"Potwierdź email",
// === 8 ===
string.Format("Aby potwierdzić swój adres email kliknij w ten link: https://korpo-soluszyn.com/users/confirm/{0}", user.Id));
return user.Id;
}
}
Opiszmy je sobie krótko:
- Walidacja, a więc sprawdzenie czy wszystkie dane, które przyszły są na poziomie struktury poprawne. Krótko mówiąc czy istnieją lub posiadają wartości z podanego zakresu itp.
- Utworzenie połączenia z bazą danych.
- Kolejne walidacja, tym razem bardziej biznesowa. Sprawdzamy już czy string istnieje. Tym razem sprawdzamy czy UŻYTKOWNIK nie jest już dodany.
- Dodatkowe przygotowanie danych do zapisu.
- Mapowanie na docelowy obiekt, których chcemy zapisywać. W tym miejscu składamy z pojedynczych wartości to co ma sens biznesowo.
- Zapisujemy użytkownika w bazie danych
- Wysyłamy maila, który daje użytkownikowi możliwość potwierdzenia podanego adresu email.
- Przygotowujemy treść maila opisanego w punkcie poprzednim.
Całkiem tego sporo, prawda?
Dlaczego to długa funkcja?
Jak widzisz ta funkcja robi 8 różnych operacji. Każda z nich wymaga od jednej do kilku linii. W funkcjach na 100+ linii też prawdopodobnie będzie wykonywana podoba ilość operacji. Po prostu każda z nich pewnie będzie wymagać np. 10-15 linii, zamiast kilku. Ale to co je łączy i to co jest tutaj istotne to to, że w obu przypadkach będą to zadania dotyczące różnych etapów przetwarzania danych albo innej dziedziny (np. zapis danych i wysyłanie powiadomień).
Dlaczego wrzucam funkcję na 30 linii i funkcję na 100+ linii do jednego worka „za duża funkcja”? Bo w obu przypadkach zadziałał ten sam mechanizm, obie są niepoprawne, powodują te same problemy. A ilość linii w tym wypadku jest po prostu zależna od tego jak rozwlekła operacja jest wykonywana. Bo jeżeli byśmy mieli np. bardziej złożone zapytanie Linq to mogłoby zajmować 20 linii. Kwestia chociażby wielości obiektów.
I takie spojrzenie na sprawę od razu prowadzi nas do pierwszego niekorzystnego efektu zostawienia takiej funkcji w kodzie.
Co kryje nazwa?
Chodzi o nazwę funkcji. Brzmi ona SaveUser() . I mając taką funkcję w kodzie ktoś mógłby pomyśleć, że po prostu zapisuje ona użytkownika. Więc warto pewnie najpierw sprawdzić te dane wcześniej czy są poprawne. Także ktoś korzystający z takiej funkcji albo dodający coś w jej pobliżu prawdopodobnie będzie skłonny napisać coś takiego:
if (string.IsNullOrEmpty(firstName))
return false;
if (string.IsNullOrEmpty(lastName))
return false;
if (string.IsNullOrEmpty(email))
return false;
if( string.IsNullOrEmpty(login))
return false;
if (string.IsNullOrEmpty(password))
return false;
SaveUser(firstName, lastName, email, login, password);
return true;
Nawet więcej, pewnie poszukałby albo dopisał też jakaś funkcję do sprawdzania czy użytkownik istnieje! W końcu kod ma być samodokumentujący. Programiści też nie wchodzą w każdą funkcję, której używają bo nie po to ktoś za nich już napisał jakiś fragment. Skoro jest zapis to funkcja zapisuje, resztą mam zająć się sam.
W taki wypadku aby być zgodnym z zawartością powinniśmy zmienić SaveUser() na ValidateAndCheckIfExistsAndSaveAndSendConfirmEmail() . Nie wygląda to zachęcająco, prawda? Ale oddaje to co robi funkcja! Problem jest tylko jeśli coś dodamy lub usuniemy z niej. Bo załóżmy, że mając taką opisową nazwę usuwamy z niej walidację bo przenieśliśmy ją gdzieś indziej. Ale nazwa została. W takim przypadku ktoś inny uzna, że nie trzeba robić dodatkowych kroków. Po prostu przepcha dane i będzie zachodził w głowę czemu wysypał się zapis do bazy, skoro miała być walidacja?
Reużywalność kodu
W jakim przypadku możemy użyć naszej funkcji? No w przypadku kiedy chcemy zapisać użytkownika ze wszystkimi danymi, który nie istniał, korzystając z domyślnej bazy danych i wymagając potwierdzenia adresu email. A co w przypadku kiedy wszystko się zgadza tylko klient ma też użytkowników do zaimportowania i nie powinny wtedy wysyłać się emaile bo już potwierdzili adres dawno temu? Nooo wtedy wystarczy napisać drugą taką funkcję bez tej jednej operacji. I nagle mamy w projekcie taki kod:
public long SaveUser(string firstName, string lastName, string email, string login, string password)
{
if (string.IsNullOrEmpty(firstName))
throw new ArgumentException();
if (string.IsNullOrEmpty(lastName))
throw new ArgumentException();
if (string.IsNullOrEmpty(email))
throw new ArgumentException();
if( string.IsNullOrEmpty(login))
throw new ArgumentException();
if (string.IsNullOrEmpty(password))
throw new ArgumentException();
using (var context = new DbContext())
{
if (context.Users.Any(x => x.Login == login))
throw ArgumentException();
var hashedPassword = Hash.Create(password);
var newUser = new User()
{
Login = login,
Email = email,
Password = hashedPassword,
FirstName = firstName,
LastName = lastName
};
context.Users.Add(newUser);
context.SaveChanges();
_mailService.Send(email,
"Potwierdź email",
string.Format("Aby potwierdzić swój adres email kliknij w ten link: https://korpo-soluszyn.com/users/confirm/{0}", user.Id));
return user.Id;
}
}
public long SaveUserWithoutConfirmationEmail(string firstName, string lastName, string email, string login, string password)
{
if (string.IsNullOrEmpty(firstName))
throw new ArgumentException();
if (string.IsNullOrEmpty(lastName))
throw new ArgumentException();
if (string.IsNullOrEmpty(email))
throw new ArgumentException();
if( string.IsNullOrEmpty(login))
throw new ArgumentException();
if (string.IsNullOrEmpty(password))
throw new ArgumentException();
using (var context = new DbContext())
{
if (context.Users.Any(x => x.Login == login))
throw ArgumentException();
var hashedPassword = Hash.Create(password);
var newUser = new User()
{
Login = login,
Email = email,
Password = hashedPassword,
FirstName = firstName,
LastName = lastName
};
context.Users.Add(newUser);
context.SaveChanges();
return user.Id;
}
}
Teraz dochodzi sytuacja kiedy jeszcze trzeba pozwolić zarejestrować użytkowników wykorzystywanych dla systemów wewnętrznych, którzy nie muszą mieć imienia i nazwiska albo adresu email. Wtedy wystarczy dodać kolejną wersję funkcji i dostajemy coś takiego:
public long SaveUser(string firstName, string lastName, string email, string login, string password)
{
if (string.IsNullOrEmpty(firstName))
throw new ArgumentException();
if (string.IsNullOrEmpty(lastName))
throw new ArgumentException();
if (string.IsNullOrEmpty(email))
throw new ArgumentException();
if( string.IsNullOrEmpty(login))
throw new ArgumentException();
if (string.IsNullOrEmpty(password))
throw new ArgumentException();
using (var context = new DbContext())
{
if (context.Users.Any(x => x.Login == login))
throw ArgumentException();
var hashedPassword = Hash.Create(password);
var newUser = new User()
{
Login = login,
Email = email,
Password = hashedPassword,
FirstName = firstName,
LastName = lastName
};
context.Users.Add(newUser);
context.SaveChanges();
_mailService.Send(email,
"Potwierdź email",
string.Format("Aby potwierdzić swój adres email kliknij w ten link: https://korpo-soluszyn.com/users/confirm/{0}", user.Id));
return user.Id;
}
}
public long SaveUserWithoutConfirmationEmail(string firstName, string lastName, string email, string login, string password)
{
if (string.IsNullOrEmpty(firstName))
throw new ArgumentException();
if (string.IsNullOrEmpty(lastName))
throw new ArgumentException();
if (string.IsNullOrEmpty(email))
throw new ArgumentException();
if( string.IsNullOrEmpty(login))
throw new ArgumentException();
if (string.IsNullOrEmpty(password))
throw new ArgumentException();
using (var context = new DbContext())
{
if (context.Users.Any(x => x.Login == login))
throw ArgumentException();
var hashedPassword = Hash.Create(password);
var newUser = new User()
{
Login = login,
Email = email,
Password = hashedPassword,
FirstName = firstName,
LastName = lastName
};
context.Users.Add(newUser);
context.SaveChanges();
return user.Id;
}
}
public long SaveUserWithoutNameAndWithoutConfirmationEmail(string firstName, string lastName, string email, string login, string password)
{
if( string.IsNullOrEmpty(login))
throw new ArgumentException();
if (string.IsNullOrEmpty(password))
throw new ArgumentException();
using (var context = new DbContext())
{
if (context.Users.Any(x => x.Login == login))
throw ArgumentException();
var hashedPassword = Hash.Create(password);
var newUser = new User()
{
Login = login,
Email = email,
Password = hashedPassword,
FirstName = firstName,
LastName = lastName
};
context.Users.Add(newUser);
context.SaveChanges();
return user.Id;
}
}
Mógłbym tak mnożyć kod w nieskończoność z różnymi zestawami pozostawionych operacji. Każda z tych funkcji, z racji łączenia wielu etapów pracy z danymi nie jest na tyle uniwersalna aby zmiany w otoczce procesu zapisu pozwalały jej nadal użyć.
Testy? A komu to potrzebne?
Podobno testy jednostkowe warto mieć. Nikt nie wie po co ale kierownik kazał pisać to piszemy. Nic trudnego. Wystarczy sprawdzić kilka scenariuszy. Co prawda musimy korzystać z prawdziwej bazy ale tak jest ok, w końcu w prawdziwym życiu też jest prawdziwa baza, co nie? Więc Mietek dawaj! Ładujemy te Asserty!
Wiem, że dla danych powinna być jakaś fabryka albo builder. Ale chciałem żeby kod był prostszy. Z resztą osoby, które piszą takie funkcje raczej nie użyją buildera ;)
I co my tutaj mamy:
public void WhenCorrectUserDataArePassed()
{
var id = SaveUser("Test", "Test", "test@test.pl", "login", "password");
var user = context.Users.Find(id);
Assert.IsNotNull(user);
Assert.Equals(user.FirstName, "Test");
Assert.Equals(user.LastName, "Test");
Assert.Equals(user.Email, "test@test.pl");
Assert.Equals(user.Login, "login");
Assert.Equals(user.Password, Hash.Create("password"));
}
[ExceptionExpected(typeof(ArgumentException))]
public void WhenEmptyUserFirstNameIsPassed()
{
var id = SaveUser("", "Test", "test@test.pl", "login", "password");
}
[ExceptionExpected(typeof(ArgumentException))]
public void WhenNullUserFirstNameIsPassed()
{
var id = SaveUser(null, "Test", "test@test.pl", "login", "password");
}
[ExceptionExpected(typeof(ArgumentException))]
public void WhenEmptyUserLastNameIsPassed()
{
var id = SaveUser("Test", "", "test@test.pl", "login", "password");
}
[ExceptionExpected(typeof(ArgumentException))]
public void WhenNullUserLastNameIsPassed()
{
var id = SaveUser("Test", null, "test@test.pl", "login", "password");
}
[ExceptionExpected(typeof(ArgumentException))]
public void WhenEmptyUserEmailIsPassed()
{
var id = SaveUser("Test", "Test", "", "login", "password");
}
[ExceptionExpected(typeof(ArgumentException))]
public void WhenNullUserEmailIsPassed()
{
var id = SaveUser("Test", "Test", null , "login", "password");
}
[ExceptionExpected(typeof(ArgumentException))]
public void WhenEmptyUserLoginIsPassed()
{
var id = SaveUser("Test", "Test", "test@test.pl", "", "password");
}
[ExceptionExpected(typeof(ArgumentException))]
public void WhenNullUserLoginIsPassed()
{
var id = SaveUser("Test", "Test", "test@test.pl" , null, "password");
}
[ExceptionExpected(typeof(ArgumentException))]
public void WhenEmptyUserPasswordIsPassed()
{
var id = SaveUser("Test", "Test", "test@test.pl", "login", "");
}
[ExceptionExpected(typeof(ArgumentException))]
public void WhenNullUserPasswordIsPassed()
{
var id = SaveUser("Test", "Test", "test@test.pl" , "login", null);
}
[ExceptionExpected(typeof(ArgumentException))]
public void WhenUserExists()
{
context.Users.Add(new User(){ Login = "login", Password = Hash.Create("password") });
context.SaveChanges();
var id = SaveUser("Test", "Test", "test@test.pl", "login", "password");
}
Ale przecież dodaliśmy też funkcje bez walidacji! Je też trzeba sprawdzić:
public void WhenCorrectUserDataArePassed()
{
var id = SaveUser("Test", "Test", "test@test.pl", "login", "password");
var user = context.Users.Find(id);
Assert.IsNotNull(user);
Assert.Equals(user.FirstName, "Test");
Assert.Equals(user.LastName, "Test");
Assert.Equals(user.Email, "test@test.pl");
Assert.Equals(user.Login, "login");
Assert.Equals(user.Password, Hash.Create("password"));
}
[ExceptionExpected(typeof(ArgumentException))]
public void WhenEmptyUserFirstNameIsPassed()
{
var id = SaveUser("", "Test", "test@test.pl", "login", "password");
}
[ExceptionExpected(typeof(ArgumentException))]
public void WhenNullUserFirstNameIsPassed()
{
var id = SaveUser(null, "Test", "test@test.pl", "login", "password");
}
[ExceptionExpected(typeof(ArgumentException))]
public void WhenEmptyUserLastNameIsPassed()
{
var id = SaveUser("Test", "", "test@test.pl", "login", "password");
}
[ExceptionExpected(typeof(ArgumentException))]
public void WhenNullUserLastNameIsPassed()
{
var id = SaveUser("Test", null, "test@test.pl", "login", "password");
}
[ExceptionExpected(typeof(ArgumentException))]
public void WhenEmptyUserEmailIsPassed()
{
var id = SaveUser("Test", "Test", "", "login", "password");
}
[ExceptionExpected(typeof(ArgumentException))]
public void WhenNullUserEmailIsPassed()
{
var id = SaveUser("Test", "Test", null , "login", "password");
}
[ExceptionExpected(typeof(ArgumentException))]
public void WhenEmptyUserLoginIsPassed()
{
var id = SaveUser("Test", "Test", "test@test.pl", "", "password");
}
[ExceptionExpected(typeof(ArgumentException))]
public void WhenNullUserLoginIsPassed()
{
var id = SaveUser("Test", "Test", "test@test.pl" , null, "password");
}
[ExceptionExpected(typeof(ArgumentException))]
public void WhenEmptyUserPasswordIsPassed()
{
var id = SaveUser("Test", "Test", "test@test.pl", "login", "");
}
[ExceptionExpected(typeof(ArgumentException))]
public void WhenNullUserPasswordIsPassed()
{
var id = SaveUser("Test", "Test", "test@test.pl" , "login", null);
}
[ExceptionExpected(typeof(ArgumentException))]
public void WhenUserExists()
{
context.Users.Add(new User(){ Login = "login", Password = Hash.Create("password") });
context.SaveChanges();
var id = SaveUser("Test", "Test", "test@test.pl", "login", "password");
}
public void WhenCorrectUserDataArePassedWithoutWalidation()
{
var id = SaveUser("Test", "Test", "test@test.pl", "login", "password");
var user = context.Users.Find(id);
Assert.IsNotNull(user);
Assert.Equals(user.FirstName, "Test");
Assert.Equals(user.LastName, "Test");
Assert.Equals(user.Email, "test@test.pl");
Assert.Equals(user.Login, "login");
Assert.Equals(user.Password, Hash.Create("password"));
}
public void WhenEmptyUserFirstNameIsPassedWithoutWalidation()
{
var id = SaveUser("", "Test", "test@test.pl", "login", "password");
var user = context.Users.Find(id);
Assert.IsNotNull(user);
Assert.Equals(user.FirstName, "");
Assert.Equals(user.LastName, "Test");
Assert.Equals(user.Email, "test@test.pl");
Assert.Equals(user.Login, "login");
Assert.Equals(user.Password, Hash.Create("password"));
}
public void WhenNullUserFirstNameIsPassedWithoutWalidation()
{
var id = SaveUser(null, "Test", "test@test.pl", "login", "password");
var user = context.Users.Find(id);
Assert.IsNotNull(user);
Assert.Equals(user.FirstName, "");
Assert.Equals(user.LastName, "Test");
Assert.Equals(user.Email, "test@test.pl");
Assert.Equals(user.Login, "login");
Assert.Equals(user.Password, Hash.Create("password"));
}
public void WhenEmptyUserLastNameIsPassedWithoutWalidation()
{
var id = SaveUser("Test", "", "test@test.pl", "login", "password");
var user = context.Users.Find(id);
Assert.IsNotNull(user);
Assert.Equals(user.FirstName, "Test");
Assert.Equals(user.LastName, "");
Assert.Equals(user.Email, "test@test.pl");
Assert.Equals(user.Login, "login");
Assert.Equals(user.Password, Hash.Create("password"));
}
public void WhenNullUserLastNameIsPassedWithoutWalidation()
{
var id = SaveUser("Test", null, "test@test.pl", "login", "password");
var user = context.Users.Find(id);
Assert.IsNotNull(user);
Assert.Equals(user.FirstName, "Test");
Assert.Equals(user.LastName, "");
Assert.Equals(user.Email, "test@test.pl");
Assert.Equals(user.Login, "login");
Assert.Equals(user.Password, Hash.Create("password"));
}
public void WhenEmptyUserEmailIsPassedWithoutWalidation()
{
var id = SaveUser("Test", "Test", "", "login", "password");
var user = context.Users.Find(id);
Assert.IsNotNull(user);
Assert.Equals(user.FirstName, "Test");
Assert.Equals(user.LastName, "Test");
Assert.Equals(user.Email, "");
Assert.Equals(user.Login, "login");
Assert.Equals(user.Password, Hash.Create("password"));
}
public void WhenNullUserEmailIsPassedWithoutWalidation()
{
var id = SaveUser("Test", "Test", null , "login", "password");
var user = context.Users.Find(id);
Assert.IsNotNull(user);
Assert.Equals(user.FirstName, "Test");
Assert.Equals(user.LastName, "Test");
Assert.Equals(user.Email, "");
Assert.Equals(user.Login, "login");
Assert.Equals(user.Password, Hash.Create("password"));
}
[ExceptionExpected(typeof(DbUpdateException))]
public void WhenEmptyUserLoginIsPassedWithoutWalidation()
{
var id = SaveUser("Test", "Test", "test@test.pl", "", "password");
}
[ExceptionExpected(typeof(DbUpdateException))]
public void WhenNullUserLoginIsPassedWithoutWalidation()
{
var id = SaveUser("Test", "Test", "test@test.pl" , null, "password");
}
[ExceptionExpected(typeof(DbUpdateException))]
public void WhenEmptyUserPasswordIsPassedWithoutWalidation()
{
var id = SaveUser("Test", "Test", "test@test.pl", "login", "");
}
[ExceptionExpected(typeof(DbUpdateException))]
public void WhenNullUserPasswordIsPassedWithoutWalidation()
{
var id = SaveUser("Test", "Test", "test@test.pl" , "login", null);
}
[ExceptionExpected(typeof(ArgumentException))]
public void WhenUserExistsWithoutWalidation()
{
context.Users.Add(new User(){ Login = "login", Password = Hash.Create("password") });
context.SaveChanges();
var id = SaveUser("Test", "Test", "test@test.pl", "login", "password");
}
Dla każdego przypadku trzeba by dopisać kolejną porcję testów bo to są wtedy inne funkcje i ktoś mógł coś zmienić w jednej nie ruszając drugiej. Także testowanie walidacji czy sprawdzania czy użytkownik istnieje za każdym razem jest konieczne.
Ale ludzie! Tyle pisania?! Po co nam to było…
To się w żaden sposób nie skaluje. I zakładając, że nie zmienimy myślenia to albo najpierw porzucimy ideę testów albo zaczniemy pchać ify do funkcji, żeby uniknąć testowania tych samych fragmentów kilka razy.
Przyczyna
Jak widzisz takie funkcje generują masę problemów. Skąd jednak się biorą?
Moja teoria ale także praktyka mówią, że przyczyny są zazwyczaj podobne i mogę wymienić dwie główne:
Tylko na chwilę…
Pierwsza z nich to zostawienie kodu, który miał być tylko na chwilę. Bo coś akurat implementowaliśmy od nowa i nie byliśmy pewni jak to podzielić. Ale w tym wszystkim przenieśliśmy się w kolejne miejsce, commit poszedł, code review nie wyłapało i tak już zostało. Sytuacja nie taka rzadka. Zwłaszcza jak pracujemy pod presją deadlineów. „Teraz to tylko ma działać, a później poprawimy”. I to później nigdy nie przychodzi. Już nieraz widziałem w kodzie zostawione komentarze TODO, że rozwiązanie jest tymczasowe. A patrzysz na datę ostatnich zmian i było to 5 lat temu.
Tutaj sprawa jest prosta. Jest to zaniedbanie połączone ze złą organizacją. Na początku wygląda niewinnie. Bo widzimy problem i przecież wiemy, że teraz musimy to zostawić ale jak tylko pojawi się okazja to OD RAZU to zmienimy. Jeszcze nie teraz bo teraz to wdrożenie jest. I klient chce dostać nowy feature do testów. I teraz to trzeba wdrożyć nowego kolegę. Jeszcze chwila tylko…
Przecież to się łączy w całość
Druga przyczyna jest trochę trudniejsza do wytłumaczenia ale równie istotna. Chodzi mianowicie o sposób myślenia przy implementacji i podejście do zadania. Ten punkt raczej dotyczy młodych stażem programistów, który głównie opierają wiedzę o tutoriale. Ale nie tylko. Bo i starsi stażem potrafią mieć takie złe podejście.
Chodzi mianowicie o pewne podejście do tworzenia kodu. Przez brak wiedzy albo doświadczenia zaczynamy pisać strukturalny kod w obiektowym języku. Zamykamy większość kodu dotyczącego jednej funkcjonalności w obrębie jednej funkcji. Nasza funkcja staje się takim hubem, który jest centralnym punktem zadania. Zamiast podejść do tego w bardziej modularny sposób.
Tego typu rozbudowane funkcje powstają kiedy nie widzimy, że większość operacji to tak naprawdę zbiór pewnych klocków. Które musimy połączyć ale też dać im pewną autonomię. Bo co moglibyśmy zrobić z klocków Lego gdyby każdy klocek miał jedno konkretne zastosowanie? Np. byłby klocek „samochód”, klocek „ściana wysoka na 4 klocki w kolorze czerwonym”, klocek „stolik ogrodowy z różową parasolką, dwoma kubkami, ławeczką i jednym ludzikiem”? A właśnie tak wyglądają te funkcje. W konkretnym zastosowaniu wyglądają na pierwszy rzut oka sensownie. Ale nie dają za wielkiego pola do manewru.
Obraz sytuacji tracimy tutaj też kiedy piszemy kod od wejścia do wyjścia. Czyli dostajemy dane od użytkownika i myślimy sobie „sprawdzę czy są ok”, potem „zanim coś zrobię to jeszcze trzeba je zmapować”, „ok, zapisujemy”, „no i mail”. W ten sposób cały proces układa nam się automatycznie w listę instrukcji. A lista instrukcji naturalnie pasuje do funkcji.
Rozwiązanie
Rozwiązanie w tym wypadku może wydawać się trudne to znalezienia. Można by powiedzieć po prostu, że zwyczajnie musisz to wkuć na pamięć.
Jednak wg mnie są dwie rzeczy, które zdecydowanie ułatwią Ci ujarzmienie wielkości funkcji i pomogą wyrobić sobie lepsze nawyki.
Testy najpierw
Pierwsza rzecz, która na pewno otworzy Ci oczy to próba zastosowania TDD. Zacznij od napisania testów do tego co będziesz chciał zaimplementować. Zaczynają Ci się mnożyć przypadki testowe albo kopiujesz jakieś testy zmieniając tylko nazwę funkcji i jakiś parametr? Świetnie, bo właśnie znalazłeś miejsca, które trzeba wydzielić tak żeby przetestować raz. Jeszcze lepiej to zadziała jak napiszesz te testy, potem dodasz faktycznie funkcję i coś będziesz musiał w tych testach zmienić bo np. nie doczytałeś o jednym parametrze. Jednorazowe przejście po 200 asercjach da do myślenia.
Poświęcisz tutaj na pewno sporo czasu na testy i jest to raczej podejście, które nie zadziała jak macie crunch i nie ma czasu na takie rzeczy. Ale na pewno pomoże w drugim przypadku kiedy nie widziałeś wcześniej problemu. A zaczynając od testów nie da się tego tak zostawić. Chyba, że jesteś bardzo cierpliwy, wydziergałeś te setki asercji i i tak nie przyszło Ci do głowy, że coś jest nie tak. Ale wtedy nie ma ratunku. We wszystkich innych przypadkach wszystko przemyślałeś na podstawie testów, kiedy sama funkcja jeszcze nie istniała. Więc nie masz czego zostawić do późniejszego poprawienia bo dopiero musisz to napisać. A głupotą by było zignorowanie wszystkich wniosków jakie właśnie wyciągnąłeś, prawda?
Zacznijmy od końca
Drugi sposób na ułatwienie sobie ogarnięcia problemu rozbudowanych funkcji można z powodzeniem użyć także przy większej presji czasowej. Nadal może on wygenerować kod, który nie jest idealny, ale na pewno nie będzie to kod tak trudny w utrzymaniu lub rozbudowie jak wcześniej.
Chodzi mianowicie o to żeby zacząć od centrum zadania. Piszemy najpierw kod, który ma być ostatecznym, centralnym punktem tego co akurat robimy. W przykładzie z tego wpisu jest to zapis użytkownika. Więc co musimy zrobić żeby zapisać użytkownika do bazy? Ano tyle (w przykładzie kontekst bazy danych powinien przyjść np. z kontenera DI ale znowu upraszczam kod):
public long SaveUser(User user)
{
using (var context = new DbContext())
{
context.Users.Add(user);
context.SaveChanges();
return user.Id;
}
}
I już mamy całą funkcję, która robi to co mówi jej nazwa – zapisuje użytkownika. Możemy napisać do niej testy.
Ale chwila. Jakoś muszę ten obiekt użytkownika utworzyć! No to mam następne zadanie czyli następną funkcję:
public User CreateUser(string firstName, string lastName, string email, string login, string password)
{
var user = new User()
{
Login = login,
Email = email,
Password = hashedPassword,
FirstName = firstName,
LastName = lastName
};
return user;
}
Powinno być przez konstruktor to załatwione ale i tak jest lepiej.
Ok, ale co jak on już istnieje? To sprawdźmy to. Jak? Osobną funkcją!
public bool UserExists(string login)
{
using (var context = new DbContext())
{
return context.Users.Any(x => x.Login == login);
}
}
I tak dalej. I tak dalej.
Ważne jest to abyś zwrócił uwagę, że każda myśl jaka mi wpadła do głowy prowadziła do powstania funkcji. Na koniec tylko wywołam sobie:
if (!ValidateUserData(firstName, lastName, email, login, password) || UserExists(login))
return Error();
var id = SaveUser(CreateUser(firstName, lastName, email, login, password));
SendConfirmationEmail(id, email);
I nie poświęciłem zauważalnie więcej czasu, a mam kod, którym mogę do woli żonglować. Bo jak nagle potrzebuję gdzieś wszystko poza walidacją to mam gotowe funkcje. Jak potrzebuję inaczej sprawdzać czy user istnieje bo np. porównywać emaile to wiem gdzie to się dzieje. Nie muszę już w tym miejscu wysyłać emaila? Nie wywołuję po prostu jednej funkcji!
Nawet jak te wszystkie funkcje umieścisz w jednej klasie i nawet jak ich się zrobi kilkanaście to i tak jesteś na dużo lepszej drodze do zachowania czystości kodu. Bo masz już elastyczność zmian. Teraz możecie powoli, nawet po jednej funkcji, przenosić fragmenty wręcz automatycznie bo „wyciągnij funkcję do osobnej klasy” to coś co na pewno znajdzie się w każdym IDE.
Leave a Comment