stebennett/code-review.md

## code-review.md

      
    Raw
  

              code-review.md
            
          
    Code Review


Note I know that this is experimentation code.


General
Design
Spring
Testing
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 and force the client to handle it (which might be just to re-throw it).
RestTemplateErrorHandler#hasError(..)can be simplified to increase readability:

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?

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 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
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.