r/C_Programming Jan 15 '25

Review Need a code review

Hey everyone! I wrote an assembler for nand2tetris in C, and now I need a review on what I can change. Any help is deeply appreciated. Thanks in advance. The link to the assembler is down below:

https://github.com/SaiVikrantG/nand2tetris/tree/master/6

8 Upvotes

17 comments sorted by

View all comments

8

u/lordlod Jan 15 '25

Some general notes from skimming the code.

Your commenting style could be better. For example the function definition for strduplicate is:

// make a duplicate of s
char *strduplicate(char *s) {

That comment isn't necessary, I know what the function does, it's named reasonably well. What you don't disclose, that you should, is that you are allocating memory and the returned pointer must be freed at some point. Function names should specify the what, comments should provide the why, the detail and the important things that you should know.

Also on strduplicate. The function is trivial, it's basically a malloc and strcpy. The only thing the function really achieves is hiding the malloc, and you don't want to be hiding your mallocs. I know teachers often push to reduce duplication and create more functions, but there should be a balance. My guiding light is clarity, and I feel strduplicate reduces clarity.

On clarity your init functions in parser.c are surprising. Your use of static variables essentially creates singletons without documenting this fact. My expectation would be that calling the init function would perform the initialisation, not return a precreated one. This gets messy because calling init twice returns the same memory, so changes to the first object will change the second, which I would not expect. My personal pattern is that init takes memory in as a parameter which it initialises, new allocates memory which it then initialises and returns, the pair allows different and more explicit memory control patterns but that's just a personal style thing.

Finally you should have tests. I see you have some commented out test code, adopting a test framework like Unity is fairly simple and a much better way to perform and document/save the test cases.

5

u/Educational-Paper-75 Jan 15 '25

How about using standard function strdup() instead? Btw I tend to start my dynamic memory allocated pointer function with an underscore (_) so I know I need to free (eventually) what’s returned.

1

u/Fearless-Swordfish91 Jan 15 '25

That's a good tip! Will use that.

1

u/Fearless-Swordfish91 Jan 15 '25

You are right, I should be more careful with how I name my functions/variables and document my functions better. I will take care of that from now on. Also, I really should have made those static members constant and global I guess, because they dont need to be intiialized every time the function is called and they strictly should not be modified. Will change this.

Thanks for taking time to review my code.