prisbteam / pcc_processing_design Goto Github PK

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.

For example, consider this line:

It would be way better written as this:

     if (j0s != 0) {
         j0s = jj0* log2(jj0);
     }
     if (j1s != 0) {
         j1s = jj1* log2(jj1);
     }
     if (j2s != 0) {
         j2s = jj2* log2(jj2);
     }
     if (j3s != 0) {
         j3s = jj3* log2(jj3);
     }
     //Gives 0 in entropy!

Yes, it takes more lines of code, but there isn't a global newline shortage.

Some libraries can do more mnemonic approaches, by providing higher level concepts like vectors and quaternions and so on. I don't know if any of those would be applicable to your code; adopting one of those would certainly be a lot of work.

I approve of you making the overall documentation files at the top of the project. That's very very good practice indeed! However...

I recommend making the parts of the documentation that are not exact technical phrases be set in a variable-width font. For example (in TechnicalManual.docx):

\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. The more complicated the document, the more important it is to get these sorts of things right. Particular attention needs to be paid to "technical phrases" (things you might cut-n-paste into a terminal) and terms from a controlled vocabulary (more especially vital when working with standards and conformance documentation; I didn't notice any of that in your current docs but I didn't go through in depth).

You have no tests. How can you know whether your code works right without testing it? (There are a few cases where it's possible, but your code is definitely not in one of them.)

Now, manually testing everything admittedly 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.)

The advantage with having defined tests set up (that you can run with cmake --test) is that the tests remember things that should work so that you don't have to spend so much effort on checking whether things work yourself. I recommend testing whether the code does what it should when presented with (small pieces of) input data that it should receive, and, for bonus points, that it also does something somewhat sensible when it receives input data that it shouldn't.

You have 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.)

I recommend putting brief descriptions (@brief) in header files (together with the likes of @param and @return) but leaving detailed descriptions (@details) for the .cpp files where you do the implementations. That seems to produce the best results overall.

For example, in PCC_Writer.h (no @return because there's no result value):

/*! 
 * @brief Write out the structure after the program has computed its contents.
 * @param new_cells_design Structure to write out
 */
void PCC_Writer(CellsDesign &new_cells_design);

In PCC_Writer.cpp:

/**
 * @details
 * Read PCC Writer specifications from the writer.ini file and output of the
 * current configuration to the screen and .log file.
 */
void PCC_Writer(CellsDesign &new_cells_design) {
    ....

Building on #11 and #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.

Here's an example section (from one of my projects) of build.yml to enable that:

    - name: Build Project
      uses: threeal/[email protected]
      with:
        run-build: true
        run-test: true
        test-args: -C Debug
      env:
        CMAKE_BUILD_TYPE: Release
      timeout-minutes: 10

Specifically, adding run-test: true is the key part.

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...

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 #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 #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 #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 #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 #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 #9)

• I recommend making the parts of the documentation that are not exact technical phrases be set in a variable-width font. (See #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 #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 #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 #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 #14)

It's strongly recommended to normally #include all headers to make a file "work" in that file, instead of assuming that the context will supply them. An example of that is this file:

Right now, my IDE is complaining that it doesn't know what std::vector is when it has that file open... because of a missing #include <vector> at the top of the file.

NB: This issue has been Good Practice in C++ since the 1990s at least.

There are some exceptions to this rule. Your code appears to not be using any of the exceptions.

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.

Using a single style helps your code be a lot easier to read. Most of your code seems to be using 4 spaces as the basic indentation unit, which is good, but it definitely isn't universal and that makes things look very sloppy.

There are multiple options for doing code style enforcement. It's a much messier topic in C++ (and C) than in some other languages. However, until you have defined rules for what things ought to be like, enforcement in any form is impossible.

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.

Pick an option and tidy things up:

1. Different comment-out style
2. Delete outdated code
3. Bring code back into service

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.

You need to fix #6 and #12 first before we can really help much with this, and some parts of it require alterations to your project's settings (to enable publication of a documentation tree). You'll probably want to call us back on this once you've resolved the preceding issues.