JujaLabs / itevents

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

#168: Exceptions refactored #219

Open IgorMaksymov opened 8 years ago

IgorMaksymov commented 8 years ago

connect to #168

vaa25 commented 8 years ago

@IgorMaksymov

  1. Throwing of RuntimeException is bad practice. You should throw custom unchecked Exception.
  2. Does UserMyBatisDao.addUser can throw any other Excepion that out of range SqlException?
  3. What sense of throwing identical MailBuilderUtilException in every private method, if we can generate it only in public methods?
  4. shouldExpect500IfNotificationServiceException is difficult to understand without context of #53 and #187. Are you sure it must be here than there?
  5. Don't use any() in tests
  6. Why did you refactoring DbUnitTableExportUtil? Is it in scope of issue? It even not included part of project.

@AndriyBaibak 1hour

IgorMaksymov commented 8 years ago
  1. our basic ServiceException is good? Some methods will throw only one exact RuntimeException and i dont think that this is good idea, to create another RuntimeException inheritor, that will be used only in one method.
  2. dont know, but if it throws, it will be catched and not handled.
  3. fixed.
  4. yes, or this tasks will be blockers for this. This is like task creation without actual task creation. Also i need to handle my NotificationServiceException
  5. why? Its easy to use. No need for static when().thenReturn() etc...
  6. What do you mean by " It even not included part of project" ? it has unused methods and Exception antipatterns, so i decided to refactor this.
vaa25 commented 8 years ago
  1. Maybe ServiceException is good.
  2. The same we can say about all other methods, but, as you can see, we don't do it for them.
  3. Ok.
  4. ...
  5. Rewatch one of meeting where teamlead told about this topic
  6. Explained in pm
IgorMaksymov commented 8 years ago

@vaa25 2 If we catch RuntimeException or something else in this block, it will be catched but not handled. That's why we search exact root exception and handle it.

IgorMaksymov commented 8 years ago

@vaa25 its outdated, update branch

vaa25 commented 8 years ago

@IgorMaksymov Why do you think that any SQLException equals EntityAlreadyExistsDaoException ? Why do you made so monstruous construction instead catching one simple DuplicateKeyException? @AndriyBaibak 10 min

IgorMaksymov commented 8 years ago

@vaa25 fixed

vaa25 commented 8 years ago

@AndriyBaibak 5 min

romach commented 8 years ago

@AndriyBaibak 2 hours spent

IgorMaksymov commented 8 years ago

@romach @vaa25 review pls

IgorMaksymov commented 8 years ago

@romach fixed