Sandro Mancuso & Uncle Bob comparative design episode 2 notes

Notes from episode 1
Notes from episode 3/4/5
Notes from episode 6

After notes from episode 1, I promised some notes after episode 2 and here they are.

As with the previous post (please read the disclaimer that's there if you didn't), I am focusing on the stuff I disagree with, would do in another way or consider controversial. This is because that's what I learn from the most, not because I want to prove to anyone that I am smarter or something. There was also lots of great stuff that I just happen to agree with, so I won't write a lot about that. Also, remember that the episodes show code in evolution, so it may be that some choices Sandro makes are temporary or made with knowledge that no further development will be done in specific code areas.

General Remarks

Stuff I forgot to write down from the first episode and is also in the second.

1. I noticed that Sandro mixes Mockito with BddMockito, which seems kind of strange to me. Typically what I see is that people prefer one namespace or the other, but Sandro takes stubbing syntax from BddMockito (given(...).willReturn(...)) and verification syntax from plain Mockito (verify(...)). I wonder what he does when he works with his team - whether they all use this convention or someone gives up and agrees to use something different.
2. Sandro doesn't use a TODO list anywhere in the example. I think one of the reasons is that his UnsupportedException occurences work as TODO marks. The other may be that this is a prepared exercise so there's little exploration and question marks to be investigated. When doing prepared presentations, I also very rarely use a TODO list, so I don't see any issue with that.
3. He uses TDD together with programming by intention. This is something I've never done before, I will try to practice it a bit in the coming weeks.

Now for the main notes.

 Users API

1. The beginning of the episode is about finishing the user API, where a UserService instance should create a user. Sandro moves the randomness of generating user ids (UUIDs) to a separate class, called IdGenerator and says he will not test it as it is not deterministic. While I agree that it isn't deterministic, I think some of the properties of the ID generator may be. Personally, I think it still is a material for a property-based test. Though there's (almost non-existent) chance of generating a UUID that was once used, I think it is reasonable to expect from the algorithm that no two UUIDs generated in a sequence are the same. Sure, that would lead to a discussion on "why two, not three" etc., but I would still write a small test around that property, especially that the return value of the generator is String, not UUID, so there's nothing in the signature that forces one to use UUIDs in the implementation.
2. The next test for the UserService is about throwing an exception when a user that we're trying to create is already in the repository. Sandro uses JUnit's expected exception annotation: @Test(expected = UserNameAlreadyInUseException.class). Not sure why he prefers it to AssertJ's built-in assertThatThrownBy(). While this is a recent feature, I think it was already around when the video was recorded. Typically when I read people writing on asserting exceptions, the annotation-oriented solutions are considered the inferior ones to specialized assertions.
3. In the same test, Sandro wonders whether to return an optional or to create a query method named "isUsernameTaken()". Only if that method returns true does he go on and create the user. I think I can see why he doesn't use an optional - because that would not scale - he would need to establish an implicit assumption that every Optional.empty() returned from repository means that user name is already taken, which could be a bit of stretch. Alternatively, he could throw the exception straight from the repository, but that might mean pushing some of the application logic into what can essentially end as adapter (but currently Sandro puts his repository together with domain logic). On the other hand, I'm wondering whether his solution would hold in a more real-life environment of race conditions and multiple threads accessing the repository. Sandro seems to assume no such requirement for this system since if there was one, the access to the repository would need to be synchronized. Then, he would probably need to solve the dilemma of where to place the synchronization construct. I wonder what would be his answer.
4. Another thing I am wondering about is whether Sandro considers the repository he created a domain construct or an adapter in the hexagonal architecture sense. He puts it into domain logic for now, but actually how he thinks of it can influence what kind of logic he may be inclined to push inside and what kind of logic he would like to keep away from it. Especially that Sandro says this is going to be replaced by a database at some point.
5. An interesting fact is that he defers the technology choice for the persistence mechanism. Not sure if he will implement it or not. This is the place where in normal project I would like to see something like a story map to be sure where I am, since the external API description in Swagger does not say anything about persistence, so as a behavioral specification, it is incomplete. If this is the only specification we have, then what should we derive our need for persistence from?
6. When testing the isUserTaken() behavior in the repository, Sandro writes two tests in a single method - one for true (name is taken), one for false (name is not taken). For me these are two behaviors and I would like to write them separately. When I look at it, the behavior looks to me like a good candidate for a parameterized test, although I must say that JUnit 4 does not make writing parameterized test methods easy. At the very least, if I really felt this is closer to a single behavior (just that each assertion shows one side of the coin), I would employ the soft assertions feature of AssertJ to not stop on the first assertion failure.
7. There is a nice discussion between Sandro and Bob on why there isn't a getUser() method in the repository that would allow checking whether the user is in the repository or not. I agree with Sandro that adding such method would be premature as no existing use case needs it yet and I think something like this would constrain my internal implementation. 

Login API

1. I noticed that Sandro doesn't use named constants in tests. Even though the production code uses the library-defined OK_200 constant (as funny as this name might be), the test uses plain 200. Not sure what the rationale is as it is never given, so I am going to try and guess that Sandro does not trust the constants and wants the tests independent of them because if tests used the named constants as the production code does, the values of the constants would essentially fall out of scope of a test, so if the value of OK_200 changed at one point, there would be no test to detect that. Personally, I prefer a different way of including constants in the test scope. I use the named constants everywhere in the tests and write an additional small test that says assertThat(OK_200).isEqualTo(200) writing such test takes me literally half a minute and the dilemma is solved for me. Now if the value of the constant changes, I have a single test to maintain. 
2. Just as Sandro created UserApi class, he creates LoginApi. I think not putting both registration and login methods in the same class is a clever move because by doing that, Sandro establishes vetrtical architecture slicing and has a green field for his new feature in a fresh vertical slice.
3. Interestingly, Sandro goes straight to the repository from the LoginAPI, not introducing any kind of LoginService as he did in case of the UserAPI -> UserService->UserRepository. The rationale is that the login API is so simple that in this case Sandro is willing to do an exception and move away from what he considers a redundant abstraction layer. I think that doing that is a nice demonstration of benefits of vertical slicing, since he can make this decision for login API without affecting the user API. This is one of the reasons why I like vertical slicing so much. If he did put both registration and login methods in the same API class, it would look confusing that for one feature, the API goes to the service which goes to the repository and for another the same API class goes straight to the repository. That being said, I don't understand in what way is the login API simpler than the user API - both are plain "DTO for DTO" cases with "else return error" conditions so I see no significant factor that would lead me to taking a different approach in each case.
4. I have some (temporary?) concerns with the UserCredentials class - it seems to be a a DTO and a value object in one. While I don't have much against DTOs implementing comparison operations for convenience, the class later seems to get more responsibilities that make it even more similar to a value object (i.e. a method boolean matches(User user)). Maybe this is a beginning of evolution that I will observe further in the next episodes?
5. In my notes form the last episode, I noticed that Sandro established a relationship between test values where such relationship is not required. He did the exact contrary in the login API test. Here, he created input UserCredentials and did not make data relationship between it and the User object set to be returned by a mock. He only established such relationship between User and its JSON representation, using the User instance as a reference. This is exactly how I like to handle such cases. This way it is clearly shown that it's not the login API's business to say what kind of values will the User object hold - this responsibility is pushed onto the UserRepository class.
6. Sandro has two identical methods called jsonContaining() but chooses not to remove the duplication. He gives a nice explanation that he is still not sure whether the two occurences of this method will change for the same or different reasons. Also, I can only guess that he is more careful with refactorings that come across vertical slices, but this is only my assumption. I would maybe leave a TODO here if I believed this conflict can be resolved in a foreseeable future. If I believed otherwise, I would probably eliminate this duplication because it would probably be easier for me to later copy-paste than find all the ocurrences of a piece of code.
7. In the controller test, Sandro has the following login() method:
   
   String login(Request request, Response response) {
     UserCredentials credentials = credentialsFrom(request);
     Optional<User> user = userRepository.userFor(credentials);
     if(user.isPresent()) {
       response.status(OK_200);
       response.type("application/json");
       return jsonFor(user.get());
     }
     response.status(NOT_FOUND_404);
     return "Invalid credentials";
   }
   One thing that bothers be is that in the test for the case where a user is NOT present, there is no assertion on the content type of the response, which means that if I move the line response.type("application/json") above the if statement, all the tests are going to compile and pass but the response is going to be different. I assume that from Sandro's perspective, this doesn't matter as he has only one client application and that application doesn't care (or maybe acceptance tests check that?). Typically in what I do for a living, I am told to be strict about what I offer through my API and generous about what I accept. This is because where I work, the API is the product and I don't know all the client applications up-front.
8. In the same method, a choice between response.status(OK_200) and response.status(NOT_FOUND_400) is what I call a situation of exclusive choice. In other words, this is not like an "add" method that can be called multiple times, but only one call to the status() method makes sense and it must be exclusively chosen. In such situation, I typically use Mockito's VerifyNoMoreInteractions() to make sure only one choice is made. In this case, however, there are several calls to the response object so VerifyNoMoreInteractions() would be too coarse-grained because it works for the whole mock, not for a single method. The code can be refactored (I will show how in a second) to make the use of VerifyNoMoreInteractions easier or something like this could work:
   
   verify(response).status(OK_200);
   verify(response, never()).status(intThat(status -> !status.equals(OK_200)));
   This could be also refactored to something like:
   
   verifyStatusSetExclusivelyTo(OK_200, response);
   
   The whole rationale for me doing it is that presence of two different calls to response.status() in one method move a potential error of calling them both at some point to the Inherently possible (can be made impossible) group from the inherently impossible (can be made possible) group.
9. Sandro refactors the login() method to use two more methods and ternary operator (condition ? a : b) to transform the code into something like this:
   
   String login(Request request, Response response) {
     UserCredentials credentials = credentialsFrom(request);
     Optional<User> user = userRepository.userFor(credentials);
     return user.isPresent()
     ? prepareOkResponse(response, user)
     : prepareErrorResponse(response);}
   
   Bob and Sandro briefly discuss this choice, Sandro commenting that he likes ternary operator and that the real problem with ternary operators is when you chain them. Personally, though I have nothing against ternary operator as such I see one more issue with the use of ternary operator in this case and the refactoring that was done here. To demonstrate it on a simpler example, I find the following use of ternary operator confusing as well:
   
   int a = getA();
   int b = getB();
   int c = getC();
   int y = (a = b) == 3 ? c = 1 : 4;The part that confuses me in the code above (besides the cryptic variable names :-)) is that there are side-effects in ternary operator - two assignment operators which, in C-like languages, happen to return value as well. A ternary operator turns "if" from statement to an expression and typically when I see someone make expression out of a statement like this, it is to stress that the whole code in scope of the operator is just a composition of pure expressions or pure functions. This is where I think ternary operator is even clearer in conveying intention than the if statement. But this is not what happens in the login() method as both prepareOkResponse() and prepareErrorResponse() are impure functions with side effects, similarly to the assignment operator I presented above. For example, the prepareErrorResponse() is defined as:
   
   private String prepareErrorResponse(Response response) {
     response.status(NOT_FOUND_404); //side effect
     return "Invalid Credentials";
   }
   When looking at the login() method and the ternary operator, I wouldn't expect to find side effect in a function like this, so this is not my favorite solution. What would I do instead? Probably either I would not do the refactoring or (in more complex use case) I would refactor further to something like custom result observer or collecting parameter, which would on the other hand cost me at least one or two new classes (and would probably require moving the login() method logic to a service class from the controller). The code could then look like this (note that the Response type is changed to my own custom class):
   
   void login(Request request, MyOwnResponse response) {
     UserCredentials credentials = credentialsFrom(request);
     Optional<User> user = userRepository.userFor(credentials);
     if(user.isPresent()) {
       response.foundCredentialsMatching(user.get());
     } else {
       response.InvalidCredentials();
       }
   }
   
   This would have the added benefit that I could now use VerifyNoMoreInteractions on the response object as now there is only one exclusive call to this object in each case and the situation of exclusive choice becomes clearer.
   
   Note that in my example, the login() method is void. This would be a result of pushing the responsibility of holding the return value to the MyOwnResponse class. Someone that calls the login() method (in my case - a controller) could then do something like this:
   
   loginService.login(request, response);
   return response.text();
10. Sandro claims his acceptance tests only test the sunny day scenarios because of test pyramid, so he pushes all the error conditions into the unit tests. Personally, I typically don't have as dramatic dilemma as I write bigger systems where there are three or four levels of tests. And while I would not test error conditions in the end-to-end tests, I would not leave them for unit tests alone. I would have maybe one test at the lowest test level where I still have real network and HTTP, one test for each error category in tests that test the whole component logic in isolation from external resources and leave the rest to unit tests. My experience is that errors and validation is something that sometimes can be more tricky than the sunny day logic. Moreover, I remember writing some code where error detection and reporting is the only logic there is (e.g. assertion libraries) so (even though the openchat app is not like that) I personally believe that validation and error reporting is often underrated part of the system, especially when it is a big system.
11. An interesting observation is that, while Sandro tends to prefer vertical architecture slicing, his package structure does not seem reflect it (yet?). For example, he has domain.posts package where I would expect posts.domain with posts, the vertical feature, as the main package differentiator. That may be a temporary choice so I think I would need to watch more and see for myself.

And there it is. I suspect I will not be doing as lengthy notes onward as there will be less things I will see for the first time, but I can be wrong :-).