Miscellanous issues arising during code review

[anjackson]

One of a set of questions that arose during an internal code review.

Some minor issues that arose in general:

• Code has very little logging
• Code has no tests
• Code has very few comments, making large code blocks difficult to understand. A bit more vertical spacing and some comments about what the major sections were doing would be good.

in the Java:

• Can we use server-relative URLs throughout? Rather that needing the replace-with-https code and other manipulations involving absolute URLs?
• There are quite a lot of hard-coded 'magic numbers/strings' that should really be declared in static properties. e.g. here
• Hard-coded per-facet logic means adding a new facet is relatively cumbersome.
• Almost no JavaDoc - it would be useful to have some on major public API components.
• Often uses a 'passing mutable state' approach when an 'immutable state management' style would be cleaner. e.g. here

in the templates:

• there seems to be a lot of repeated HTML fragments, e.g. per facet, that would be better done as sub-templates (e.g. this also makes adding a new facet cumbersome)
• there seems to be rather a lot of programmatic logic in the JSPs that might be better done in helper methods or ameliorated by more re-use of template code/sub-templates.

and in the JavaScript:

• Also quite a lot of hard-coded 'magic numbers'.
• Why are events being swallowed in the JavaScript? (various event propagation blocks)
• Some repeated jQuery calls should be replace for performance reasons (e.g. these repeated calls to get .collection-description)
• the code implements it's own accordion sidebar logic - is this neccessary when e.g. bootstrap components are available?
• Date range validation does not ensure the start date is before the end date.

[anjackson]

Assigning this to you @ldbiz to see if I missed anything important. See also #5 #6 #7 #8

[bzaar]

Hi Andy, thank you for highlighting those issues. I will split out some of them so they are easier to manage and track. E.g. I've just split out #12 for logging.

[anjackson]

No worries, and just to be clear, we're not expecting you to address all these things at this stage in the game. Or even agree with them all. They are just issues arising as we look at the code from a long-term support point of view.

I'll try to use the future-work tag to indicate this.

[bzaar]

Yes, I've noticed the future-work tag. Thanks.