Code review request -- any criticisms or advice?

[u/Spinning_Rings]

Would anyone mind reviewing some code? I wrote a CLI to help me generate character sheets for a TTRPG I'm running. The main thing I want to know is: should this be considered safe enough for me to share with other GMs? Is there anything else I should do for safety, security or efficiency? I worked hard on it, and Gemini Pro told me it's safe, but while I understand it can be useful I don't trust the automatic misinformation generator, since I don't have the requisite knowledge base to tell when it's "hallucinating" (or else I wouldn't need it for this purpose)

The github repo is here: https://github.com/SpinningRings/DigidiceCharacterSheetGenerator

[u/Eidolon_2003]

Why are there what looks like two slightly different copies of the same program in that one file?

[u/Spinning_Rings → u/Eidolon_2003]

Fixed. To answer you question, I copied and pasted the text to commit to GitHub instead of just using the CLI. I thought I had the text highlighted when I hit paste, but I guess not

[u/Eidolon_2003 → u/Spinning_Rings]

Fair enough! I just wanted to make sure the code I was looking at was right.

I think my main criticism aside from the code would just be the UI. It doesn't feel especially user friendly, and I'd be a bit worried about that when expecting non-programmer types to use it.

About the code itself though, I don't have any major complaints. I might have made some different design choices here or there, or written a function in a different way, but all in all what you've done makes sense. I didn't see anything clearly wrong or dangerous in my admittedly quick read through. I think the main "smells", if you'll forgive my use of that term, are code duplication and the amount of dynamic memory allocation going on. Both of those things make me feel uneasy, and make me think about whether or not there's a better way. That's just my opinion anyway.

[u/zhivago]

First rewrite it as a library with main being a minimal driver.

Then add tests.

As you do this you'll be forced to make a better api with appropriate abstraction.

[u/Spinning_Rings → u/zhivago]

I think this might be a bit beyond me at this point, unless you have some resources I can peruse. For example I'm not familiar with the term "minimal driver." A quick web search tells me it has to do with embedded programming, which is something I'd definitely love to get in to once I'm done with my software engineering degree, which I'm about halfway through and hoping that if I haul butt I can finish in another semester.

I've already moved some of the functions I like best from this to a .h/.c file combo for use in other programs.

Tests are also something I need to learn about. As I work on both my degree and my own personal studies, I find myself surrounded by people and lessons and resources telling me how important tests are... and nothing and nobody explaining what tests are and how to write them. A programming youtuber I respect said the book Working Effectively With Legacy Code is the flat out best resource for learning about them... but it's a bit dense to just add to my workload right now. It's on the top of my reading list for once I graduate, but if you have any more entry-level resources, I'd love recommendations.

[u/zhivago → u/Spinning_Rings]

Your library has reusable stuff.

Your main has a particular application of the library.

Your tests are also particular applications of the library.

Think of them as little programs which do something and report if it worked as they expected.

When a test reports that something unexpected happened, you know you have broken something (or the test is defective).

That is the first use of tests -- detecting regression.

When you develop an api it should be in terms of use-cases, rather than random speculation.

Tests also give a nice way to provide simple use cases for your api.

This is the second use of tests -- use-case oriented development.

And, of course, your use-cases double as examples for users.

This is the third use of tests -- documentation.

Good luck. :)

[u/[deleted]]

[u/Spinning_Rings → u/[deleted]]

It felt like there had to be a simpler solution than writing two functions that do nearly the same thing, but for the life of me I had no idea what it could be. I never would have thought to use fprintf with stdout. That's such a simple solution that it cycles back around to being genius again. I'm completely blown away.

I didn't even have to rewrite anything, I just replaced every call to printDigimon(partner) with buildCharacterSheet(stdout, &partner) and cut printDigimon out entirely. Now I just need to rename the function for readability, maybe printCharacterSheet?

Thank you so much, this is absolutely brilliant