Mennyire "clean", ha a for ciklus feltételénél nem a ciklusváltozót, hanem egy másik változót vizsgálok (elsősorban C#, de bármilyen nyelven érdekel)?

Az a probléma vele, hogy a ciklusmagban lévő IF szerkezetben megadott kifejezés lehet, hogy soha nem lesz igaz, ami miatt beállhatna egy végtelen ciklus... hacsak nem rontanád tovább a helyzetet azzal, hogy a j változód átmehet negatívba, ami miatt IndexOutOfRangeException kivétel miatt meghal a programod (megmentve a gépet a végtelen ciklustól).

Ha nagyon ezt akarod, vedd be legalább a for ciklus feltételébe, hogy a j 0-nál kisebb lehetőleg ne legyen.

int k = 0;

int j = temp.Length;

while(k <= WhichLott)

{

if(temp[j] != "")

{

lotNums.Add(int.Parse(temp[j]));

j--;

k++;

}

}

Ez sokkal jobban kifejezi a működést. Hogy mennyire clean azt mindenki döntse el maga. De pl. 'temp', 'k' és 'j' változónevek ordítóan nem 'clean'-ek.

For ciklusnál érdemes kerülni a ciklusmagban a ciklusváltozó módosítását.

Pont ezt akartam mondani mint az előző.

Már ott nem clean a dolog hogy k-nak nevezel el valamit.

A clean code arról szól, hogy úgy tudom felolvasni a kódodat, mint egy verset. Na most ezt nem tudom. Fogalmam sincs mi az a lotNums lista. Csak tippre mondanám: lottó számok? Akkor legyen az a neve hogy: lotteryNumbers

ez a WhichLott ez egyenesen hátborzongató, fogalmam sincs hogy miért reprezentál egy integert.

a "" helyett használd inkább a string.Empty property-t.

Gondolom kezdő vagy még, ne add fel, de ez még messze van a clean code-tól. Nem lehet minden esetben elkerülni természetesen, de az egymásba ágyazott ciklusokat, if statementeket is kerülni kéne. Itt úgy lehetett volna megoldani hogy:

if (temp[j] == string.Empty)

continue;

lotteryNumbers.Add(Convert.ToInt32(temp[j]));

k++;

Nem látom a kódodat, de gyanítom hogy a WhichLott rossz naming convencióval van ellátva.

Ha egy szimpla helyi változót deklarálsz, akkor camelCase-t kell használni.

Ha egy privát változót deklarálsz, akkor alulvonás _camelCase-t kell használni.

Ha pedig property-t használsz, akkor meg PascalCase-t.

További produktív programozást kívánok, gyakorolj sokat. Ellenőrizd a kódot, és próbáld meg úgy felolvasni mint egy verset. :)

#8 Egyrészt, mert ha van egy expliciten erre kitalált property az osztályon, akkor célszerű azt használni, másrészt mikroszkopikus mértékben hatékonyabb, mert nem példányosít egy új objektumot (a "" literál meg igen).

Amúgy a kód "clean"-ségére nem mennék rá, viszont funkcionálisan erősen problémás. Egy ciklusra fontos, hogy megfogalmazható legyen egy ún. terminálófüggvény, ami vagy része a kódnak, vagy csak absztrakt módon megfogalmazható rá. A terminálófüggvény lényege, hogy valamilyen számértékkel rendelkezik a ciklus elején, a ciklus futása során az értéke biztosan csökken, és soha nem nő, és ha 0-hoz ér, a ciklus végetér. Ez az absztrakt programkonstrukció igazából egy egyszerű szempontot formalizál: A ciklusod biztosan fejeződjön be valamikor. Ne legyenek végtelen ciklusok. Jelen esetben, mivel a k változó csak eg ybizonyos feltétel mellett nő, és ez a feltétel nem feltétlenül fog teljesülni, a ciklus a végtelenségig futhatna.

Mindezen felül a ciklusnak biztonságosnak is kell lennie, soha nem szabad megtörténnie, hogy a cikluson belül valamilyen változó olyan értéket vesz fel, ami hibára vezet. Jelen esetben a j változó nem mehet 0 alá, tehát a ciklus fejlécében ki kell kötnöd, hogy a j érétke nagyobb kell legyen 0-nál. Ez önmagában megoldja az előző problémát is, mert a j értéke minden lépésben csökken, tehát elkerülhetetlenül el fog érni a ciklus a végére. Ha neked az a célod, hogy pontosan WhichLott darab számot válassz ki, akkor el kell gondolkoznod: Gaarntálni tudod, hogy mindig lesz ennyi kiválasztható szám? Ha nem, akkor le kell kezelni azt az esetet is, hogy nem találsz elegendő mennyiségű számot, például a ciklus lefutása után ellenőrizheted k érétkét, hogy megegyezik-e a WhichLot értékével. Ha igen, akkor találtál k darab számot, ha nem, akkor nem.

+1 int.Parse helyett a TryParse-t használd, az nem fog elszállni, ha nem számot kap.