dotNETportal.cz

Já jsem si zase všiml těchto:

-v vyhodnocovat boolean výrazem "boolResult == 0" je úsměvné

-request.Abort() v catch bloku se neprovede pokud nedojde k výjimce

-jak už napsal kolega - intCounter a boolResult plní stejnou funkci, tudíž je tam nejméně jeden z nich navíc, dále je algoritmus napsaný tak, že celý cyklus while se nikdy neuplatní

-taky si kladu otázku, k čemu celé to pracné čtení obsahu je, když se s tím výsledkem nic neudělá? No budiž, může to být test, ale pak je zbyteně komplikovaný, například proměnná buffer je zbytečné plýtvání pamětí

Ten request.Abort() je tam asi proto, že autor kódu měl záměr přerušit FTP operaci když dojde k vyjímce, ovšem jaksi mu nedošlo, že je to určeno pro asynchronní operace -> je to k ničemu.

Aha, Abort() není ekvivalent Dispose() nebo Close()?? Pak tam ale úplně chybí uzavření třída request. To je dost problém. To spojení zůstane vždy otevřené.

Koukám, že vy na tom nejste o nic lépe než autor kódu. Abort není ekvivalent ani Dispose, ani Close, ale přerušení běžící asynchronní operace (spuštěné metodou Begin*). Žádné uzavření třídy není potřeba, protože třída neimplementuje rozhraní IDisposable a tudíž se o všechno postará Garbage Collector. Spojení samozřejmě otevřené nezůstane, protože se otevírá až v momentě přístupu ke streamu requestStream a na ten je korektně použito Using...End Using.

:) obávám se, že je to pravdivé hodnocení. Opravdu jsem to using trochu "přehlédl", což je dost hrubá chyba. Jak Stream, tak FileStream jsou IDisposable, takže se na konci bloku správně uzavřou, což popírá mé předchozí námitky. No, na mě ten kód působí, že ten autor to jádro, které je vjádru dobré odněkud zkopnul a obalil to několika svými menšími zmatky.

1) zahazuje se výjimka !!!

2) kód míchá obecnou věc (upload filu na ftp) s konkrétními (přístup k heslu, formát ftp adresy)

Jak už jsem psal tak záleží na tom, jestli metoda má vyhazovat vyjímky nebo ne, ale běžná praxe je odchytit ("zahodit") konkrétní vyjímky, které je možné uvnitř metody nějak rozumně ošetřit.

Aha, lépe jsem se na to podíval - ten program dělá upload. Tak beru zpět svůj poslední bod. To fakt asi funguje :)) To jádro se mi nezdá špatné. A to, že v catch je deklarovaná proměnná výjimky, která se nepoužije by mi moc nevadilo.

(v prvním bodu mého minulého příspěvku mělo být namísto 0 false)

Vzhledem k tomu, že se nějak nikdo nemá k vytknutí konkrétních nedostatků (po 475+ přečteních), napíšu co se nezdá na tomto kusu kódu mě:

1) strDomain a strFileDestName by se mělo ověřovat pomocí String.IsNullOrWhiteSpace

2) Výsledná adresa by měla být ověřována pomocí System.Uri.TryCreate

3) Chybí kontrola existence souboru strFileSourceName

4) Uživatelské jméno by mělo být v konfiguračním souboru

5) Heslo v konfiguračním souboru by mělo být v šifrované sekci

6) Proměnné boolResult a intCounter jsou zbytečné

7) Srozumitelnější je použít readBytes > 0 místo readBytes != 0 a bytesRead místo readBytes

8) Nejsou dodržovány konvence pro pojmenovávání

Body 4 a 5 platí, pokud není požadavek získávat přihlašovací údaje pomocí Dependency injection ale přímo.

Pokud metoda může vyhazovat vyjímky, v Catch by se měly odchytávat konkrétní vyjímky a ty ostatní být popsané v komentářích <exception cref=""></exception>.

Jako naprosté minimum pro tuning této metody bych považoval validaci předaných parametrů.

Rad bych vedel, co konkretne je na tomto kodu zprasene?

Ze neni elegantne napsany? Nebo zpusob, jakym je napsany, ma fatalni vliv na funkcnost aplikace?

Taky jsem zvědavý na výsledky. Pár chyb proti best practices tam je, ale 10 jich najít nemůžu :-)

To, že kód funguje, neznamená, že je napsaný správně.

Když butete vruty zatloukat kladivem, tak také budou držet. Ale jen do chvíle, než na ně někdo sáhne. A to je přesně problém i tohoto kódu, z hlediska budoucí udržovatelnosti nebo z hlediska odolnosti vůči chybám.

Už jsem viděl daleko větší prasárny. Bylo by zajímavé na závěr vložit i opravený kód pro srovnání.

Jinak dobrá databáze nehorázností je Hall of Shame na CodeProjectu:

http://www.codeproject.com/feature/HallO...

Neříkám, že tohle je největší prasárna, v tom samém projektu bych našel i daleko horší. Opravený kód uvedu, jen co se nahromadí komentáře s náměty na chyby.

Dobrá databáze nemravností je také http://www.thedailywtf.com.

Děláš jako kdybys nikdy nic podobného nenapsal ;) Navíc půlka shnilotin může být naprosto OK, některý jazyky nemají moc dobrou generalizaci nad IO a sítí, případně něco může být i úmysl (takhle bez kontextu se to blbě posuzuje).

A asi největší sviňárna - nepřímo tvrdíš o autorovi, jaké je prase, ale co když to mohl být v pohodě programátor, jen na to měl jednu noc a prostě musel to napsat, protože to posral management? Viděl jsem už zprasený kód od dobrýho člověka, jen proto, že to muselo být okamžitě, včera bylo pozdě a "a na 100% to byla jednorázovka" (čemuž je samozřejmě neradno věřit, ale kdžy není čas ...).

Divil by ses, ale naposledy, kdy jsem napsal podobně prasácký kód, bylo asi přes 14 lety v QBASICu, než jsem objevil, že se tam dají dělat i funkce.

Napsat tohleto aspoň trochu kulturně nezabere víc než 2 minuty času navíc, a je typické házet svou neschopnost házet na management.

Navíc celý ten projekt je psán v tomto duchu, programátora znám osobně a je to jeho neschopnost. Nic v té ukázce není zprasené schválně, prostě mu nedocházely důsledky jeho činů.

Psát aspoň trochu pěkně nezabere nikdy o tolik víc, aby se to dalo omluvit nedostatkem času, a těch pár minut strávených dnes ušetří už zítra nebo pozítří daleko víc času při hledání chyb nebo při budoucím rozšiřování.

Já mluvil spíš obecně, v tomhle případě by napsat čistě možná vyšlo ještě rychlejc :) Nicméně lokální prasárny typicky nejsou moc problém, v případě potřeby se to dá celkem rychle zkulturnit (až na výjímky, kdy není ani jasné, co to vůbec dělá a to aní z commentu a názvu :) ), průser je když je zprasená celá architektura, to pak hned neopravíš, že jo. A udělat pěknou architekturu nějaký čas stojí, člověk se musí zamyslet, v optimálním případě sepsat nějakou analýzu a tak (pokud je tam víc lidí, tak sepsání něčeho je nutnost, aby ostatní se měli čím řídit)

Jednou jsem musel asi za 6 hodin napsat takový jednodšší eshop, a myslím, že vám pošlu zdrojové kódy, aby jste věděli, co je opravdová prasárna :-D

Musel? To na vás jako mířili pistolí a drželi v ruce stopky?

Neexistuje jiný důvod, proč slíbit, že něco za 6 hodin udělat jde, a pak to naprasit. Zajistíte si tím jen to, že aplikace bude časem vyhazovat chyby a bude velmi obtížné je najít, a když pak ten kód po vás někdo převezme, bude vás proklínat až do 15. kolene.

Ano, důvod byl v mé blbosti, jelikož došlo ke ztrátě dat projektu, takže jsem jej mumsel dělat znovu a při tom splnit limit