Odpraste tento kód (1. díl)

Tomáš Herceg       5. 7. 2011       C#, Offtopic       6572 zobrazení

Dnes jsem v jednom dost mizerně napsaném projektu, který jsme dostali na údržbu, narazil na tento kousek kódu. Jen během prvních 20 sekund v něm vidím aspoň 10 chyb. Najdete je?

Pokročilejší čtenáře, kteří ví, co je to IoC/DI, prosím, aby chyby zatím nehledali a do komentářů nic nepsali – k tomu se dostaneme později.

Nejdříve bych rád oslovil začátečníky a vyzval je, aby chyby, které v kódu vidí, napsali do komentářů. Upozorňuji, že kód funguje, ale to ještě neznamená, že bychom jej v aplikaci mohli nechat v tomto stavu.

         static bool UploadFile(string strDomain, string strFileDestName, string strFileSourceName)
{
//return false;

string ftpDest = "ftp://ftp." + strDomain + "/htdocs/" + strFileDestName;
string userName = strDomain;
string password = System.Configuration.ConfigurationSettings.AppSettings["ftpPassword"];
bool boolResult = false;
int intCounter = 1;

FtpWebRequest request = WebRequest.Create(ftpDest) as FtpWebRequest;
request.Method =
WebRequestMethods.Ftp.UploadFile;
request.UsePassive =
true;
request.Credentials =
new NetworkCredential(userName, password);
request.Timeout = 10000;

while ((boolResult == false) && (intCounter > 0))
{
try
{
using (Stream requestStream = request.GetRequestStream())
{
const int bufferLength = 2048;
var buffer = new byte[bufferLength];
int count = 0;
int readBytes = 0;
using (FileStream stream = File.OpenRead(strFileSourceName))
{
do
{
readBytes = stream.Read(buffer, 0, bufferLength);
requestStream.Write(buffer, 0, readBytes);
count += readBytes;
}
while (readBytes != 0);
}
request.ContentLength = count;
}
boolResult =
true;
}
catch (Exception ex)
{
request.Abort();
boolResult =
false;
intCounter--;

}
}

return boolResult;
}

 

hodnocení článku

1 bodů / 1 hlasů       Hodnotit mohou jen registrované uživatelé.

 

Nový příspěvek

 

Diskuse: Odpraste tento kód (1. díl)

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í

nahlásit spamnahlásit spam 0 odpovědětodpovědě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.

nahlásit spamnahlásit spam 0 odpovědětodpovědět

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é.

nahlásit spamnahlásit spam 0 odpovědětodpovědět

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.

nahlásit spamnahlásit spam 0 odpovědětodpovědět

:) 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.

nahlásit spamnahlásit spam 0 odpovědětodpovědět

Diskuse: Odpraste tento kód (1. díl)

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)

nahlásit spamnahlásit spam 0 odpovědětodpovědět

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.

nahlásit spamnahlásit spam 0 odpovědětodpovědět

Diskuse: Odpraste tento kód (1. díl)

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)

nahlásit spamnahlásit spam 0 odpovědětodpovědět

Diskuse: Odpraste tento kód (1. díl)

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ů.

nahlásit spamnahlásit spam 2 / 2 odpovědětodpovědět

Diskuse: Odpraste tento kód (1. díl)

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?

nahlásit spamnahlásit spam 0 odpovědětodpovědět

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

nahlásit spamnahlásit spam 0 odpovědětodpovědět

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.

nahlásit spamnahlásit spam 0 odpovědětodpovědět

Diskuse: Odpraste tento kód (1. díl)

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...

nahlásit spamnahlásit spam 0 odpovědětodpovědět

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.

nahlásit spamnahlásit spam 0 odpovědětodpovědět

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 ...).

nahlásit spamnahlásit spam 0 odpovědětodpovědět

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í.

nahlásit spamnahlásit spam 0 odpovědětodpovědět

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)

nahlásit spamnahlásit spam 0 odpovědětodpovědět

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

nahlásit spamnahlásit spam 1 / 1 odpovědětodpovědět

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.

nahlásit spamnahlásit spam 1 / 1 odpovědětodpovědět

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

nahlásit spamnahlásit spam 0 odpovědětodpovědět
                       
Nadpis:
Antispam: Komu se občas házejí perly?
Příspěvek bude publikován pod identitou   anonym.

Nyní zakládáte pod článkem nové diskusní vlákno.
Pokud chcete reagovat na jiný příspěvek, klikněte na tlačítko "Odpovědět" u některého diskusního příspěvku.

Nyní odpovídáte na příspěvek pod článkem. Nebo chcete raději založit nové vlákno?

 

  • Administrátoři si vyhrazují právo komentáře upravovat či mazat bez udání důvodu.
    Mazány budou zejména komentáře obsahující vulgarity nebo porušující pravidla publikování.
  • Pokud nejste zaregistrováni, Vaše IP adresa bude zveřejněna. Pokud s tímto nesouhlasíte, příspěvek neodesílejte.

přihlásit pomocí externího účtu

přihlásit pomocí jména a hesla

Uživatel:  
Heslo:  

zapomenuté heslo

 

založit nový uživatelský účet

zaregistrujte se

 
zavřít

Nahlásit spam

Opravdu chcete tento příspěvek nahlásit pro porušování pravidel fóra?

Nahlásit Zrušit

Chyba

zavřít

feedback