Constants can go in the Constants.java file

@cammace I don't like the approach of having this kind of all-constant classes. Constants should be in their pertinent classes, basically because it's where they belong and provide more context. Actually, using an all-constant class approach is a well known antipattern.

@Guardiola31337 why replacing the constant with its value?

@zugaldia it's a technique what I learnt from the book Working Effectively With Unit Tests. I really like the techniques described on it in favor of DAMP (Descriptive And Maintainable Procedures). Mainly because they increase maintainability and readability of tests.
In this concrete case, why I prefer expecting literals?

• The main advantage of expecting literals is that you only need to focus on the actual value of the assertion, increasing readability and traceability.
  If neither your expected nor actual values are literals you’re forced to determine the value of both when a test fails. Conversely, if your expected value is a literal, only the actual value will need to be traced when a test is failing.
• Literals will reflect the examples from the business.

@Guardiola31337 Thanks for the explanation. While agree with these general goals, I disagree in this particular case. This test really intends to test response.body().getCode().equals(DirectionsCriteria.RESPONSE_OK) which is what the developer would type in their code. The fact that DirectionsCriteria.RESPONSE_OK has the value Ok or OK or something else is secondary in this case. In fact, if the server happened to change its response from Ok to OK and we simply updated the literal here, the test would now pass, but we'd miss updating the value of DirectionsCriteria.RESPONSE_OK.

@zugaldia Ok agreed. It's also explanatory using the constant so I leave it as it was 👍

@Guardiola31337 same comnent as above.

@Guardiola31337 why public?

You're right 😅 I missed that.

On a second thought, I think we should fix this in a simpler way.

• setOrigin(Position origin) should set a private Position origin value.
• setDestination(Position destination) should set a private Position destination value.
• setCoordinates(List<Position> coordinates) should set a private List<Position> coordinates value.

Then, at build time we check if both (origin, destination) and coordinates have been set. If the developer has mixed them we through a ServicesException.

@zugaldia Since I don't know in depth the SDK, does it mean that coordinates would be a list of coordinates in between the origin and the destination? At the moment, origin is the first element of the list and destination is the last one. When you try to add a new origin, if you already had more than two coordinates, the new origin is prepended to the coordinates list. Similarly, when you try to add a new destination, if you already had more than two coordinates, the new destination is appended. But, if you called setCoordinates the whole coordinates is reset. Is that the behavior that we are looking for?
cc/ @cammace