search

JujaLabs / itevents

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

#125 add period to filter #163

Closed romach closed 3 years ago

romach commented 8 years ago

connect to #125

vaa25 commented 8 years ago

@romach resolve conflicts please if they are

vaa25 commented 8 years ago

@romach

  1. FilterConverter: why getDefaultItemsPerPage is separate public method? Is it dead code?
  2. FilterWrapperBuilder: it used only in tests, but placed in the production code. Is this correct?
  3. DateTimeUtil: static methods still there, and no new issue about it. discussion
  4. FilterConverterTest.java: placed in package wrapper instead of converter; contains unused import
  5. EventMyBatisDaoDbTest uses static methods of DateTimeUtil, same as 3.
  6. EventDateReplacementDataSetLoader: you changed format, but didn't change comment described it. Can you explain why did you change format? @AndriyBaibak 30 min
romach commented 8 years ago

@vaa25

1) FilterConverter: why getDefaultItemsPerPage is separate public method? Is it dead code?

Yes, but this code is yours, should I fix it?

2) FilterWrapperBuilder: it used only in tests, but placed in the production code. Is this correct?

Will be fixed.

3) DateTimeUtil: static methods still there, and no new issue about it.

  • new issue will be created.
  • //todo will be added

4) FilterConverterTest.java: placed in package wrapper instead of converter; contains unused import

Will be fixed.

5) EventMyBatisDaoDbTest uses static methods of DateTimeUtil, same as 3.

  • new issue will be created.
  • //todo will be added

6) EventDateReplacementDataSetLoader: you changed format, but didn't change comment described it. Can you explain why did you change format?

Comment will be fixed. I have changed the format because in this test I receive current time from database and set time to expected event in this method. So these times must have the same precision (milliseconds).

vaa25 commented 8 years ago

@romach 1) yes, please) Its very good if somebody thinks that his halfyear code is bullshit)

romach commented 8 years ago

Fixed according to above comments. @vaa25, @alex-anakin review me, please.

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

@romach

  1. Separate converters.xml context is redundant.
  2. It isn't necessary to run application context in FilterConverterTest if you use mocks. Because running it is a very slow process. But one of the main advantages of the unit test is performance. You can use MockitoJUnitRunner instead SpringJUnit4ClassRunner.
  3. I recommend to use self-explanatory methods names instead comments (in class EventDateReplacementDataSetLoader).
alex-anakin commented 8 years ago

@AndriyBaibak time tracking: 20 minutes spent

romach commented 8 years ago

@alex-anakin

Separate converters.xml context is redundant.

converters.xml deleted

It isn't necessary to run application context in FilterConverterTest if you use mocks. Because running it is a very slow process. But one of the main advantages of the unit test is performance. You can use MockitoJUnitRunner instead SpringJUnit4ClassRunner.

MockitoJUnitRunner is used

I recommend to use self-explanatory methods names instead comments (in class EventDateReplacementDataSetLoader).

EventDateReplacementDataSetLoader refactored