trivago / jade

A simple but yet powerful library to expose your entities through JSON API.
Other
55 stars 6 forks source link

[CHORE] Project clean up #13

Closed regniblod closed 6 years ago

regniblod commented 6 years ago

I decided to start contributing in this project since I need to implement some new features for an internal trivago project that uses this project.

To start contributing I (and everyone else who wants to contribute as well) need a clean starting point, so I decided to do a bit of clean up of the project.

This is what I changed:

regniblod commented 6 years ago

Hey @moein, I fixed all your comments except for the Exception ones.

I know what you mean with "they don't provide useful information" but maybe that means we have to give them meaningful information?

I was thinking about converting all the \InvalidArgumentException and such into meaningful exceptions from the Domain.

Otherwise what's the value on throwing an exception that I'm not even supposed to see in the DocBlock from the project using JADE? I'm supposed to just wrap every single call to a JADE method into a try { ... } catch (\Exception $e) {} just in case?

moein commented 6 years ago

@regniblod Exceptions like \InvalidArgumentException or \LogicException are not meant to be captured at all. These are exceptions that developers should see in the dev testing. For example, if you have a method that receives 2 numbers: father's age and son's age. If any of these ages is not a positive integer it's an InvalidArgumentException. In this case, the person who is calling the method has to take care of this constraint. Also, the father's age has to be greater than son's age so if that's not the case it's a LogicException since each argument by itself is valid but not the conjunction. And finally if you have a constraint from the business side that the father has to be older than 18 then that's when you use domain exception and that's the only exception the developer who is calling the method is supposed to catch as it might change in the future. I hope it's clear enough

regniblod commented 6 years ago

Thanks for the explanation @moein!

I did 2 more commits, I hope everything looks better now.