Nicolas Malin <[email protected]> writes:
> On 8/10/19 12:07 AM, Mathieu Lirzin wrote:
>
>> Here are a few inline comments regarding the code.
>>
>> Maybe I overlooked some good reason justifying some design decision you
>> made, so if you want to discuss more about the suggestions I proposed,
>> we can do some pair programming next week.
>
> It was a big task, so I focused my mind only on how homogenize and
> centralize a number displaying. So if you I have some idea to continue
> the improvement through other design concept, It's welcome :)
This is indeed a big effort which is adding a great amount of
flexibility.
>>> */
>>> - public static String formatPrice(Double price) {
>>> - if (price == null) {
>>> + public static String formatNumber(Double number, String formatType,
>>> Delegator delegator, Locale locale) {
>> The dependency on the Delegator should be avoided if possible because it
>> makes writing unit complex tests. By the way where are those tests ? :-)
> Normally not because delegator and locale are optional. If it's not
> the case, I will improve it to support unit test
To me this is the only critical part that needs to be fixed because
previous implementation was achieving better testability. Other remarks
are less important improvements that can be worked on later.
>> To remove a dependency on a class which is hard to instantiate like
>> ‘Delegator’, the common solution is to replace it with an interface
>> (ideally a functional interface to let the caller pass a lambda).
>>
>> In this particular case after a quick look, the ‘delegator’ argument
>> could potentially be replaced by a ‘templateProvider’ argument of type
>> ‘Function<String, String>’ where the input is a format type and the
>> output is a template.
>>
>> When things are complicated to decouple, an alternative is write an
>> overload specifically for the test and use it inside the coupled one.
> I's seem to be a good idea :) but I'm far away to understand what do
> you explain, I need to studies more ^^
Decoupling methods from stateful objects like ‘Delegator’,
‘HttpServletrequest’ and ‘GenericValue’ is hard to explain and
understand theorically, however in practice when adopting the hygiene of
writing unit tests with plain java objects it become second nature. :-)
Thanks.
--
Mathieu Lirzin
GPG: F2A3 8D7E EB2B 6640 5761 070D 0ADE E100 9460 4D37
