Timeline for Extensible code to support different HR rules
Current License: CC BY-SA 3.0
28 events
| when toggle format | what | by | license | comment | |
|---|---|---|---|---|---|
| May 23, 2017 at 12:40 | history | edited | CommunityBot |
replaced http://stackoverflow.com/ with https://stackoverflow.com/
|
|
| Feb 17, 2017 at 9:09 | comment | added | Simsons | @t3chb0t, Don't we need to add ILeaveRequest to LeaveRequestProcessor ctor? | |
| Dec 22, 2016 at 9:22 | comment | added | RobH | @t3chb0t - I completely agree with what you're saying. I know that you've only suggested an alternative and aren't making any claim that it's perfect. You've also written a very good and comprehensive answer to the question. | |
| Dec 22, 2016 at 8:50 | comment | added | t3chb0t | @RobH He's also throwing exceptions but I too like his suggestions. As I've said earlier. You cannot come up with a perfect soultion of this size in just 2h and I don't claim it was perfect but I still think it's a good start for further iterations and improvements. Your arguments make sense but so do all the others that's why you usually spend days with engineering making the solution better and better each day as you recognize more and more patterns. I bet sooner or later someone will post another example with contracts and aspects and even more DI and IoC and all kind of stuff. | |
| Dec 22, 2016 at 8:33 | comment | added | RobH | @user1477388 Pawel's answer is almost exactly how I'd approach it. | |
| Dec 21, 2016 at 19:38 | comment | added | user1477388 | @RobH Do you have a better solution? Please share. t3chb0t how would you recommend the error handling? Just run a try/catch in the method that calls ProcessLeaveRequest? | |
| Dec 21, 2016 at 12:26 | comment | added | t3chb0t | @RobH Usually you design something like this for several hours, I spent writing it maybe 1.5h ;-) I'm pretty sure people will find more weaknesses. But I assure you, I'm not entirely happy with it too. | |
| Dec 21, 2016 at 12:23 | comment | added | t3chb0t |
@RobH I know it's not ideal and it's a start - that's why I also mentioned (somewhere in the middle) that the next step could/should be creating LeaveRequestRules that will validate a request in a more generic and extendible way where you can pass a collection of them to the request. A request contains just three properties, I think it's ok that it can validate itself. Actaully it could be called LeaveRequestValidator because a request is nothing more then just three or four values so the name isn't perfect too.
|
|
| Dec 21, 2016 at 12:08 | comment | added | RobH | Your proposed solution isn't ideal. Why should a leave request be able to validate itself? You're breaking the Open/Closed principle right there - you have to change the source to change the rules that apply to each type. The caller also has to know which type they're dealing with to know which exceptions might be thrown. I'd say that's pretty close to a LSP violation too. | |
| Dec 21, 2016 at 11:53 | comment | added | t3chb0t | @RubberDuck you're right, I've added an appendinx about it. | |
| Dec 21, 2016 at 11:53 | history | edited | RobH | CC BY-SA 3.0 |
Fix typo in Liskov
|
| Dec 21, 2016 at 11:40 | history | edited | t3chb0t | CC BY-SA 3.0 |
added 677 characters in body
|
| Dec 21, 2016 at 10:59 | comment | added | RubberDuck |
Let's agree to disagree @t3chb0t. I see a need for a ValidationResult, you don't. That's fine.
|
|
| Dec 21, 2016 at 10:54 | comment | added | t3chb0t | @RubberDuck the expected flow is that an employee requests his leave with correct data, if he didn't it's an exception. It's not like everyone is expected to put invalid data in their requests only to find out what is correct by receiving exceptions. It's not how it is supposed to work. You want to validate their requests but don't expect the requests to be invalid all the time, they shoudn't. | |
| Dec 21, 2016 at 10:00 | comment | added | RubberDuck | @t3chb0t exceptions are for exceptional behavior. You're talking about using them for expected control flow. | |
| Dec 21, 2016 at 7:30 | comment | added | t3chb0t |
@germi I don't see any reason why it should not be allowed. It depends on the design of the database. If the Employee model has all the properties you need to validate the leave-request then it's fine with me. If you however need a database trip to get more information anyway then you may as well just use the employee-id. The parameters should be as simple as possible. This makes testing much easier because you don't have to create complex object for them. For example if the Employee model had 30 properties and you use only three for the request then a simpler interface should be used.
|
|
| Dec 21, 2016 at 7:20 | comment | added | germi |
I have a question regarding the use of employeeId vs. the actual instance of Employee. I would have passed the employee as an argument to the constructor so inside the request I could directly query it regarding any information I might need. By just handing over the ID I might have to make another trip to the database to get an employee that - most likely - was already retrieved before the request was created. Is there any specific argument for passing the ID / against passing the employee?
|
|
| Dec 21, 2016 at 4:50 | comment | added | t3chb0t | @RubberDuck but what alternatives do we have? You could create rules for each case and to differentiate between them you'd need to work either with error codes (nasty) or you'd need to check the type of the rule somewhere. Not so pretty either. Those states are rather exceptional and esier to handle with exeptions. | |
| Dec 21, 2016 at 4:41 | comment | added | t3chb0t |
@RubberDuck ;-) of course not but this is not the part of the application that should care about it - it should be seen as library code. IMO the user of the LeaveRequestProcessor should catch them and handle them accordingly by either showing some messages (not necessarily the exception message) and/or logging them.
|
|
| Dec 21, 2016 at 1:53 | comment | added | RubberDuck |
This is a very good answer, but I've got to question creating exceptions like OutOfVacationException. Let's pretend that we neglected to catch it, should the entire application crash because an employee is out of vacation time??
|
|
| S Dec 21, 2016 at 0:20 | history | suggested | Cave Johnson | CC BY-SA 3.0 |
Formatted one of the lists and minor spelling corrections
|
| Dec 20, 2016 at 23:50 | review | Suggested edits | |||
| S Dec 21, 2016 at 0:20 | |||||
| Dec 20, 2016 at 20:35 | history | edited | t3chb0t | CC BY-SA 3.0 |
added 30 characters in body
|
| Dec 20, 2016 at 20:27 | vote | accept | Simsons | ||
| Dec 20, 2016 at 17:47 | history | edited | t3chb0t | CC BY-SA 3.0 |
added 86 characters in body
|
| Dec 20, 2016 at 17:15 | history | edited | t3chb0t | CC BY-SA 3.0 |
added 338 characters in body
|
| Dec 20, 2016 at 16:53 | history | edited | t3chb0t | CC BY-SA 3.0 |
added 133 characters in body
|
| Dec 20, 2016 at 16:40 | history | answered | t3chb0t | CC BY-SA 3.0 |