Code Review

[dkfellows]

Hi, I've been asked to do a quick code review (as a senior Research Software Engineer). I'm definitely not going to pick up all the issues in your code, but I'll at least spot some of things that probably ought to be looked at.

• Lots and lots of commented out code. It's my experience that code that is commented out rots on the vine as it fails to keep up with changes elsewhere. It's marginally better to use #if 0/#endif to prevent the compiler from building it, since then you get at least some basic syntax checking from your IDE, but it's actually better still to either delete things that are no longer wanted (git preserves them in the commit history; you don't lose them!) or to make them actually live code. (See Commented out code  #4)

• Check that output data is being written to where you think it should be. It looks like PCC_Writer.cpp is writing to standard out, not the output stream which it sets up. There's a variance between what it does and the comments at the very least... (See Data destination wrong? #5)

• Not many doc comments. Though there's some other comments that could probably be repurposed. (Doxygen-style comments are a little finicky, but putting API documentation in your header files is a very good idea.) (See Few doc comments #6)

• In my experience it's best to split code over several lines and use {curly braces} even in cases where they're technically not necessary. It's longer but really boosts the readability. (See Split code over multiple lines #7)

• Pick one indentation rule for your code. Use only that indentation rule. Use it everywhere in your code (never mind what third-party dependencies do). In general, that applies to everything to do with basic code style; pick one rule (there are many viable options; I have my favourites but it's not my code) and stick to it. (See Code style #8)

• It's strongly recommended to #include all headers to make a file "work" in that file, instead of assuming that the context will supply them. (See More #includes #9)

• I recommend making the parts of the documentation that are not exact technical phrases be set in a variable-width font. (See Doc formatting #10.) For example:

  \src — contains all the source files, <*.h> libraries of the project and the main.cpp file. In particular, \src directory contains several subfolders with project libraries:

  would be better as:

  \src — contains all the source files, <*.h> libraries of the project and the main.cpp file. In particular, \src directory contains several subfolders with project libraries:

  It makes the different parts "pop" correctly to the eye of the reader-in-a-hurry.

• You have no tests. How can you know whether your code works right without testing it? Manually testing everything gets really boring, especially when you can test many things automatically once you set them up. (Since you're already using CMake, adding in CTest is pretty simple. See Testing! #11)

• It's a very good idea to set up Github Action automation to build your code on every commit. It lets you find out problems early. (See Automated build #12)

• It's a very good idea to have your Github Action automation run your tests on every commit. Doing that automatically is great because then you don't forget just because you happened to have bad day for some reason. (See Automated testing #13)

• It's a very good idea to have your primary branch's documentation automatically built and deployed to an (internal?) site. It's not a huge amount of work to set up, yet it helps your whole software project feel ever so much more professionally done. You're already part of the way there with Doxygen. (See Documentation build and deployment #14)