• 1

• 11.9k
• 1
• 20
• 46

Overall, this seems fairly well written. But you said to be relentless and blunt, so here's what I really think! 😉

If you want to investigate it more, or even test it, all dependencies and a test_main.c are ready made in the following Github repo: click here. Litterally just download and press make.

Ugh. This seems to me to straddle the line in the rules for the site:

Be sure to embed the code you want reviewed in the question itself; you can leave supporting, but non-essential, code in links to other sites.

I wouldn't consider missing #includes to be "supporting but non-essential code". If your actual code is separated into a header and a source file, you should post them both separately here rather than inlining the header and omitting the includes.

#Style I have to say that while I am one of these people who likes to line things up to some degree, you've gone overboard with aligning things, in my opinion. For me, it's actually harder to read with your formatting. I don't think that every variable and function declaration throughout the code needs the same spacing. Furthermore, the spacing on functions that go longer than you like makes it look like another function declaration on the next line, but an invalid one. If you're going to wrap the arguments, it's tradition to either indent them one level, or line them up with the function arguments on the previous line.

You should avoid declaring variables until you need them. If a person reading your code wants to know what type a variable is, it's easier to find if it's close to its first use. Likewise, if you want to change a type, it's easier to do if it's near where it's used.

#Naming I think your function names could be shorter. There's a lot of redundant words in their names. For example, hashtable_alloc_table() could just be hashtable_alloc(), and hashtable_insert_entry() could just be hashtable_insert(). (What else would you be inserting into a hash table? A dinner plate?)

Adding t_ to the front of every type is unnecessary. Types are obviously types from the context in which they're used. If you must add something, make it a suffix of _t like every other C programmer so it's consistent. Also, what's the purpose of giving struct names a prefix of s_? Are you ever going to write the type as struct s_whatever instead of t_whatever? If not, just give it the same name, so it's:

typedef struct foo {
 // fields
} foo;

In hashtable_alloc_table(), the argument is called size, but the units aren't clear. My assumption on reading the declaration was that it was going to be in bytes. But it's actually the number of entries to hold. As such, I would name it numEntries rather than size because size is ambiguous.

The difference between hashtable_dealloc_table() and hashtable_destroy_table() is surprising given their names. It seems like hashtable_dealloc_table() should be renamed to hashtable_shrink_table() or something more in line with what it's doing.

The name hashtable_check_load_factor() is also misleading. I wouldn't expect a function which is named "check " to change anything. I would call it something like hashtable_set_appropriate_load_factor() or something like that so that a caller knows that it may change the hash table.

Finally, what does the prefix ft stand for? It's not at all clear from the code you posted. A comment about its meaning somewhere might be appropriate.

#Memory Leak in hashtable_alloc_table() There's a memory leak in 'hashtable_alloc_table(). If the table is allocated, but the bucket list isn't, it returns NULL`, but it never frees the table. That memory is now considered in-use by the OS making it unavailable to be re-used.

#Redundancy Why have the caller pass in a pointer to a pointer to hashtable_realloc_table(), and then return a pointer to a hash table? You should do one or the other. A pointer to a pointer allows you to change the value of the pointer the caller uses, so you don't need to also return the new one. You can simply delete the old one and replace it with the new one.

#Comments I think you have too much info in your function comments. Why should I care which functions you call from that function? I shouldn't need to know that info. I shouldn't need to know which headers include the functions that any given function depends on. They should simply be included at the top of the file (which they aren't here).

The problem with comments is that they can get out of date with the code. That has happened with your hashtable_realloc_table() function. It says that it "Grows the hash table by half", but it actually doubles the hash table size. Perhaps it used to only grow it by half, but now it doubles it?

I have no idea what "search tags" are in this context, and it seems unnecessary. I either already know what to search for or I don't. If I search for the function name, I'll find the function definition, so I don't need the tag.

#Standard Library You say:

Note: I implement my own standard library functions

Oh Heck no! The original implementors of the standard library made all kinds of mistakes in how they designed the standard library functions. Your implementations likely have all those same mistakes plus a whole bunch more due to the fact that you're a beginner at this. It's an interesting exercise to learn the language, but you shouldn't use them in real code.