Ved hjelp av struct

Personlig tror jeg du tok det riktige valget ved å bruke en struct med en char* og int i stedet for to matriser med char* og int. Pair er en konseptuell dataenhet i appen din, så det er fornuftig å sette dem sammen. Hvis du hadde to separate matriser, ville det være enkelt for dem å komme ut av synkronisering og vanskelig å feilsøke hvorfor det skjedde. Fint arbeid!

Foretrekker const og fungerer fremfor makroer

Du har definert matrisestørrelsen ved hjelp av en makro. Dette gir du har en ulempe mens du programmerer. Du har fjernet typeinformasjonen for verdien. Ved å gjøre det:

const int ARR_SIZE = 10; 

får du typesikkerhet. Rediger: At «sa C ++ – isme som ikke» t jobbe i C. My bad! Men resten av rådene i neste avsnitt er riktige så vidt jeg vet.

Med makroer som tar parametere, risikerer du at de brukes på uventede måter og forårsaker problemer med feilsøking. Heldigvis har du ikke gjort det her. Men generelt, hvis du finner deg selv å strekke deg etter en makro, spør deg selv om du har det bedre med en konstant eller en funksjon. Du vil nesten alltid være (forutsatt at du kan bruke konstanten på ønsket måte).

Feil

Det er noen feil i koden din. I make_pair(), sjekker du ikke for å se hvis malloc() lyktes. Hvis det mislykkes, tildeles ikke noe minne, og pair peker til NULL. Når du prøver å tilordne pair->val eller pair->word, vil du krasje.

Hvis du fikste det, table_insert() bruker resultatet av make_pair() uten å sjekke om det «s NULL først. Dette vant» t krasjer umiddelbart, fordi du bare tilordner arraypekeren i tbl->pairs[tbl->sz] for å ha verdien pair. Hva vil skje er senere når du prøver å søke eller skrive ut eller sette inn et annet element, vil du få et krasj når du gjentar over den oppføringen i tabellen og prøver å gjøre noe med det.

Du kan gjøre disse feilene ikke mulig ved ikke å dynamisk fordeling av matrisepostene. Bare gjør matrisen til en matrise med Pair -strenger snarere enn en peker til dem.

Navngivning

Mange av navnene dine er veldig bra . Pair og Table er anstendige, lesbare navn for denne oppgaven. make_pair(), table_insert() osv. er informative. Noen kan skjønt forbedres. tbl sparer deg ikke mye for å skrive over table. Bare bruk hele ordet. Samme med sz vs. size. i er akseptabelt som loop-variabelnavn, men det ville vært enda bedre hvis det var mer beskrivende. For eksempel entry_index eller pair_index. Den skal beskrive det du gjentar.

Kommentarer

• når jeg endrer definere til const int ARR_SIZE = 10; det gir meg en feil i struct Table { struct Pair* pairs[ARR_SIZE]; size_t sz; }; at ARR_SIZE ikke er const. Så hvordan bruker jeg det i dette tilfellet da?
• Beklager – at ‘ sa C ++ – ism. Jeg trodde dette var et C ++ spørsmål. Min dårlige. Jeg ‘ vil oppdatere svaret mitt.
• Jeg ‘ er en sterk tilhenger av beskrivende variabelnavn, men med enkelheten til løkkene i dette programmet tror jeg variabelnavnet i fungerer helt fint.
• Hvis malloc() mislykkes, tildeles pair->val eller pair->word kan krasje, hvis du er heldig . Det kan bare fortsette og gi gale resultater, eller det kan gjøre noe helt uventet. At ‘ er gleden over udefinert oppførsel!

• Ikke kast resultatet av malloc.

  malloc returnerer void *, og i C er det gyldig å konvertere void * til en hvilken som helst peker. Hvis du kaster for å undertrykke en advarsel om konvertering av heltall til peker, betyr det at en kompilator ikke har en malloc prototype, og som standard er den å returnere en int (det er en eldgammel C-konvensjon). Nå hvis int og pekeren er av forskjellig størrelse, vil malloc-returverdien bli avkortet, med alle stygge konsekvenser.

• Det anbefales ikke å sizeof(type). Foretrekker sizeof(expression). I ditt tilfelle bør du vurdere

   struct Pair * pair = malloc(sizeof(*pair)); 

• table_insert setter blindt inn i en full tabell. Den skal teste for tbl->sz < ARR_SIZE og returner en feilindikasjon hvis den ikke er det.

• Selve innsettingen

   struct Pair* pair = make_pair(word, val); tbl->pairs[tbl->sz] = pair; // add pair at the last position 

  bør egentlig være en enkelt linje:

   tbl->pairs[tbl->sz] = make_pair(word, val); 

Kommentarer

• hvorfor er det ikke en god ide å kaste malloc? I c ++ vil det til og med få en kompilatorfeil?
• @Sa ndro4912 Spørsmålet er merket C. C ++ er et annet språk, spesielt i denne forbindelse. Nå hvis C-kompilatoren klager, betyr det at malloc prototypen mangler, noe som kan føre til stygge problemer.
• ja jeg vet er c. Jeg ble bare lurt på hvorfor det ikke er noen god praksis å indikere typen endringer med en rollebesetning
• @ Sandro4912 Se rediger.
• @ Sandro4912 pair = malloc(sizeof *pair) er lettere å kode riktig første gang, lettere å gjennomgå og vedlikeholde.

Aktiver alle kompilatoradvarsler

Med en godt aktivert kompilator burde for (int i = 0; i < tbl->sz; ++i) ha advart om å blande tegn på i og tbl->sz samt rekkevidde. Spar tid og aktiver alle advarsler og bruk for (size_t i = 0; i < tbl->sz; ++i).

Generelt er koden squishy når du bruker int,size_t nesten om hverandre . Jeg ville bare re-arkitekt og bruke size_t.

Blandet bruk av grunne og dype kopier

make_pair(const char* word, int val) tildeler og danner en helt ny struct Pair (dyp kopi), men kopierer likevel ikke det word peker på.

Kanskje

// pair->word = word; pair->word = strdup(word); 

Bruk const

search_word() endrer ikke *tbl, så bruk const to convey that. Same for table_find () , table_print () , print_search_result () `.

// int search_word(struct Table* tbl, const char* word) int search_word(const struct Table* tbl, const char* word) 

Navngivning

Kode bruker const char* word;, men det er ikke et «ord», men en streng som brukt i strcmp().

—– Tillegg

Kontraktsbrudd?

Ingenting i kravene «Implementere en oppslagstabell med operasjoner som …» indikerer at const char* er en peker til en streng . Så å ringe til strcmp() er tvilsom med mindre det ikke er oppgitte krav. Som en char * kan kode bruke en enkel sammenligning

// if (strcmp(tbl->pairs[i]->word, word) == 0) if (tbl->pairs[i]->word == word) 

Bruk av assert()

er min bruk av påstand passende?

Hvis char * pekeren for å legge til / søk er ikke spesifisert for å være en streng , assert(word); er ikke riktig da word == NULL er ikke kjent for å være ugyldig.

assert(return_val) i table_find(struct Table* tbl, const char* word, int* return_val) er OK, men jeg vil designe om for å tillate return_val == NULL

if (i != -1) { // add if (return_val) { *return_val = tbl->pairs[i]->val; } return 0; }