Replace monolithic design with modular code

[phelps-sg]

Related to #166, the code is currently very monolithic with some very large functions comprising several hundred lines e.g. RecordSample() in CovidSim, and much duplicated logic and state. This will make it hard to write sensible unit-tests for the simulation. The readability and overall quality of the code would be much enhanced if large functions were broken into smaller methods, and if good OO design was used to eliminate repetition. This would also allow a unit-test suite to be developed as per #166.

[Feynstein]

I had an issue on that subject called... Object oriented pass on CovidSim.cpp but as I appropriated the code I saw that it needs to go further. So I support this issue. It can be done at first by adding files in the program but eventually I think it would be better to make library modules that can be compiled separately. Btw my intellisense is having a real hard time at parsing this thing XD

[xerocrypt]

Much of it wouldn't require the addition of files or modules, but the offloading of duplicate code to generic functions that can be instantiated wherever needed. That would improve the maintainability and testability of the code.

[edwardaskew]

I think step one for this is to get rid of all of the (mutable) globals - even if it means passing stuff around in a fairly inelegant way - and then start looking at what is used where and how things can be grouped.

I'm willing to give it a go unless someone else is already part way through something like this?

[Feynstein]

I talked about an elegant (I think) way in #201
You can see how I first implemented it in my fork

[spiderkat99]

I'll ignore the unit test discussion here as I have made my thoughts very clear elsewhere.

This is a valid exercise as the current code structure is unacceptable and it is untestable via any method.

If you want to create a plan of what to attack first have a look at coverage metrics that I uploaded in #166.

The worst offenders have the highest complexity metric and will give the biggest bang for the buck - so to speak.

The actual worst offender is:

ReadParams - Cyclomatic = 666, I really couldn't make this up - the number of the beast, it would be funny if it wasn't so serious ...

The Path Count also bust the tool as it maxed out at 999999999

RecordSample is also up there as you say above as it also bust the Path Count but the Cyclomatic metric is less, which isn't really a surprise, at 91 but that is still way too high.

[zebmason]

I've already refactored out a couple of obvious classes #213. Done a shoddy job but I would expect that code to be replaced by a third party GIS library. There is rather a lack of git submodules in this code.

[zebmason]

Just remembered I did think that the text output should be replaced by spdlog. After all it shouldn't mangle the output by interleaving threads.

[phelps-sg]

I've opened a related issue #243 suggesting adherence to a style guide. Of particular relevance are the following from the google cpp style guide:

• https://google.github.io/styleguide/cppguide.html#Write_Short_Functions
• https://google.github.io/styleguide/cppguide.html#Local_Variables

[robscovell-ts]

OO modularisation is one approach. Functional modularisation is another that could be considered for refactoring this model, i.e. decomposing into pure functions that can be verified and tested, and then re-composing those functions into the full model.

It is possible that #116 is caused by thread-safety issues when run on multiple cores. Functional modularisation could go a long way towards mitigating such issues.

[weshinsley]

Pruning with a couple of closing comments...

1. This is a broad topic; code cleanup and smaller functions etc, is a good thing, and is in progress, without needing a specific issue to say we should try and do it.
2. ReadParams is an uninteresting function to analyse. C/C++ code for reading parameters from ini-like files is never very pretty; this function allows detecting the parameters in any order, which is why the path count tool breaks due to the possibility-explosion. This doesn't indicate "serious concern" - more that the particular test tool is not the mode informative or appropriate one - nor is that the most interest part of the code, since it is called once at the start. Cleaning up ReadParams would be nice and we'll get there, but there are other priorities.
3. 116 was a bug introduced and fixed before release to do with parsing the seeds. It has nothing specific to do with thread-safety, multi-core, or functional modularisation/