Code Review - Feb 2020

[andrewphilipsmith]

As you can see I have added comments to many of the files. At present these comments are a stream of consciousness. I have not attempted to prioritise this comments or even define if any action is required.

Some will certainly require comment/reply from @shurst-ma before making changes. Others are probably actionable by anyone.

I have deliberately used the word "TODO" in my comments, knowing that CodeClimate will highlight this.

There are some files I've not reviewed yet - map_chef.py being the main one I've not looked at. I will add more comments/detail in the coming days and attempt to make the comments I have made more actionable.

[andrewphilipsmith]

I finished going through the mapactionpy_arcmap module. (I still intend to go through the mapactionpy_controller and cmf_watcher modules, though I suspect that it would be more value to have someone else review these two modules).

I am still attempting to summarise my thinking on this and work out how this related to the task that need to be completed in order to complete the project. More on this in the next few days.

Refactoring Tasks

There are a number of helpful refactoring tasks that can be tackled. In some cases I've added details within the code. This list is not exhaustive, merely an unscientific list of highlights that I noted whilst reviewing the code:

• Move various files, in their entirety from mapactionpy_arcmap to mapactionpy_controller. These are noted in comments in the relevant files.

• Consolidate the two MapRecipe implementations (mapactionpy_arcmap.map_recipe.MapRecipe and mapactionpy_controller.product_bundle_definition.MapRecipe)

• Consolidate the LayerSpec class (mapactionpy_controller.product_bundle_definition.LayerSpec) with the layers dict in mapactionpy_arcmap.map_recipe.MapRecipe.

• Mark internal methods appropriately

• Unit tests!

• Find a consistent way to resolve the mix of method parameters and object members. See MapChef.updateTextElements() and MapChef.cook() for examples.

Design questions

There are some design questions - by which I mean the design of the object-model shared between the modules - which needs discussed and agreed. Again this is not a scientific list of questions, but some I noted whilst reviewing the code.

• What is the separation of responsibilities between MapChef and ArcMapRunner? Why is the boundary between the two classes where it is?

• Is it indented that the cook() method might be called multiple times in the life of a MapChef object or just once? At present it looks to me like cook() can only be called once.

  • If cook() only gets called once in the life of a MapChef object, then it should be entire procedural, with no parameters (everything set via the constructor) and subsequent attempt to callcook()should result in an exception. In fact it might be appropriate forcook()` to be called from within the constructor.
  • If cook() can be called multiple times, then there need to be consistency in which variables are passed as method parameters and which are object members.

• What is the source of truth for map version numbers?

• The Event class:

  • This should probably be renamed as "event" is already a heavily overloaded word in programming - "HumanitarianEvent" might be better.
  • There should be some internal validation validation for self-consistency for the members of this object. eg:
    • iso3_country should match country_name
    • The glidenumber(s) should match the regex for glidenumbers
    • The iso3 country code within the glidenumber should match the iso3_country member.
    • etc Provision should be made for fictional countries/events too.

• Commandline interface(s); There are several commandline interfaces scattered throughout the modules. These could/should be consolidated. They should also be callable from the top level of the module (eg python -m mapactionpy_controller <options>, not python -m mapactionpy_controller.check_naming_convention <options>).

• Common elements for MapRecipe class and MapReport class

  • The MapRecipe and the MapReport are both special cases of the state of the map. A present there seems to be a lot of copying information from one to the other and/or there is a requirement for the user of a MapReport files to manually cross-check the report with the recipe.
  • Could/should these be implemented as subclasses of a common State class? Or one as the subclass of the other?
  • Or implemented with the ability to round-robin between the in-memory object and the json representation - then they could just differentiated by json schemas?
  • How should the intermediatory state handled? Is there a class that represents this?

• MapRecipe; There is the potential to make changes to the structure of the mapCookBook.json (and by implication the related class) to include information about the:

  • layers with special meaning; without needing to hardcode information about particular layers.
  • map frames; without need to hardcode assumptions about how many mapframes there are or information about which map frame marginalia elements relate to (eg scalebar, legend etc)

• MapReport; I would like to check what it the ideal location for storing the files and what is an appropriate naming convention for them? It is not obvious to me that storing them with mxd is the ideal solution.

[codeclimate[bot]]

Code Climate has analyzed commit cf87a6bf and detected 74 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	3
Bug Risk	71

View more on Code Climate.