FrancisG-Massey / Capstone2016

4 stars 0 forks source link

Use static method. #288

Closed geekdenz closed 7 years ago

geekdenz commented 7 years ago

Sorry, no other means of contacting you atm, so I thought I'd log a trivial issue:

See https://github.com/FrancisG-Massey/Capstone2016/blob/master/ProjectNest-API/src/java/org/nestnz/api/Common.java#L107 .

Matcher m = Pattern.compile(Common.DATASETPARAM_REGEX).matcher(dirtySQL.toLowerCase());

could be put in a static code block to improve performance like so:

private static Matcher datasetMatcher = null; static { m = Pattern.compile(Common.DATASETPARAM_REGEX).matcher(dirtySQL.toLowerCase()); }

All members of the team. Please keep in touch: heuert@landcareresearch.co.nz

Looking forward to hearing from you.

sam-hunt commented 7 years ago

Ah thanks :) Seems simple enough for the meantime. However I believe this could probably be optimized out of the main request pipeline completely after implementing #138 (and ideally #177 first).

At the moment, as a byproduct of keeping the SQL queries abstracted in JSON files (datasets), the queries (and by extension database tables) used by the API can currently be changed on the fly without recompiling/restarting.

As above and noted in the linked issue descriptions, I planned to support caching these datasets in memory on initialization of the servlet container. This would allow the regex you noted was compiled here, to be compiled and used just once on startup (to pre-parse the queries), instead of on a per-request basis as it is at the moment.

This would also remove from the request handling pipeline: the two filesystem IO reads, (one for the URL-to-Dataset Lookup, and one more for the Dataset Read), the JSON object parsing of the datasets, and the regex parsing for dataset query SQL. While the refactor would be quite significant, these changes would speed things up significantly.

sam-hunt commented 7 years ago

Okay I've looked into this now thanks :)

private static Matcher datasetMatcher = null; static { m = Pattern.compile(Common.DATASETPARAM_REGEX).matcher(dirtySQL.toLowerCase()); }

Seems can be done without a block as it's a single expression:

private static Matcher datasetMatcher = Pattern.compile(Common.DATASETPARAM_REGEX).matcher(dirtySQL.toLowerCase());

Except that dirtySQL is a local instance variable, so only the Pattern can be compiled statically, and the Matcher must be generated each time. It also needs to remain public unfortunately as I think some tests reference this in order to reduce code duplication and inconsistency between the tests and code.

Thanks again, I hadn't heard for static blocks before, but seems like something that could benefit other parts of the codebase too. Cheers :)