Suggestions for code readability

[jwuttke]

To improve readability of the source code, I'd suggest the following:

• Regarding code formatting:

  • Run clang-format -i over all cpp and hpp files to restore compliance with the extant style file .clang-format.
  • Remove outcommented code (it's under version control)
  • Change .clang-format to impose a maximum number of characters per row (100 ?)
  • Change .clang-format to allow no more than two consecutive blank lines
  • Change .clang-format to break line after if conditions
  • Personally, I would further simplify things by replacing all tabs by four spaces, but I presume explicit use of tabs is a conscientious choice of yours.

• Regarding code structure:

  • I would expect class Binder to be defined and implemented in files binder.(c|h)pp. So why not rename binder.(ch)pp to main.(ch)pp, and split of a new binder.(ch)pp from context.(ch).pp?

If you agree with any or all of these, I would be available to carry the changes out and submit them through PRs.

[lyskov]

Let me address these one-by-one:

run clang-format -i

-- yes, at this this point it is probably a good idea to do this since it will simplify merging outside PR's. So such PR will be welcome. When/if this is PR is created lets keep it as reformat-only (ie no other code refactoring) to simplify merging.

Remove outcommented code (it's under version control)

-- While i do like to remove some of the commented out code (clearly this is overdue!), I however do not want all such commented code to be removed (i tried to relay on SC in the past and did not like that much, - doing Git archeology for an hour to recover few lines of code is not fun!)

Change .clang-format to impose a maximum number of characters per row (100 ?)

-- sure, i will be open to impose such limit. However 100 is way too low these days (after all we all have wide screens now, not a tty with 80 chars). So how about ~200?

Change .clang-format to allow no more than two consecutive blank lines
Change .clang-format to break line after if conditions

-- sorry but no on both counts

Personally, I would further simplify things by replacing all tabs by four spaces, but I presume explicit use of tabs is a conscientious choice of yours.

-- Not sure how this will simplify things but let's not revisit the old tabs-vs-spaces debate (i decided to stop participating in these ~25 years ago and feel much better now!). So let's focus on practicality: Binder is part of RosettaCommons Rosetta project which uses tabs so it will be best to use tabs here as well.

I would expect class Binder to be defined and implemented in files binder.(c|h)pp. So why not rename binder.(ch)pp to main.(ch)pp, and split of a new binder.(ch)pp from context.(ch).pp?

-- yes, this sounds like a good idea to me, - such PR would be welcome!

Thanks,

[jwuttke]

Good. Partial acceptance is the best I could reasonably hope for, thank you very much for this. I will submit two PRs accordingly.

Let me respond to one point only: line length. The desire for limitation does not come from hardware or terminal emulations, but from the physiology of eye movement. That's why classical book setting limits lines to 66-80 characters. In reading code, we can occasionally accomodate longer lines. Also, often blocks are indented. Hence my proposal of a hard limit of 100. Possibly 120. A higher limit would do more harm than good because people working with a limited editor frame would then be confused by two different kinds of line wrappings, those done by the editor and those in code.