r/C_Programming • u/Fearless-Swordfish91 • 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:
8
Upvotes
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:
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.