Kod w C do sprawdzenia

Witam,

Mam problem z kodem w C. Mam za zadanie przeanalizowac kod zalaczony ponizej pod katem bledow, niespojnosci, zlych praktyk i znalezc najgorszy blad i zasugerowac jego poprawe.

Nie oczekuje, ze ktos rozwiaze za mnie zadanie, NIE ale PROSZE o to czy ktos z doswiadczonych programistow moze spedzic 5 minut i zerknac na ten kod i powiedzic czy dostrzega jakeis bledy w C ? Ja usiadlem nad tym i nie moge nic znalezc, albo za malo wiem albo jestem slepy ...

czas mi ucieka, bo mam zadanie oddac na poniedzialek i postanowilem zasiegnac porady na forum. czy jest ktos w stanie pomoc ?

http://www.menie.org/georges/embedded/xmodem.html

ok, domyslam sie, ze uzycie goto jest bledem i ze nie kazdy case w switch'u nie ma break'a.

domyslam sie, ze uzycie goto jest bledem
Nie jest to błąd, tylko zła praktyka.

nie kazdy case w switch'u nie ma break'a
Tzw. fallthrough także nie jest błędem, a celowym działaniem. Na przykład jeśli chcesz, aby kod wykonywał się dla liczb 2, 3 oraz 5, to robisz:

switch (liczba) {
  case 2:
  case 3:
  case 5:
    cośtam;
  break;

Ewentualnie autor wykorzystuje goto, więc ten brejk tak czy owak byłby zbędny.

Z tego co odkryłem:

1. Autor nigdzie nie dostał olśnienia, że istnieje typ size_t, a szkoda. To nie jest błąd per se, ale może być błędogenne, a już na pewno namawia do przemyśleń dlaczego autor przewidział odnoszenie się do tablicy po ujemnym indeksie, po to tylko, aby się okazało, że jednak nie przewidział ;p
2. To pewnie kwestia wieku tego kodu, natomiast już nie korzysta się z modyfikatora register (por. register int count = destsz - len;), ponieważ kompilatory i tak lepiej wiedzą, co gdzie wsadzić ;-)
3. Funkcje xmodemReceive oraz xmodemTransmit są zdecydowanie za długie i mają zbyt duży poziom zagnieżdżeń.
4. Nie rozumiem, dlaczego autor raz robi case 'C':, a potem case NAK: - tzn. dlaczego raz na pałę wstawia literał, a za drugim razem wykorzystuje zdefiniowane wyżej NAK. Ja wszystko oparłbym o stałe/define'y, aby nie mieszać.
5. for (i = 0; i < (bufsz+(crc?1:0)+3); ++i) { robi mi się gorąco gdy patrzę na ten warunek - bynajmniej nie z podniecenia.
6. Kody błędów są liczbowe, wpisane na chama prosto w kod (np. return (c == ACK)?len:-5;) - tak się nie robi. return (c == AKC) ? len : ERR_NO_ACK_RECEIVED;, z uprzednią definicją #define ERR_NO_ACK_RECEIVED -5 (nazwa przykładowa) jest znacznie czytelniejsze.

To tak pobieżnie - doczepić można by się tutaj praktycznie co drugiej linijki.

Czesc Patryk,

Dziekuje za szybka odpowiedz.

for (i = 0; i < (bufsz+(crc?1:0)+3); ++i) ... w sensie, ze '(bufsz+(crc?1:0)+3)' powinno byc zdefiniowane wczesniej poza funkcja ?

Choćby i było zdefiniowane wcześniej.
Cokolwiek, aby ten brzydki warunek nie był taki... brzydki.

Jak robię review, takiego kodu to automatem pierwszy zarzut: funkcje są za długie, podziel to najmniejsze.
Czemu? Bo przy takich monolitach nie da się zrobić uczciwego code review, bo sprowadzi się on do poprawek literówek i standardów formatowania kodu, a samej logiki się nie zrozumie (a z architekturą będzie ciężko).

Dzieki Marek,

a co myslisz o braku komentarzy ? Czy jest to zla praktyka ?

Komentarze tylko tam, gdzie rzeczywiście sa niezbędne, ew w nagłówkach, zeby ładnie tworzyła się dokumentacja automatyczna. Ogólnie, jeśli nie robisz jakiś "sztuczek" albo innych niekonwencjonalnych rzeczy to kod ma byc czytelny na tyle, aby dokumentacja nie była potrzebna.
dodano:
A ew jeszcze robie sobie rzeczy typu [todo] - ale to pewnie ze wzgledu na wiek, aby nie zapomniec, ze piszac i testujac, jeszcze czegos nie zrobilem.

czy ktos moglby zamiescic jeszcze jakies uwagi / spostrzezenia co do zamieszczonego kodu ? dzieki

To raczej wieloetapowe, poczytaj co napisał @MarekR22 tu: http://4programmers.net/Forum/1209978
Jak poprawisz to co już wiesz to wklej kod, wtedy będzie nieco jaśniej.

a czy samo uzycie funckji for w takiej postaci jest bledem ?

for(;;)

Pijany Krawiec napisał(a):

a czy samo uzycie funckji for w takiej postaci jest bledem ?

for(;;)

znaczy sie rozumiem, ze nie ma warunkow i funckja bedzie dzialac w nieskonczonosc, chybaze mam break w funckji.

Jak rozumiesz to czemu pytasz?

1. for nie jest funkcją.
2. A pisanie for (;;) nie jest błędem - jedni wolą while (1), a inni for (;;). Inna sprawa, że nieskończona pętla stanowi niezbyt zalecaną konstrukcję, ponieważ nie jest jasne, kiedy kod się przerywa (wiadomo, że po break, ale warunek nie jest ukazany wprost). Podobna sprawa jak w przypadku goto.

czesc, wrzucilem kod w splinta i lece linijka po linijce i zastanawiam sie nad:

if(crc)  

<- Blad z tego wzgledu, ze crc jest zdeklarowane wczesniej jako int a warunek musi by bool

unsigned short tcrc = (buf[sz]<<8)+buf[sz+1];

<- to bym rozbil na dwie linie kodu i najpierw zdeklarowal <code class="c">(buf[sz]<<8)+buf[sz+1]

```c
crc16_ccitt 

<-- jest niezdeklarowane

unsigned short tcrc = (buf[sz]<<8)+buf[sz+1];

<-- czy to jest bledem ze tcrc jest short a po prawej wynik jest int ?
unsigned char cks = 0; <- czy takie cos jest bledem ? czy cks powinno bys zdeklarowane jako int ?

funkcja flushinput

while (_inbyte(((DLY_1S)*3)>>1) >= 0)

  rownanie zrobil bym osobno po za petla a dopiero pozniej bym wrzucil jako zdefiniowana wartocs w warunek petli

funkcja xmodemReceive

if (trychar) nie jest bool

co myslicie o tym ? jezeli jest ok to przejde przez kod dalej linijka po linijce

Patryk27...czy moglbys udzielic troche info nt w ktorej lini jest odniesienie do tablicy po ujemnym indeksie bo ja tego nie widze ...

Pijany Krawiec napisał(a):

Patryk27...czy moglbys udzielic troche info nt w ktorej lini jest odniesienie do tablicy po ujemnym indeksie bo ja tego nie widze ...

Nie ma - ale skoro autor wykorzystuje typ int, to daje do myślenia gdzie niby indeks może być ujemny (ponieważ int jest, zgodnie ze standardem C, liczbą ze znakiem). Dla indeksów powinno się w 99.9999% wykorzystywać size_t (mało kiedy ujemny indeks ma jakikolwiek sens).

Hej,

Mam pytanie odnosnie petli for i warunku

for (i = 0; i < (bufsz+(crc?1:0)+3); ++i)

jak zapisac to aby wygladalo lepiej ? domyslam sie, ze moge zastapic 'ternary operator' warunkiem if tylko nie wiem jak bo crc przyjmuje wartosc albo 0 albo 1 i jest zdefiniowane jako c

czy moglby mnie ktos poprawic ?

w tym mencie zrobilbym to tak gdyby crc bylo bool:

zdefiniowalbym

if crc {
unsigned zzz = bufsz + 4
for (i = 0; i < zzz ; ++ 1)
if ((c = _inbyte(DLY_1S)) < 0) goto reject;
*p++ = c;
}
else {
unsigned yyy = bufsz + 3
for (i = 0; i < zzz ; ++ 1)
if ((c = _inbyte(DLY_1S)) < 0) goto reject;
*p++ = c;
}

Ma to być w funkcji:

size_t size=bufsz+3+(crc>0);
for(unsigned char pend=p+size;p<pend;++p) if((*p=_inbyte(DLY_1S))<0) return false;
return true;

Poza tym podejrzewam że wystarczy zawsze dać +4 owszem czasami jeden krok za dużo, ale za to niewielka cena za lepsza czytelność.