stebennett · March 15, 2016 13:04 · Mar 15, 2016 · Mar 15, 2016
diff --git a/code-review.md b/code-review.md
@@ -16,7 +16,7 @@
 * Be consistent with the use of annotations and dependency injection. Sometimes it's used on fields, sometimes on constructors. If you're doing it multiple ways, you need to explain why.
 
 ## Design
-* Good to see abstraction layers. It looks a little like the hexagonal architecture implementation, although some parts (`ProductJson`) are bleeding between the layers.
+* Good to see abstraction layers. It looks a little like the [hexagonal architecture](http://alistair.cockburn.us/Hexagonal+architecture) implementation, although some parts (`ProductJson`) are bleeding between the layers.
 * `ProductJson` is poorly named. It looks like a data model. We should separate the representation content type from the domain data model.
 * `Message` class doesn't provide value any value at this time, as it's only extended by `ErrorMessage`.
 * As `ErrorMessage` is a representation, it could be argued that it should be an immutable class (no-setters, fields declared in constructor). Same thing could be argued for `ProductJson`. Doing this allows you to control the creation of these objects and ensure that fields that must be set are actually set (f.ex, every product needs a name and ID, however it's possible to create one that doesn't). Changing this to constructor args means we can check this at compile time.

diff --git a/code-review.md b/code-review.md
@@ -0,0 +1,66 @@
+# Code Review
+
+> **Note** I know that this is experimentation code.
+
+* [General](#general)
+* [Design](#design)
+* [Spring](#spring)
+* [Testing](#testing)
+* [Java](#java)
+
+## General
+* Formatting is off. Makes the code look clumsy, and difficult to follow. Be consistent with indentation, naming, comments, spacing etc.
+* Lots of comments providing no real value. See `GlobalExceptionHandler`, `OrionAssemblerMvpController` for examples.
+* Annotations that are not next to the code which they operate on. See `OrionAssemblerMvpController`
+* `TODO` left in code. If it's just putting something in a constant, then we should just do it, rather than leave the TODO. The cost of not doing it is higher than making the change.
+* Be consistent with the use of annotations and dependency injection. Sometimes it's used on fields, sometimes on constructors. If you're doing it multiple ways, you need to explain why.
+
+## Design
+* Good to see abstraction layers. It looks a little like the hexagonal architecture implementation, although some parts (`ProductJson`) are bleeding between the layers.
+* `ProductJson` is poorly named. It looks like a data model. We should separate the representation content type from the domain data model.
+* `Message` class doesn't provide value any value at this time, as it's only extended by `ErrorMessage`.
+* As `ErrorMessage` is a representation, it could be argued that it should be an immutable class (no-setters, fields declared in constructor). Same thing could be argued for `ProductJson`. Doing this allows you to control the creation of these objects and ensure that fields that must be set are actually set (f.ex, every product needs a name and ID, however it's possible to create one that doesn't). Changing this to constructor args means we can check this at compile time.
+* By extending `RuntimeException` for `APIException`, you're creating an _unchecked_ Exception. This means that calling code is not forced to make the decision as to whether it should handle it. In turn, this results in `OrionAssembleServiceImpl#assembleData(..)` throwing exceptions that could be handled (and recovered) by the client, yet are unknown without checking the implementation. It's usually better to [favor checked exceptions](https://docs.oracle.com/javase/tutorial/essential/exceptions/runtime.html) and force the client to handle it (which might be just to re-throw it).
+* `RestTemplateErrorHandler#hasError(..)`can be simplified to increase readability:
+
+```java
+public boolean hasError(ClientHttpResponse response) throws IOException {
+  return 200 != response.getRawStatusCode();
+}
+```
+* I'd also be tempted to replace `200` with the Constant from `HttpStatus.OK`, to further help readability.
+* I would be tempted to create `InventoryService` and `ProductService` classes which are composed of an `OrionCallMvpService` (to do the Http call), and a _Mapper_ (to Map to Product Model, or Inventory Model). Doing so would encapsulate calls to Product and Inventory services. By calling Product and Inventory service through the same `OrionCallMvpService`, we create an artificial coupling in our code between these two services - a coupling which doesn't exist in our domain.
+* `OrionAssembleServiceImpl#orionCallService` is default modifier Is this what you expect, especially given the fact that it's not final?
+* I'm not sure about the implementation which throws an `APIException` because an item is out of stock. Is there a better way we could do this?
+* The catching, unwrapping and re-throwing logic of the `ExcecutionException` in `OrionAssembleServiceImpl` is clumsy, and is possibly a consequence of the collaborator not throwing a checked exception.
+* You already have a `RestTemplateErrorHandler` checking for statusCode != 200, so do we really need to do the check again in `OrionCallServiceImpl`?
+
+
+## Spring
+* The code uses Spring annotations to set fields to property values and dependencies. This means that any unit tests for these classes need to be run in a Spring container, creating an unnecessary overhead. I would favor constructor injection as it then allows the class to be created without the Spring container. It also allows us to check that our collaborators are correctly set. Dependency injecttion does not have to mean we use Spring (examples can be seen in `OrionAssembleServiceImpl`).
+* `OrionAssembleServiceImpl` isn't threadsafe because of the `ProductJson` field. Beans created by the Spring container are _singleton_ by default. Without knowing Spring well, it's easy to fall into this trap.
+* `OrionAssembleServiceImpl#checkInventory(int)` can be simplified. Also, why the use of Boxed primitives?
+
+```java
+public boolean checkInventory(int quantity) {
+  return quantity < 50;
+}
+```
+
+* The request headers are common for all clients of the `RestTemplate` we're using so why not set them on the `RestTemplate`? This will make the call to the endpoint simpler.
+* I'm not a fan of using the Spring annotation to enable Async calls. It's just something I'm not comfortable with, there seems to be too much "magic" involved. _This is just a personal preference_.
+
+## Testing
+* Tests don't need the `test_` prefix. From JUnit 4, the `@Test` annotation is used to control test execution.
+* `OrionAssembleServiceTest#test_CustomException()` doesn't check the low stock, it just checks that a call throws an `Exception`. Indicates a problem with our design.
+* `OrionCallMvpServiceImplTest#throwCustomServiceAPIException()` - it's unclear what causes the `APIException` to be thrown in this test case.
+* Tests are "fat" integration tests. Is this what you were aiming for? What about unit tests?
+* What value do we drive by checking the error message in the Exception is correct?
+
+## Java
+* Interfaces and Impl classes (f.ex `OrionAssembleService` and `OrionAssembleServiceImpl`). Why do we need the interfaces?
+* Interfaces with unnecessary `public` modifiers.
+* [Train wreck](http://c2.com/cgi/wiki?TrainWreck) calls. e.g `productData.get().findValue("name").textValue()`. This makes it difficult to write tests for, especially if you want to Mock stuff out. Also breaks the [Law of Demeter](https://en.wikipedia.org/wiki/Law_of_Demeter)
+* Hidden exceptions. `InterruptedException` in `OrionAssembleService` is consumed, meaning the returning `productJson` object will be left in an inconsistent state.
+* No need to initialise variables at the start of the method. (e.g `OrionCallMvpServiceImpl#fetchServiceData(String, String)` - `request`, `response`).
+* I'd like to see a greater use of `final` for fields which we don't expect consumers to change.