JujaLabs / itevents

Resource to subscribe on it events
Apache License 2.0
7 stars 5 forks source link

#172 date time util refactor #195

Closed romach closed 3 years ago

romach commented 8 years ago

connect to #172

romach commented 8 years ago

@vaa25 , @alex-anakin review me, please

vaa25 commented 8 years ago

@romach There are some new Date() in OneTimePassword, MyBatisEventService, MyBatisUserService @AndriyBaibak 30 min

romach commented 8 years ago

@vaa25 done

vaa25 commented 8 years ago
alex-anakin commented 8 years ago

@romach

  1. Result of CustomDateTime.getDate() is very similar to RealClock.getNowLocalDateTime(). Can you explain the difference or neccessary of CustomDateTime.getDate() existing?
  2. Method CustomDateTime.getLocalDateTime() is never used.
  3. You should use interface that class CustomDateTime will implement.
  4. Class RealClock can be final.
  5. Comment over RealClock.getNowLocalDateTime() is redundant because method purpose is obvious.
  6. Class CustomDateTime is over engineered or badly designed. For example, how used one of it' methods: Date yesterdayDate = new CustomDateTime() .withLocalDateTime(clock.getNowLocalDateTime().minusDays(1)).getDate(); Inside withLocalDateTime method there is another new CustomDateTime(...). Why so complicated?
alex-anakin commented 8 years ago

@AndriyBaibak time tracking: 30 minutes spent

romach commented 8 years ago

@alex-anakin

  1. now CustomDateTime is used in RealClock and FrozenClock to reduce code duplication;
  2. fixed in 1.;
  3. DateTime interface added;
  4. RealClock and FrozenClock classes was made final;
  5. Comment over RealClock.getNowLocalDateTime is deleted;
  6. I decided to make CustomDateTime class immutable. So, I can't use setters to change object's state;
alex-anakin commented 8 years ago

@romach Concerning class CustomDateTime. CustomDateTime breaks the single responsibility principle. It is responsible for getting date values and for creating new instances of the class. Perhaps you should create another class and implement the factory method pattern. Or try find your own way.

alex-anakin commented 8 years ago

@AndriyBaibak time tracking: 20 minutes spent

alex-anakin commented 8 years ago

testing new labels

IgorMaksymov commented 8 years ago

@romach after merging master to this branch, please check TODO in MyBatisUserService

romach commented 8 years ago

@alex-anakin DateTimeFactory added, review please