Jak NIE należy pisać kodu

27 października 2009

Właśnie pracuję nad jednym małym zleceniem. Problem w tym, że jestem podwykonawcą, odpowiedzialnym tylko za jeden element, całą resztę zrobił już inny programista. Muszę teraz edytować jego kod i dostosowywać do nowych potrzeb.

No to zaczynamy. Otwieram pliki. No super... Zamykam pliki i przełączam edytor na ISO :) Mogliby się ludzie już przestawić na UTF.

Otwieram pliki :) Plik konfiguracyjny z kilkudziesięcioma definicjami stałych, dla każdej tabeli w bazie jedna definicja. Nie podoba mi się to ale niech już tak będzie.

Szukam plików odpowiedzialnych za element którym mam się zająć. Na ftp trochę bajzel, można było to chyba trochę lepiej pogrupować ale nie jest tragicznie.

Jest coś dotyczącego zlecenia, otwieram. Ech, wszystko wpakowane w funkcje. Przykład:

zobacz jako zwykły tekst (nowe okno)
PHP:
  1. function BodyLogowanie_START()
  2. {
  3.   print'
  4.  <TABLE width="100%" height="100%" border="0" cellspacing="0" cellpadding="0" style="margin-bottom:15px;">
  5.    <TR>
  6.    <TD align="center" valign="middle">';
  7. }
  8. function BodyLogowanie_END()
  9. {
  11.    </TD>
  12.  </TR>
  13.  </TABLE>';
  14. }

Funkcje fajna rzecz ale ich używanie obwarowane jest pewnymi zasadami. Każdy lepszy kurs i każda książka o tym mówi. Założę się, że są tutaj funkcje, które używane są tylko w jednym miejscu przez co funkcjami nie powinny być. Olać, idę dalej.

Przeglądam pliki, cały HTML oparty na tabelkach. Oj dostałoby się koledze za to na publicznych forach :) Olać, HTML w tym zleceniu to już zupełnie nie moja bajka.

O, jest jakiś kawałek o logowaniu. Od razu w oczy rzuca się ten kod (fragment zapytania do MySQL):

zobacz jako zwykły tekst (nowe okno)
PHP:
  1. login='".$_POST['email']."'

To już nie jest takie śmieszne, ktoś się włamie a potem powiedzą, że moja wina. Trzeba będzie poprawić...

Identyfikator połączenia z bazą danych trzymany w sesji? Przyznam, że się nie spotkałem :)

Przyzwyczaiłem się już do angielskiego nazewnictwa plików, zmiennych itd. dlatego rażą mnie w oczy polskie słowa. Ale tu już się czepiać nie będę, wielu programistów tak robi, sam kiedyś tak pisałem. Musiałem się potem przestawić jak zacząłem pisać kod w wielonarodowych zespołach.

Pisałem już, że kod PHP jest całkowicie wymieszany z HTML'em? :) Dodajmy do tego, że wszystko jest pozamykane w dziesiątkach funkcji i mamy niezły mętlik. Nie lubię programować gdy nad kodem muszę się zastanawiać jak nad wierszem: "Co autor miał na myśli?".

Jak już jesteśmy przy domyślaniu się to wspomnę, że jest zero komentarzy w kodzie.

Dobra, czas zacząć pracować. Znalazłem to co mnie interesuje. I od razu szok:

zobacz jako zwykły tekst (nowe okno)
PHP:
  1. $zapytanie="SELECT login,email FROM `"._CONF_table_users."`;";
  2. $wykonaj=$Connect->MysqlQueryRet($zapytanie);
  3. if($Connect->GetValue('error_mysql')==False){
  4.   $i=0;
  5.   while($wiersz=@mysql_fetch_array($wykonaj)){
  6.     if($wiersz['email']==$_POST['email']||$wiersz['login']==$_POST['login'])$i++;
  7.   }
  8.   if($i>0)$ret=$RETURN_KONTO[1][0];else $ret=$RETURN_KONTO[0][0];
  9. }

Jak z PHP mam do czynienia już 6 czy 7 rok tak jeszcze takiego cuda nie widziałem. Czy ten programista nigdy nie słyszał o warunkach WHERE w zapytaniach SQL? Wyciągać całą zawartość tabeli tylko po to żeby sprawdzić czy czasem nam się konto nie zdubluje? :/ Paranoja... Nie chce myśleć co się będzie działo z serwerem gdy na stronie zarejestruje się więcej niż kilka tysięcy ludzi.

Po powyższym wiedziałem już, że łatwo z tym zleceniem nie będzie ;) Na tym zakończę tą "recenzję" bo chociaż są jeszcze inne błędy, niektóre naprawdę poważne, to recenzowanie nie jest celem tego wpisu. Teraz kilka pytań bez odpowiedzi.

  • Czy powinienem zwrócić programiście uwagę na jego błędy?
  • Czy powinienem powiadomić właściciela projektu o kwiatkach jakie zgotował im ich programista?
  • Czy powinienem poprawić chociaż te błędy, które jako tako dotyczą mojego zlecenia czy pojechać szablonem, który wyznaczył główny programista i mieć zlecenie z głowy?
  • Czy powinienem o tym pisać na blogu? :D

Odpowiedzi na pytania wcale nie są proste. Albo zacznę interweniować i wszyscy mnie znienawidzą a sam będę miał tylko więcej nerwów i pracy albo nie odezwę się ani słowem i będę miał spokój ale strona będzie przez następne trzy lata łatana, pewnie przez tego samego programistę.

Z powyższego wpisu można wyciągnąć kilka wniosków.

  • Po pierwsze, trzeba naprawdę dużo praktyki żeby zacząć dobrze programować.
  • Po drugie, nie należy oszczędzać na programistach :P Nie wiem ile temu chłopakowi płacą ale kod jest słaby a projekt ciągnie się już pół roku i pewnie jeszcze trochę potrwa nim strona pojawi się w sieci. Lepszy programista zainkasowałby odpowiednio dużo kasy ale zrobiłby to porządnie a oni już by zarabiali.
  • Po trzecie, na rynku chyba nie ma wielu dobrych programistów... Dostaję na to coraz więcej dowodów.

Zachęcam do dyskusji w komentarzach. Co o tym wszystkim myślicie? Co doradzacie?

  • Dodano: wtorek, 27 paź 2009 roku o godzinie 16:24
  • Kategorie: Tworzenie stron www
  • Tagi: , ,
  • Możesz śledzić odpowiedzi poprzez RSS
  • Możesz przeskoczyć do końca i zostawić komentarz. Pingowanie aktualnie jest wyłączone .
 Dodaj komentarz

15 odpowiedzi dla tego wpisu

  1. guci0 napisał: 27.10.09 o godzinie 18:43

    “Czy powinienem o tym pisać na blogu? ” – koniecznie pisać :)

  2. Michał napisał: 29.10.09 o godzinie 20:46

    Ojj nawet nie wiesz jak dobrze cię rozumiem…

    * Czy powinienem zwrócić programiście uwagę na jego błędy? Nie, ja zwracałem i obróciło się to przeciwko mnie
    * Czy powinienem powiadomić właściciela projektu o kwiatkach jakie zgotował im ich programista? Jak wyżej
    * Czy powinienem poprawić chociaż te błędy, które jako tako dotyczą mojego zlecenia czy pojechać szablonem, który wyznaczył główny programista i mieć zlecenie z głowy? Sam zrób to, co nalezy do ciebie dobrze, ew jakieś małe błędy możesz poprawić.
    * Czy powinienem o tym pisać na blogu? :D Wydaje mi się, ze tak

    Pozdrawiam…

  3. DawPi napisał: 11.11.09 o godzinie 2:32

    Po pierwsze to nie nazywaj go programistą.

  4. LO napisał: 09.12.09 o godzinie 17:06

    Witam,

    Jak dla mnie jest tylko jedno usprawiedliwienie dla typa – nie wstawiając parametrów do treści zapytania unika SQL injection :P

    Pominę milczeniem, że są na to duuużo lepsze sposoby…

    Pozdrawiam
    LO

  5. MariuszT napisał: 09.12.09 o godzinie 17:11

    Pewnie masz na myśli trzecie okienko z kodem ale w drugim okienku pokazałem, że zmienne są bezpośrednio wstawiane do zapytania, otwarta furtka dla SQL Injection.

  6. Paweł Danielewski napisał: 08.01.10 o godzinie 3:11

    Niestety dobrych programistów PHP w Polsce jest jak na lekarstwo. Jest za to cała masa takich którzy psują rynek. Biorąc (a przynajmniej chcą brać) 50 – 60 zł za godzinę pracy podczas gdy ich poziom jest bliski ZERU. Pamiętam jak nie tak dawno współpracowałem z jednym ze “zlecenia.przez.net”. Chłopak co chwila pytał jak zaimplementować phpmailera, chwilę później jak wgrać bazę big dumpem… albo dlaczego widzi “krzaczki” w wynikach. W przesyłanych ofertach 90% firm przedstawia się jako “profesjonalni” i “doświadczeni” podczas gdy efekt ma się nijak do do tych słów.

  7. mati napisał: 22.01.10 o godzinie 19:32

    Po kolei:
    > Czy powinienem zwrócić programiście uwagę na jego błędy?
    Tak. I oczekiwać ich naprawienia. Jeśli nie reaguje – eskalacja problemu wyżej.
    > Czy powinienem powiadomić właściciela projektu o kwiatkach jakie zgotował im ich programista?
    Eskalować problem powinieneś gdy rozmowa z programistą nic nie dała. Nigdy nie od razu.I to też na zasadzie: “Czy są Państwo zainteresowani kilkoma moimi uwagami związanymi z kodem, który zastałem?”
    > Czy powinienem poprawić chociaż te błędy, które jako tako dotyczą mojego zlecenia czy pojechać szablonem, który wyznaczył główny programista i mieć zlecenie z głowy?
    Tak. Jeśli poprawienie błędów nie jest czasochłonne. Jeśli nie – podpisać aneks do zawartej umowy (zwiększający oczywiście wynagrodzenie i termin wykonania).
    > Czy powinienem o tym pisać na blogu?
    Nie.

    Oczywiście odpowiedź 2 i 3 jest inna (podobnie jak część pierwszej), jeśli tworzycie konsorcjum lub jesteś podwykonawcą. Ale z tego co napisałeś (pomimo “jestem podwykonawcą”) nie jesteś – jesteś po prostu kolejną sobą współtworzącą ów tajemniczy projekt (to wynika z tekstu).

    Jeśli jesteś podwykonawcą kontakt właściciel projektu-Ty nie istnieje.
    Dlaczego? Bo tak (możemy o tym porozmawiać na gg).

    Jeśli tworzycie konsorcjum, to któraś z osób jest liderem konsorcjum (choć w przypadku pojedynczych ludzi pasuje bardziej słowo ‘zespół’). Niemniej problemy wewnątrzkonsorcjalne nie interesują klienta. Donoszenie na konsorcjanta jest strzałem w stopę.

  8. MariuszT napisał: 22.01.10 o godzinie 19:59

    Ależ Ty uwielbiasz być skrupulatny. Masz rację, nie jestem podwykonawcą, przynajmniej nie takim zgodnym z definicją. Po prostu przyszedłem w połowie projektu i miałem coś tam w nim dodać.

    Nie zgodzę się z Tobą, że nie powinienem o tym pisać na blogu. Zachowałem pełną anonimowość i mam 95% pewności, że nikt z zainteresowanych nigdy tego nie przeczyta i nie skojarzy ze sobą. A problem ważny, niestety często spotykany i warto go omówić.

    Napiszę Ci jak to się skończyło. Po kilku kontaktach z programistą łatwo było wywnioskować, że ma spore braki, niekiedy podstawowe. Nie było wątpliwości, że nie jest w stanie poprawić swoich błędów. Nie dostrzega ich a po wskazaniu nie ma pojęcia czemu to jest problem i/lub nie wie jak go poprawić. W najlepszym przypadku mógłby coś poprawić i przy okazji coś innego sknocić.

    Udawanie się z tym do zleceniodawcy w tym konkretnym przypadku nie ma sensu. Wydatek dla niego 2tys to poważna sprawa. Nie wiem jak miałbym mu powiedzieć, że to co do tej pory zostało zrobione nadaje się do… napisania od nowa. Nie zgodziłby się ze mną (psychologiczny efekt obrony) a nawet jeżeli by mi zawierzył to i tak nie byłby w stanie zareagować bo go na to nie stać. Nie stać z wielu powodów. Z powodów finansowych, z powodu strat wygenerowanych drastycznym opóźnieniem i z powodu (tu zgaduję ale pewnie tak właśnie jest) podpisania lakonicznej umowy z programistą gdzie nie mógłby go teraz pociągnąć do odpowiedzialności. W efekcie zostawiłbym po sobie tylko smrodek chociaż niczym nie zawiniłem, strona nadal by działała z tym, że teraz właściciel już by wiedział, że jest kiepska przez co miałby tylko więcej nerwów.

  9. mati napisał: 23.01.10 o godzinie 20:29

    5% to dużo.
    Tym bardziej, że z dalszej części komentarza można wywnioskować, że błędy nie zostały poprawione. A nawet jeśli te konkretne, to spodziewać sie można, że jest wiele podstawowych. Tak się po prostu nie robie – przyjmujesz zlecenie, które nie wiąże się z generowaniem contentu do blogu. Co innego napisać ogolnie, gdy błędy zostaną poprawione.

    Nawet błędy w oprogramowaniu zgłaszają do producenta i ogłaszane jest po przygotowaniu odpowiednich łat. A przy tym producent nic nie płaci za zgłoszenie takiego błędu – te, które wyłapią ludzie zatrudnieni najczęściej nigdy nie są nagłaśniane.

    Więc podsumowując resztę wypowiedzi: w trosce o dobre samopoczucie zleceniodawcy nie poinformowałeś go o często podstawowych błędach zagrażających bezpieczeństwu projektu (jeśli 2000 to pewnie malutkiego, ale jednak projektu)?

    Swoją drogą kiedyś myślałem, że zrobienie średnio rozbudowanego projektu za 8 000 to właściwie kilka tygodni mojej pracy.
    Teraz wiem, ze ktoś kto podejmuje się tego za 8 000 jest dziwny. Bo nawet 80 000 to wtedy za mało. Mówie o wszystkich rzeczach, nie tylko kodowaniu ale i rzeczach pobocznych.
    No i warunkach wielkopolskich – nie wiem jak w innych czesciach kraju.

    Swoją drogą planuje niedługo przeprowadzić właśnie taki test – coś co wydaje się proste do zrobienia (miesiąc?) ale zrobić za niższą – niż podałem – kwotę.
    Uda się? Zobaczymy.

  10. MariuszT napisał: 23.01.10 o godzinie 21:06

    Nadal się z Tobą nie zgodzę, tym wpisem nie działam na szkodę zleceniodawcy bo nikt nie jest w stanie zidentyfikować serwisu. Nie robię żadnego swojego portfolio, nikt nie wie o tym zleceniu więc nie generuję żadnego zagrożenia. 5% to możliwość trafienia na ten tekst i rozpoznania serwisu przez samego programistę lub zleceniodawcę, nie osoby postronne.

    Samopoczucie zleceniodawcy nie interesuje mnie aż tak bardzo żeby z tego powodu nie powiadamiać go o lukach w jego oprogramowaniu. Bardzo dokładnie opisałem czemu postąpiłem tak a nie inaczej oraz opisałem cały proces jaki by się odbył po mojej ewentualnej interwencji gdzie to ja bym miał nieprzyjemności z powodu błędów innego programisty.

  11. mati napisał: 24.01.10 o godzinie 2:12

    Takich rzeczy nie pisze się na blogu. Bo nawet jeśli ty się nie afiszujesz, to nie znaczy, że w zwykłej rozmowie ów programista nie powie:
    – no ja pisałem xxxxx , a yyyy pisał jakiś Mariusz Tarnaski.
    Albo zleceniodawca:
    - część kodu napisał twórca innych, znanych w Tomaszowie Mazowieckim stron – osada.pl i nasztomaszow.pl.

    Jesteś wiec pewien, że tylko właściciel serwisu i programista? Możesz to zagwarantować?Dasz sobie coś uciąć? :P

    I drugi temat:
    Właśnie opisałeś w komentarzu tylko to, że wg Ciebie drugi programista to kretyn. I, że nie powiedziałeś zleceniodawcy bo “nie zgodził by się” z Tobą a tak (tu uwaga, mój ulubiony fragment :P ) “by wiedział, że jest kiepska przez co miałby tylko więcej nerwów”.

    Postaw sie na miejscu zleceniodawcy – wywalasz kase i chcesz wiedzieć że coś nie gra i trzeba zapłacić jeszcze za naprawę, czy żyć w błogiej nieświadomości będąc przekonanym, że wszystko gra?

    A to czy zostawiłbyś po sobie smrodek zależy wyłącznie od sposobu w jaki przekazałbyś tą informację.

    Dziwie się trochę, bo równolegle toczymy dyskusję dotyczącą obsługi klienta. To tutaj to przywiezienie bardzo szybko zimnej pizzy. Jesteś profesjonalistą – wiec to, że ciężko pracuje Ci się z kimś nie powinno mieć wpływu na twój profesjonalizm.

  12. MariuszT napisał: 24.01.10 o godzinie 2:46

    Mati, musisz pamiętać, że nie rozmawiamy o pracy w korporacji. Tam bez ogródek wymieniłbym wszystkie błędy. Tu natomiast wiem jaki byłby efekt mojej ingerencji – już to przerabiałem kiedyś. Wystarczyło delikatnie zwrócić uwagę żeby wyszło, że się mądruję i pozjadałem wszystkie rozumy. Drugi raz nie chcę takich niemiłych “podziękowań” za profesjonalizm.

    Żebyś zrozumiał moje intencje i obawy to może przytoczę znany przykład tego typu problemów. Są dwie koleżanki, przyjaciółki od lat. Jedna ma męża, który ją zdradza. Druga się o tym dowiaduje. Ma powiedzieć swojej przyjaciółce, że jej mąż to gnida? :)

  13. mati napisał: 24.01.10 o godzinie 20:26

    Nie rozmawiamy o pracy w korporacji. Zatrudniając kogoś, do zrobienia czegoś mam prawo wymagać. Wymagać przede wszystkim poprawnego wykonania pracy a także zweryfikowania poprawności rzeczy z którymi ma się bezpośrednio styczność.

    I podałem przykład jak (oczywiście moim zdaniem) powinno się zwracać się uwagę na takie rzeczy.
    “Czy są Państwo zainteresowani moją opinią o fragmentach kodu, które przy realizowaniu swojej części projektu przeanalizowałem?”
    “Przeglądając kod mój niepokój wzbudziło kilka elementów, które moim zdaniem czynią podatność skryptu na różnego rodzaju atakach. Jeśli będą państwo zainteresowani i znajdą chwilkę czasu moge je przedstawić”

    Drugi problem jest zupełnie bez związku:
    Przykład bardziej pasujący:
    Jest sobie kobieta. Zaginął jej mąż. Zleca jego odszukanie detektywowi. Detektyw znajduje męża i odkrywa, że zniknął bo pojechał na wakacje z jej przyjaciółką. I zdradza ją.
    Pytanie: czy detektyw (mając zapłacone za odnalezienie męża) tylko odnajduje, czy również informuje, że uciekł aby ją zdradzać?
    Jeśli nie, będzie ją zdradzał wciąż – ale będzie spokojnie spała. Nie będzie się denerwowała i żyła w przeświadczeniu, że wszystko jest ok. Aż kiedyś może się dowie, i wtedy ją sieknie totalnie.
    Jeśli tak – może zarzucić Ci mądrowanie się, pozjadanie wszystkich rozumów.
    Możesz przedstawić dowody – odnośniki do stron, for gdzie opisany jest sql injection, a nawet podać co ma wpisać w adresie aby zademonstrować atak … eeee sorry, miałem na myśli pokazać zdjęcia.
    Kobieta pewnie nie będzie początkowo szczęśliwa, może da szanse pierwszemu programiście się wykazać i naprawić ko…. to znaczy dać szanse mężowi na naprawę tego związku.

  14. MariuszT napisał: 24.01.10 o godzinie 20:52

    Obawiam się, że kręcimy się w miejscu. Możliwe, że inaczej patrzymy na tą sprawę bo ja tych ludzi znam, rozmawiałem z nimi etc. Dla mnie EOT.

  15. mati napisał: 24.01.10 o godzinie 21:11

    Kręcimy się w miejscu.
    Ale przynajmniej (tam mi się wydaje) przekonałem Cię do tego, że nie powinieneś opisywać tego na blogu.

    Uważam jak uważam – i zlecając coś liczyłbym że dowiem się o ewentualnych dziurach w kodzie.

Odpowiedz



Podobne wpisy: