search

icatproject / icat.utils

Setup scripts and a few useful bits of Java code.
Other
0 stars 2 forks source link

19 unit conversion #20

Open patrick-austin opened 2 years ago

patrick-austin commented 2 years ago

Adds support for unit conversion, to be used in icat.server and icat.lucene as part of search improvements.

patrick-austin commented 5 months ago

You have a constructor that modifies a static variable and therefore changes the behaviour of all other instances. Either have a separate non-static instance of SimpleUnitFormat for each instance of IcatUnits, or only have static methods and no instances.

I'm not totally confident on how singletons work, but since we're calling SimpleUnitFormat.getInstance() won't this be shared across all instances of IcatUnits even if we didn't declare it as static? Or would we also want to change this to SimpleUnitFormat.getNewInstance()? I suppose with e.g. the EntityInfoHandler change we want to remove uses of singletons in our codebase? Happy to go with whatever's most consistent with our design patterns elsewhere - just not 100% sure what they are...

In terms of making everything static, since we're passing configuration settings in icat.server/icat.lucene I don't see a way the "converter" could be made properly static other than maybe doing SimpleUnitFormat.getNewInstance() and passing the alias settings everytime we do a conversion, but that would be needless repitition.

It's not clear from the name of either class that the purpose of this is to convert quantities to SI units, so provide a method with a name such as convertToSiUnits(...).

Yeah looking at it I think I was trying to persist the terminology of the underlying library (i.e. SystemUnit) but probably better to be explicit that "System" is short for Système international and that the main class is for conversion.

ajkyffin commented 5 months ago

As you say, since this needs runtime configuration of aliases, it would be best to have it be non-static and use SimpleUnitFormat.getNewInstance().