Witam.
Prosiłbym Was o code review aplikacji do skracania adresów Czym więcej feedbacku, tym lepiej!
https://github.com/selfmadeking/URL-shortener-Symfony-4

Kurcze, aż szkoda żeby taki fajny temat się zmarnował, więc choć się nie znam, to pierwszy rzucę kamieniem - może bardziej doświadczeni koledzy się przyłączą ;-)

Po pierwsze bardzo nie podoba mi się brak typowania zmiennych i pól w klasie.

[PHP] pobierz, plaintext 
private $urlGeneratorService;
    private $em;
 
    public function __construct(UrlGeneratorService $urlGeneratorService, EntityManagerInterface $em)
    {
        $this->urlGeneratorService = $urlGeneratorService;
        $this->em = $em;
    }
[PHP] pobierz, plaintext 

Dlaczego pola klasy nie mają docka z typami? Przecież korzystasz z phpstorma, a on potrafi wygenerować - stajesz na parametrach konstruktora i klikasz alt+enter....

Większość funkcji nie ma typowania zmiennych na wejściu, ale jednocześnie w niektórych miejscach masz typowanie na wyjściu, czyli używasz php 7.1+ - brakuje konsekwencji. W każdym razie mz w jakiś sposób trzeba parametry opisać - albo phpdock albo typowanie.

Zaciekawiła mnie funkcja, którą wezmę jako przykład na warsztat.

[PHP] pobierz, plaintext 
public function userIsAuthorOfList($list)
    {
        $urls = $list->getListOfUrls();
        $userId = $this->security->getUser();
        if (isset($userId)) {
            $userId = $userId->getId();
        }
        $userIdOfUrl = $urls->first()->getUserId();
        if (isset($userIdOfUrl)) {
            $userIdOfUrl = $userIdOfUrl->getId();
        }
 
        return (bool) $userIdOfUrl == $userId;
 
    }
[PHP] pobierz, plaintext 

$list - nie jest typowana na wejsciu. Co to jest - jakiś obiekt, zmienna skalarna, tablica? Lista czego?
Dalej w tej samej funkcji masz:

[PHP] pobierz, plaintext 
$userId = $this->security->getUser();
        if (isset($userId)) {
            $userId = $userId->getId();
        }
 
[PHP] pobierz, plaintext 

Czyli co zmienna $userId jest tak na prawdę obiektem User, ale za chwilę zamieniasz ją na skalarne Id? Zupełnie bez sensu bo kompletnie psuje czytelność kodu.

W tej funkcj imasz coś takiego:

[PHP] pobierz, plaintext 
$userIdOfUrl = $urls->first()->getUserId();
[PHP] pobierz, plaintext 

Co tak na prawdę jest $userIdOfUrl = $list->getListOfUrls()->first()->getUserId();
Poczytaj o Lod (Law of Demeter) - raczej powinno być coś w stylu $list->getUser() lub $list->getUserId(); Zresztą nie sprawdzałem, ale mogę się założyć, że w innych częściach systemu robisz coś podobnego, aby sprawdzić użytkownika listy = duplika kodu, czyli tym bardziej warto to zrefaktoryzować.

Po co rzutujesz wartość bool do bool w tym miejscu? To chyba zbędne.

[PHP] pobierz, plaintext 
return (bool) $userIdOfUrl == $userId;
[PHP] pobierz, plaintext 

Ogólnie ta funkcja to idealny kandydat do refactoringu na wielu płaszczyznach. Powinno się ją sprowadzić do 2-3 linijek.

Kilka luźnych uwag:
- servisy nie powinny raczej zapisywać niczego do flash - to rola kontrolera - serwis może być wykorzystywany w inny sposób (np. api/cli itp).
- logując wyjątek logujesz tylko sam message - mało to przydatne przy debugowaniu lepjej zrobić $logger->error((string)$e)
- w symfony if ($form->isSubmited() && $form->isValid()) możesz zastąpić if ($form->isValid()) - bo metoda isValid sprawdza między innymi czy form jest wysłany.


To takie moje 0,05pln żaby rozpocząć temat. Niestety nie mam teraz czasu na wgryzienie się w Twój system i pisanie czegoś więcej o logice itp, dlatego tylko kilka uwag co do samego sposobu kodowania. Dodam, że raczej mistrzem kodowania nie jestem, więc traktuj moje uwagi jako wymianę zdań.

Jedna uwaga do Form->isValid. Owszem sprawdza ale rzuca exception. Nawet sama treść wskazuje aby używać wpierw `isSubmitted()`
https://github.com/symfony/form/blob/master/Form.php#L736

No proszę, człowiek uczy się całe życie. Zawsze dla czytelności robiłem if (isPost) {} i dlatego ładnie działało samo isValid(), a tutaj faktycznie kolega tego nie robi i u niego wywali wyjątek.

Jak powyżej, np. taka metoda:

[PHP] pobierz, plaintext 
    public function validate($value, Constraint $constraint)
    {
        if (null === $value || '' === $value) {
            return;
        }
        if (!is_string($value)) {
            throw new UnexpectedValueException($value, 'url');
        }
        $pattern = '/^(https?:\/\/)?([\da-z\.-]+)\.([a-z\.]{2,6})([\/\w \.-]*)*\/?/';
        if (!preg_match( $pattern, $value, $matches)) {
            $this->context->buildViolation($constraint->message)
                ->setParameter('{{ url }}', $value)
                ->addViolation();
        }
    }
[PHP] pobierz, plaintext 

1. Metoda może rzucić wyjątek, zwrócić null albo być typu void - za dużo
2. Jak już oczekujemy stringa, to czemu nie użyjemy typizowania?
3. Po to jest walidator, żeby nie było wyjątków
4. Zachęcam do używania funkcji "empty" - sprawdza kilka rzeczy za jednym zamachem
5. I nie ma żadnych wbudowanych lub gotowych bibliotek do walidowania URLi?
6. Dobrą praktyką też jest używać return tylko raz i najlepiej na samym końcu metody, poprawia się czytelność i ułatwia debugowanie/rozbudowę, jeśli metoda co chwila nie przerywa swojego biegu w jakiś pozagnieżdżanych ifach

Kolejne rzeczy, które na pierwszy rzut oka mi się nie podobają:
- użycie migracji (to jest dobre gdy projekt jest na produkcji, tutaj nie ma takiej potrzeby, wystarczy wygenerować schemat z encji i użyć fikstur)
- wrzucanie do repo zakomentowanego kodu
- brak instrukcji instalacji apki w readme
- dużo kodu w kontrolerach i używanie ich jak serwisów (np. wystarczy serwis do redirectów zamiast tych dwóch kontrolerów, nie ma też większego sensu tworzyć kontrolera tylko dla jednej metody)
- zbyt opasłe metody, starajmy się zawsze, aby metoda była albo krótka, albo chociaż bardzo wyspecjalizowana


Poza tym całkiem nieźle, widać, że chyba spodobało się Symfony, kto umie taką apkę zrobić nie powinien się chyba za bardzo wstydzić.

Odniosę się do pkt.6 z którym się nie zgodzę. Nie widzę sensu nie przerywanie programu tak szybko jak to tylko możliwe. Redukuje to ilość ifów które potencjalnie mogą wystąpić. Dużo łatwiej się czyta że np. jak jest teraz zamiast przenosić tego returna na koniec i w całym kodzie szukać gdzie jaki warunek jest spełniony i co to zwraca.

Swoją drogą tam wzorzec na url jest niepoprawny. Nie uwzględnia domen np z polskimi znakami ani żadnej współczesnej

Z if'ami każdy ma swoją teorię. Ja akurat też jestem zwolennikiem przerywania jak najwcześniej. W "Czystym kodzie" był opisywany ten aspekt i tam Marti pisał, że jedno wyjście z funkcji to zaszłość z programowania proceduralnego z długimi i skomplikowanymi funkcjami. Tam faktycznie jeden return miał sens, ale w dobie tanich funkcji, gdzie piszemy (powinniśmy pisać) funkcje, które mają po kilka - kilkanaście liniejek, wg niego kilka wyjść z funkcji poprawia czytelność. Ja osobiście w swoim kodzie staram się jak mogę unikać "else" bo wg mnie psuje czytelność, więc tym bardziej stosuje wczesne returny.

Z drugiej strony rozumiem podejście, że funkcja powinna mieć jedno wyjście - po prostu nie zgadzam się z nim w przypadku krótkich funkcji.

Takie porównanie dla przykładu - funkcja przekształcająca daną wartość na tablicę (tak wiem, że bez sensu)

[PHP] pobierz, plaintext 
public function transformToArray($val): array
    {
        if (is_array($val)) {
            return $val;
        }
 
        if (is_object($var)) {
            return $this->transformObjectToArray($val);
        }
 
        if (is_scalar($var)) {
            return $this->transformScalarToArray($val);
        }
    }
 
    public function transformToArray2($val): array
    {
        $result = null;
        if (is_array($val)) {
            $result = $val;
        } else if (is_object($var)) {
            $result = $this->transformObjectToArray($val);
        } else {
            $result = $this->transformScalarToArray($val);
        }
 
        return $result
    }
[PHP] pobierz, plaintext 

Moim zdaniem pierwsza funkcja jest znacznie bardziej czytelna, druga mnie trochę przyprawia o ból głowy. Czytanie pierwszej przerywam w momencie, gdy docieram do interesującego mnie przypadku, a w drugiej muszę scrollować od końca czy jeszcze czegoś nie robię z resultem przed zwróceniem itd. No ale jak pisałem koniec końcu pewnie kwestia preferencji - drugie podejście też ma jakieś poparcie w literaturze.

- bo użyłeś "else if" czego ja np. nie zalecam. Wystarczą te same ify co w funkcji pierwszej, lecz rozbudowane o sprawdzenie czy zmienna "result" jest ustawiona i tyle. Największą dla mnie zaletą tego podejścia jest łatwość debugowania, wystarczy dać:

[PHP] pobierz, plaintext 
dump($result);
[PHP] pobierz, plaintext 

Identycznie z kodem typu:

[PHP] pobierz, plaintext 
return ($a + $b);
[PHP] pobierz, plaintext 

Gdzie po return masz blok kodu, niby nic takiego ale jak chcesz prześledzić w debugerze los jakiegoś obiektu to musisz się trochę więcej napocić.

- jest taka zasada w programowaniu "don't sacrifice clarity for brevity", wolę nawet więcej ifów, klamr, brak operatorów trójargumentowych itp. byle kod był jak najłatwiejszy do zrozumienia.
Taki przykład:

[PHP] pobierz, plaintext 
public function doSth($x = null){
    if($x){
    $y = 0;
}else{
    $y = $x;
}
}
[PHP] pobierz, plaintext 

vs

[PHP] pobierz, plaintext 
public function doSth($x = null){
    $y = 0;
    if($x){
    $y = $x;
}
}
[PHP] pobierz, plaintext 

W przykładzie pierwszym programista czyta: "blok warunkowy, y jest 0 lub x w zależności od x"
W drugim niby to samo, ale człowiek czyta; "deklarujemy y z domyślną wartością 0, potem nadpisujemy iksem gdy ten nie jest nullem"
Akurat w tych przykładach nie ma to znaczenia, ale za miesiąc te metody mają po 50 linijek i pomimo, że druga metoda była na początku bardziej zwięzła, to nagle okazało się, że y zawsze jest nadpisywany, jest jakiś blok martwego kodu albo odwrotnie, jakiś kod się zawsze wykonuje. Po prostu metoda pierwsza bardziej odwzorowuje jakiś blok decyzyjny opisany w dokumentacji procesu biznesowego, zaś druga choć bardziej zwięzła jest jednocześnie bardziej wrażliwa na zmiany w kodzie (zwłaszcza robione przez wielu programistów).
Typowy tego przykład, to użycie "else if" zamiast zagnieżdżonego ifa - ok, mamy mniej wcięć i linijek, ale czy kod jest dzięki temu bardziej czytelny?
Po prostu uważam, że zwięzłość kodu nigdy nie powinna być ważniejsza niż czytelność i jeśli z jakiś powodów redukcja paru linijek może zaburzyć czytelność to powinniśmy z niej zrezygnować.

No ale to wszystko są szczegóły, które nie mają większego znaczenia jeśli nie tworzy się nie wiadomo jak rozbudowanych metod - niestety w praktyce bywa różnie Są o wiele większe problemy w programowaniu
Jakby ktoś chciał zgłębiać ten temat dalej to jedna z ciekawszych debat w sieci o tym:
https://stackoverflow.com/questions/36707/s...eturn-statement

Pilsener odnosisz sie do wypowiedzi Pytona wyrwanej z kontekstu i jako kontrargument podajesz przyklad totalnie z 4 liter. No tak troche nieladnie. Pyton mowil o redukcji IFow spowodowanej szybszym wyjsciem z funkcji. Ty zas kontrargumentujesz podajac zupelnie inny przyklad majacy sie nijak do tego co mowil Pyton

Ja tez wychodze z funkcji jesli to mozliwe bo redukuje to zbedne zagniezdzenia pozniej. To wlasnie sprawia kod czytelniejszym o co i zdaje sie tobie rowniez zalezy.

Czyli wg twoich wskazowek kod bedzie wygladal tak

[PHP] pobierz, plaintext 
    public function transformToArray2($val): array
    {
        $result = null;
        if (is_array($val)) {
            $result = $val;
        }
        if ($result === null && is_object($var)) {
            $result = $this->transformObjectToArray($val);
        } 
       if ($result === null) {
            $result = $this->transformScalarToArray($val);
        }
 
        return $result
    }
 
[PHP] pobierz, plaintext 

Czy to jest bardziej czytelne od kodu z kilkoma return? No ja tez uwazam ze nie. Juz nie wspominajac o tym ze kod musi wykonac pare dodatkowych sprawdzan za kazdym razem. Krotko rzecz mowiac same minusy Argument za debugowaniem w tym momencie uwazam za nietrafiony. Kodu nie powinno sie pisac by moc latwo dac dumpa tylko by byl czytelny i efektywny.


Czyli zamiast prostego:

[PHP] pobierz, plaintext 
return ($a+$b);
[PHP] pobierz, plaintext 

mamy tworzyc dodatkowa zmienna

[PHP] pobierz, plaintext 
$c = $a+$b;
return $c;
[PHP] pobierz, plaintext 

tylko dlatego ze moze kiedys ktos tam bedzie chcial zrobic dumpa z dzialania $a+$b. No prosze cie.

Kurcze Pilsener dla mnie 2 wersja jest bardziej czytelna. Może dlatego, że w pierwszej zgubiłeś ! w pierwszym warunku i się zastanawiałem co ta funkcja robi. Ale nawet bez tego jakoś przyzwyczaiłem się do drugiego zapisu i tak bym to zrobił... Widać wszystko kwestią gustu i coś dla jednych jest bardziej czytelne niż dla innych.

BTW poruszyłeś ciekawy temat, który uciekł w toku dyskusji, czyli CQS - command-guery separation. Ostatnio staram się stosować (ostatnio w sumie całe swoje kodowe nawyki zmieniam) i zauważyłem, że czasami powoduje puchnięcie kodu (zwłaszcza jak funkcja na wyższym poziomie grupuje działania wykonywane przez funkcje na niższym poziomie, gdzie część jest typu query, a inne typu command). Stosujecie tą regułę bez odstępstw, czy Wam się jednak zdarza. Ja staram się bez odstępstw, bo uznałem, że czytelny kod czasami może być dłuższy niż ten nieczytelny.

- hehe, chciałem tylko rozwinąć temat i skopiowałem potrzebne zdanie, potem pomyślałem, że nieładnie tak kopiować czyjąś pracę, więc dałem w cytat i tak jakoś wyszło

I oczywiście masz rację Nospor z tymi przykładami, ja jestem przeciwnikiem jakiś sztywnych reguł typu "zawsze jeden return".
A z tym kodem po return to tak tylko na początku zazwyczaj wygląda, za jakiś czas masz już cały blok kodu tam a to już wkurza. Jak masz tego typu "szkielet" to potem nikomu się nie będzie chciało tego refaktorować, programista to zwierzę dość leniwe, zazwyczaj zmienia kod po najmniejszej linii oporu i w taki sposób można natworzyć miejsc, gdzie kod się będzie rozwijał w złą stronę i w sposób niezbyt kontrolowany.

Poza tym pewne rzeczy robi się niejako automatycznie gdy piszesz kod jak Boziu kazała, czyli zaczynasz od testów. Robisz test, potem metodę i od razu deklarujesz zmienną, zwracasz ją i zapuszczasz test - potem rozbudowujesz tak powstały szkielet metody co wymusza niejako nieco inne podejście. Tylko znów ilu programistów tak robi? W dobie fw i bibliotek testowanie jednostkowe co chwila się zmieniającej logiki biznesowej nie ma większego sensu.



Bo tu masz kolejny przykład:

[PHP] pobierz, plaintext 
if(!$sth)
[PHP] pobierz, plaintext 

vs

[PHP] pobierz, plaintext 
if($sth == false)
[PHP] pobierz, plaintext 

Te wykrzykniki tak na pierwszy rzut oka zbyt widoczne nie są Zwłaszcza jak stosujesz niezależnie tak zwane "rzutowanie na booleana" (dużo osób ma ten nawyk)

[PHP] pobierz, plaintext 
if(!!$sth)
[PHP] pobierz, plaintext 

- tak, to też jest dobre, wpisuje się w generalną zasadę by metody były jak najbardziej wyspecjalizowane. Tylko jakbyś zebrał do kupy te wszystkie pryncypia, to nie byłoby w głowie miejsca na programowanie

Generalnie warto poczytać o jakiś zasadach "czystego kodu" i starać się ich trzymać, nie dać się zwariować

Zgadzam się, że nie można na zasady patrzeć ślepo (zwłaszcza, że w praktyce często zasady okazują się wewnętrznie sprzeczne), ale akurat CQS jest taką zasadą, że albo się ją stosuje albo nie - tak mi się przynajmniej wydaje. Jeśli stosuje się ją tam, gdzie wygodnie to traci sens bo wracamy do punktu wyjścia i nadal nie wiemy czy metoda jest zapytaniem czy komendą bez zajrzenia w jej kod. Chyba nawet stosowanie jej w 95% jest gorsze niż nie stosowanie, bo wtedy ma się wrażenie że coś jest np. komendą, a nagle okazuje się mutować obiekt. Gdy jej nie stosujemy to przynajmniej wiemy, że w tym zakresie mamy w kodzie burdel i wszystko trzeba dokładnie sprawdzić... Takie jest moje zdanie, choć praktyki z CQS mam mało - nowy nabytek w arsenale ;-).

PHPStorm poradzi sobie z wykryciem typów pól określonych w konstruktorze. Dodawanie docblocków w tym przypadku jest nadmiarowe.

Przy okazji, wrzucanie do repo informacji o ide tylko je zaśmieca.

Ciekawe, że każdy ma swoje standardy w kwestii kodu.

Ja bym osobiście jednak komentarzy przy polach klasy nie uznał za nadmiarowe, bo to że ide rozpozna to jedna sprawa, a czytelność kodu to druga. Wchodząc do takiej klasy po miesiącu skąd mam wiedzieć jaki jest typ pola - musiałbym sprawdzić w konstruktorze / w innym miejscu, gdzie jest ustawiana wartość pola. Tak więc moim zdaniem jednak warto docka dać. Nadmiarowy to wg mnie taki, który powiela informacje z sygnatury funkcji bezpośrednio nad nią np:




Inna sprawa, że idąc tym tokiem rozumowania w zasadzie można by nie definiować w ogóle pól w klasie bo ide i bez tego sobie poradzi, a w samych PHP nie ma wymogu aby pola były zdefiniowane - można je tworzyć dynamicznie.

Co do wrzucania do repo informacji o ide to nie rozumiem o co chodzi - chyba że to nie było do mnie.

Na szczescie w nowym wydaniu php bedzie mozna typowac rowniez i wlasciwosci i nie bedzie juz takich rozterek

Chyba czytasz w moich myślach, bo właśnie miałem napisać "ciekawe czy kiedyś wprowadzą opcjonalne typowanie pól" ;-)
No to jeszcze poproszę return type mixed przy funkcji i będzie super.

Oczadzial? Nie po to wprowadzono typowanie by teraz wszystko do kosza wyrzucic.

Czasami sa sytuacje, ze nie da sie w php przewidziec zwracanego typu. Np. dzisiaj budowalem konfiguracje do kontenera DI i odczyt wartosci parametru mogl zwracac wszystko od inta po inny obiekt. Nie da sie takiego czegos opisac bez doca, a brak zwracanego typu moze oznaczac, ze albo ktos nie oznaczyl typu, albo funkcja nic nie zwraca. Ja w takich sutuacjach zawsze daje doca return mixed aby uniknac dwuznacznosci i wolalbym to jawnie zdeklarowac dokladnie tak samo jak :void, gdy nic nie zwracamy.
Idealnie by bylo gdyby doc byl uzywany tylko, gdy trzeba dac cos wiecej niz to co mozna zawrzec w sygnaturze.

Jesli nie wiesz, co bedzie zwracac twoja funkcja, znaczy ze masz zle zaprojektowany system

W nowym php tez maja wprowadzic typ OBJECT ktory bedzie ogolnie obiektem nie wazne jakim byle obiektem. Moze to ci pomoze troche ogarnac to co tam masz u siebie

Czy ja wiem czy zle zaprojektowany. Php ma w sobie dynamike i czasem po prostu nie da sie twardo typowac (albo nie ma to sensu). Tak jak pisalem przyklad z dzisiaj - na wejsciu do kontenera DI mialem konfiguracje w postaci tablicy. Tablica zawierala parametry, ktore wykorzyatywalo reflection api do inicjalizacji klas. Co by nie zrobic to getter do takiej tablicy bedzie zwracal praktycznie dowolny typ danych. Object tu tematu nie rozwiaze, bo w takiej tablicy moga byc skalary, tablice, obiekty/kolejcje obiektow - praktycznie wszystko.
Od czasu jak narzucilem sobie typowanie udalo mi sie 90% sytuacji tak zorganizowac aby funkcja zwracala 1 i tylko 1 typ danych, co z perspektywy uwazam za super rozwiazanie, bo api metod i czytelnosc kodu na tym zyskaly. Ale w swiecie php zawsze znajdzie sie taki przypadek, ktorego nie da sie przerobic albo nie ma to sensu. Getter do rozbudowanej konfiguracji jest IMO takim przypadkiem.
Skoro jest void to i mixed moim zdaniem tez powinno byc aby zachowac jednolity interfejs. W php mixed to tez typ danych w pewnyn sensie

Rozwiaze o tyle, ze mozesz zwracac obiekt, ktory bedzie mial te rozna wartosc w sobie. No ale potem get() w tym obiekcie znowu i tak bedzie zwracal mixed hehe
No ok, jest te pare sytuacji gdzie moze i faktycznie nie da sie okreslic typu.

Nie ma takiej sytuacji że nie da się tego opisać jednolitym typem danych. IMO kwestia znalezienia rozwiązania.

a co myslicie o czymś takim? okreslony typ ewentulnie null, czy lepszym rozwiazaniem są exceptiony?

[PHP] pobierz, plaintext 
public function nazwa(?JakiśTypObiektu $obiekt): ?string
{}
[PHP] pobierz, plaintext 

Null jest dość specyficzny. Ale taki prototyp funkcji nic w sumie nie mówi. Wszystko zależy od tego jak zaprojektujesz. Często tak, Exception załatwia sprawę, a czasami nie. Więc kontekst jest ważny

Taki prototyp często jest używany w Entity gdzie początkowy stan obiektu zwraca null na wszystkim niemal. Więc o ile coś + null jest w miaaarę powiedzmy ok, o tyle int+string+object+.... już nie...

Ale uważasz, że zawsze jest sens walczyć o silne typowanie? PHP to jest język dynamiczny i KIEDY MA TO SENS warto korzystać z możliwości, które daje. O ile jestem zazwyczaj przeciwnikiem funkcji zwracających wszystkie możliwe typy danych, to czasami na to po prostu sens - np. wspomniana konfiguracja, która może przechowywać zarówno typy proste jak i obiekty. To bardzo generyczny obiekt i MZ nie ma sensu dla idei walczyć o typowanie. Oczywiście jeśli się mylę, to chętnie poznam argumenty i jakieś sensowne podejście do tego tematu w PHP.

Zaciekawił mnie kolejny wątek - czyli null (+ wartości domyślne, z którymi się poniekąd wiąże). Co do zasady jestem zwolennikiem, że funkcja nie powinna zwracać null, gdy jest to możliwe. Sztandarowy przykład - wolę zwrócić pusta tablicę niż null. Oczywiście znów nie zawsze jest sens o to walczyć i w praktyce mimo wszystko często stosuję null (zamiast np. stosować null object), ale zawsze 2 razy się zastanawiam, czy nie da się tego rozwiązać lepiej.
Exceptiony MZ zazwyczaj nie są dobrym wyjściem - Exception jak sama nazwa wskazuje powinien obrazować zjawisko wyjątkowe, a np. brak odczytanych rezultatów z bazy zazwyczaj nie jest zjawiskiem wyjątkowym - np. brak produktów w sklepie w danej kategorii. Oczywiście może takim być - np. loguje się użytkownik i nagle się okazuje, że nie ma tabeli z jego ACL'ami, a logika aplikacji zakłada, że każdy użytkownik ma tam jakieś wpisy - wtedy oczywiście Exception jest ok.

Ostatnio też miałem dość ciekawą rozmowę na temat domyślnych wartości argumentów w funkcjach. Do tej pory stosowałem je na potęgę z 2 głupich powodów:
- mniej pisania w kodzie klienckim
- w czasie refactoringu, gdy dochodził parametr stosowałem wartość domyślną aby nie trzeba modyfikować kodu klienckiego

Rozmowa otworzyła mi oczy, że głupio robiłem. Po pierwsze kod kliencki musi wiedzieć co chce zrobić i im mniej tam przerzucania odpowiedzialności na "inteligencję" swoich klas/funkcji tym lepiej, bo pomaga to zapobiegać efektom ubocznym. Przeróbka koda klienckiego przy refactoringu dzisiaj też jest prosta, więc nie jest to argument za stosowaniem wartości domyślnych. Przede wszystkim jednak ciekawym doświadczeniem było zaprzestanie używania wartości domyślnych w swoim kodzie. Po 2-3 dniach okazało się, że w 95% przypadków można pozbyć się tego brzydactwa.

Dlatego ogólnie na dzisiaj moja opinia jest taka - null / wartości domyślne powinno się stosować ale z umiarem. Zawsze się warto zastanowić czy robimy to z wygody, czy z realnej potrzeby.

Ogólnie muszę przyznać, że dużo w moim kodzie poprawiło stosowanie się do zasady YAGNI i polubienie się z refactoringiem. Wiele problemów typu domyślne wartości parametrów często się same rozwiązują, bo np. zakładamy, że dzisiaj co prawda parametr będzie zawsze taki sam (i dlatego dajemy u default value) ALE KIEDYŚ to się zmieni. W praktyce okazuje się, że w 9 na 10 sytuacji to się nigdy już nie zmieni, albo funkcję przepiszemy jescze 3 razy zanim się zmieni i zostajemy z parametrem, który w ogóle nie jest do niczego potrzebny.