Použití struct

Osobně si myslím, že jste se rozhodli správně pomocí struct s char* a int místo 2 polí char* a int. Pair je konceptuální datová jednotka ve vaší aplikaci, takže je logické dát je dohromady. Pokud byste měli 2 samostatná pole, bylo by pro ně snadné dostat se ze synchronizace a těžko ladit, proč se to stalo. Dobrá práce!

Preferujte const a funkce před makry

Velikost pole jste definovali pomocí makra. Toto dává jste při programování v nevýhodě. Odebrali jste informace o typu hodnoty. Tím, že to uděláte:

const int ARR_SIZE = 10; 

získáte bezpečnost typu. Upravit: To je „C ++ – ism, který to neudělá“ t práce v C. můj špatný! Ale zbytek doporučení v následujícím odstavci je správný, pokud vím.

U maker, která berou parametry, riskujete, že budou použity neočekávaným způsobem a způsobí těžké problémy s laděním. Naštěstí jste to tady neudělali. Ale obecně, pokud zjistíte, že sáhnete po makru, zeptejte se sami sebe, zda by vám bylo lépe s konstantou nebo funkcí. Budete téměř vždy (za předpokladu, že můžete použít konstantu požadovaným způsobem).

Chyby

Ve vašem kódu jsou některé chyby. V make_pair() nezkontrolujete, zda pokud malloc() uspěl. Pokud selže, není přidělena žádná paměť a pair ukazuje na NULL. Při pokusu o přiřazení pair->val nebo pair->word dojde k chybě.

Pokud jste to opravili, table_insert() použije výsledek make_pair(), aniž by zkontroloval, zda je nejprve NULL. Toto zvítězilo “ Okamžitě nepadne, protože právě přiřazujete ukazatel pole v tbl->pairs[tbl->sz], aby měl hodnotu pair. Co se stane později, když pokusíte-li se vyhledat nebo vytisknout nebo vložit jinou položku, dojde k selhání při iteraci nad touto položkou v tabulce a pokusem se s ní cokoli dělat.

Tyto chyby můžete znemožnit tím, že dynamické přidělování položek pole. Jednoduše z pole udělejte pole Pair struktur, spíše než ukazatel na ně.

Pojmenování

Mnoho vašich jmen je opravdu dobrých . Pair a Table jsou slušné a čitelné názvy pro tento úkol. make_pair(), table_insert() atd. jsou informativní. Některé by však mohly být vylepšeny. tbl vám při psaní table příliš nešetří psaní. Stačí použít celé slovo. Totéž s sz vs. size. i je přijatelný jako název proměnné smyčky, ale bylo by ještě lepší, kdyby byl popisnější. Například entry_index nebo pair_index. Mělo by to popisovat, na co „se vracíte.“

Komentáře

• když změním definici na const int ARR_SIZE = 10; dává mi chybu v struct Table { struct Pair* pairs[ARR_SIZE]; size_t sz; };, že ARR_SIZE není const, tak jak ji tedy použít v tomto případě?
• Omlouvám se – že ‚ sa C ++ – ism. Myslel jsem, že se jedná o otázku C ++. Moje chyba. ‚ Aktualizuji svoji odpověď.
• I ‚ podporuji popisné názvy proměnných, ale díky jednoduchosti smyček v tomto programu si myslím, že název proměnné i funguje dobře.
• Pokud malloc() selže, přiřadí se pair->val nebo pair->word může dojít k chybě, pokud máte štěstí . Může to jen pokračovat a dát špatné výsledky, nebo to může udělat něco naprosto neočekávaného. To ‚ je radost z nedefinovaného chování!

• Nevrhejte výsledek malloc.

  malloc vrací void * a v jazyce C je platný převod void * na libovolný ukazatel. Pokud použijete k potlačení upozornění na převod celého čísla na ukazatel, znamená to, že kompilátor nemá malloc prototyp a ve výchozím nastavení vrátí int (jedná se o starou konvenci C). Nyní, pokud int a ukazatel mají různou velikost, návratová hodnota malloc by byla zkrácena, se všemi ošklivými důsledky.

• Nedoporučuje se sizeof(type). Upřednostňovat sizeof(expression). Ve vašem případě zvažte

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

• table_insert slepě vloží do celé tabulky. Mělo by se testovat na tbl->sz < ARR_SIZE a vraťte indikaci chyby, pokud tomu tak není.

• Vlastní vložení

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

  by měl být opravdu jeden řádek:

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

Komentáře

• proč není dobrý nápad vrhat malloc? V jazyce C ++ by dokonce došlo k chybě kompilátoru?
• @Sa ndro4912 Otázka je označena C. C ++ je jiný jazyk, zejména v tomto ohledu. Pokud si nyní kompilátor C stěžuje, znamená to, že chybí prototyp malloc, což může vést k nepříjemným problémům.
• ano, já vím, že je to c. Jen mě zajímalo, proč zde není dobrým zvykem označovat změny typu s obsazením
• @ Sandro4912 Viz úpravy.
• @ Sandro4912 pair = malloc(sizeof *pair) je jednodušší správně kódovat poprvé, snadněji se kontroluje a udržuje.

Povolit vše varování kompilátoru

S dobře aktivovaným kompilátorem by měl for (int i = 0; i < tbl->sz; ++i) varovat před smícháním signatur i a tbl->sz a také rozsah. Ušetřete čas a povolte všechna varování a používejte for (size_t i = 0; i < tbl->sz; ++i).

Obecně je kód sporný při používání int,size_t téměř zaměnitelně . Změním architekturu a použiji pouze size_t.

Smíšené použití mělké a hluboké kopie

make_pair(const char* word, int val) přiděluje a tvoří zcela nový struct Pair (hluboké kopírování), ale nekopíruje to, na co word ukazuje.

Možná

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

Použijte const

search_word() nemění *tbl, proto použijte 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) 

Pojmenování

Kód používá const char* word; přesto to není „slovo“, ale řetězec , jak se používá v strcmp().

—– Dodatky

Porušení smlouvy?

Nic v požadavcích „Implementace vyhledávací tabulky s operacemi jako …“ nenaznačuje, že const char* je ukazatel na řetězec . Takže volání strcmp() je sporné, pokud neexistují nestátní požadavky. Jako char * by kód mohl použít jednoduché porovnání

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

Použití assert()

je moje použití prosazení vhodné?

Pokud ukazatel char * pro přidání / hledání není zadán jako řetězec , assert(word); není správný jako word == NULL není známo, že je neplatná.

assert(return_val) v table_find(struct Table* tbl, const char* word, int* return_val) je v pořádku, přesto bych přepracoval návrh tak, aby return_val == NULL

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