Object-Oriented Design: Refactoring a REST Connector

In this story we will try to see how to improve a simple REST connector that places a request on an external service through an HTTP POST call and returns the result back from the remote server. The full example can be found . Following is the source code for our connector ( ): here Connector1 { Logger LOGGER = LogManager.getLogger(Connector1.class); String URI = ; ObjectMapper objectMapper; { .init(); .objectMapper = ObjectMapper(); } { { HttpResponse response = POST(URI, payload, ContentType.APPLICATION_JSON); String rawBody = rawResponse(response.getEntity()); LOGGER.debug( , itemId, rawBody); (response.getStatusLine().getStatusCode() == ) { LOGGER.info( , itemId, rawBody); mapRawResponse(rawBody); } { LOGGER.error( , response.getStatusLine().getStatusCode(), itemId, rawBody); ; } } (IOException e) { e.printStackTrace(); LOGGER.error(format( , itemId), e); ; } } { BufferedReader responseReader = BufferedReader( InputStreamReader(responseEntity.getContent())); responseReader.lines().collect(Collectors.joining()); } { objectMapper.readValue(rawBody, ResponseDTO.class); } } public class Connector1 extends AbstractConnector private static final private static final "http://someURI.com/path1/path2" private final public Connector1 () super // don't forget to call this! :( this new ResponseDTO public placeRequest (String payload, String itemId) try // just a simple and toy API for the example "[EXTERNAL->LOCAL] item_id={} | raw response body: [{}]" if 200 "[EXTERNAL->LOCAL] Successful response for item_id={}. Raw response body: [{}]" return else "[EXTERNAL->LOCAL] Unexpected response (HTTP {}) for item_id={}. Raw response body: [{}]" return null catch "[EXTERNAL->LOCAL] Error while processing item_id=%s" return null // swallowed exception! String IOException private rawResponse (HttpEntity responseEntity) throws new new return ResponseDTO JsonProcessingException private mapRawResponse (String rawBody) throws return The previous code reflects a commonly found REST connector pattern. Please note that although the analysis will be about the connector, several of the points discussed here apply to software design and source code organization in general. Inheritance Something that immediately catches my attention is that base class. Why is that used here? Let's see how that abstract class looks like: AbstractConnector { HttpClient httpClient; { (); } { .httpClient = createHttpClient(); } { HttpPost post = HttpPost(uri); post.setEntity( StringEntity(payload, contentType)); HttpResponse response = httpClient.execute(post); response; } { HttpClients.createDefault(); } { httpClient; } } public abstract class AbstractConnector private public AbstractConnector () super public void init () this HttpResponse IOException public POST (String uri, String payload, ContentType contentType) throws new new return HttpClient private createHttpClient () return HttpClient public getHttpClient () return It is quite common to find implementations like the previous one (again, this is just an example). This particular abstract class implementation doesn't add any value to the application; it only forwards the requests to the backed HTTP client library (Apache HTTP Client, in this case) and uses classes from that library for returning the result. In fact this connector limits the flexibility of that HTTP library and exposes the internal implementation details - at the same time! Please also note that hard-wiring of the instance. through getHttpClient() - HttpClient (Im) mutability And why is that method there, exposed to the public? On the one hand, the client is forced to manually initialize the connector; on the other hand, any client could call at any time! Those kind of organizations lead to unexpected (and bad) behaviors in general. init() init() We should favor object immutability whenever possible. This idea doesn't apply to object initialization itself but to maintaining objects immutable for their life. Some of the benefits of working with truly immutable objects are: whole there can't be invalid objects, fail-fast object creation (validations enforced at object creation time), they are inherently thread-safe, avoids defensive copying, simpler object testing Going back to ... Connector1 As mentioned before, there is no valid reason to use inheritance in cases like these (and many many others). We should carefully think whether inheritance is the best option for a particular scenario. really This (connector) model forces every potential connector in the app to have the same fixed behavior and to follow the same predefined rules. This relation is static in nature, and so, not changeable dynamically. A connector should be able to evolve on its own. We should favor composition over inheritance. In the example, the for POSTing is hard-coded. That is not a good practice. Usually the host is different for each deployment environment. Another chance is that you as a developer want to run a local HTTP mock server and you need to specify a local host and port in the URI. Or just because you may want to try custom URIs. In general it is fine to leave the URI path as a constant in the code, but not the host, port or schema. URI Please note that on returns a : placeRequest() Connector1 ResponseDTO { ResponseItemDTO item; } { String result; (value = ) ResponseContextDTO context; } { String trackingID; String someOtherContext; } public class ResponseDTO private // accessors... public class ResponseItemDTO private @JsonProperty "#context$$1234" // just a non-usual JSON field name to show implementation details private // accessors... public class ResponseContextDTO private private // accessors... That DTO is the object representation of the response sent by the external service. Exposing it not only forces clients (clients of the connector, in this case) to understand that representation but also to change whenever that representation changes. Further, a change on that representation could lead to errors and unexpected behavior on the app, even more if the representation is exposed transitively to more clients. As a general rule: The lower the coupling between classes, the better. Interfaces and contracts When two objects (or systems, in general) talk, there is a predefined interface. The contract declares what a party offers, in which terms, which are the supported formats, and so on. The point here is that we should encapsulate and hide internal implementation details and only offer a simplified view: the . That interface should act as a contract. interface An example may help: suppose this connector has a client that just needs the ID of the created resource on the remote server. It shouldn't matter whether the response is JSON or XML, or that ID is split up in several fields in the response. The connector API should only specify that responds an ID. And that ID representation should make sense to the application. placeRequest() It is a good idea to write interfaces in business terms. In our example, connector's clients shouldn't be working with a but with an abstraction that is simple and useful to the app (more on this, later). ResponseDTO We should program against interfaces and not implementations. Working with ' 's null Let's recall (a fragment of) the body of : placeRequest() { { HttpResponse response = ... String rawBody = ... (response.getStatusLine().getStatusCode() == ) { mapRawResponse(rawBody); } { ; } } (IOException e) { e.printStackTrace(); ; } } ResponseDTO public placeRequest (String payload, String itemId) try if 200 return else return null catch return null Note that this method returns the expected response Some of the issues with are: or null otherwise. null is an object, null NOT is a value or the absence of a value, depending on the case, null forces a particular handling (special-casing) null is polymorphic with anything! null NOT hard-to-find bugs In our example, is "returned" for two completely different situations: when the response from the server is not the expected, and when an exception is thrown. So, the client cannot differentiate among those cases; the client doesn't know what happened nor have all the context/information to take the best course of action. null We should carefully think of it before using null Logging to the console Please note that in the code. It is a good idea to avoid using things like or . Logging to the console has some drawbacks: e.printStackTrace() printStackTrace() System.out.println() we don't have control over it, it could be a bottleneck for high logging rates, data could be silently swallowed, we can't query/use previously logged data, we can't rotate the logs (by date, size, ...) We should use logging frameworks instead of logging to the console. (Unit) Testing the connector So, we have the connector ready and now we want to test it. Suppose we add the following test: { Connector1 underTest = Connector1(); underTest.placeRequest( , ); } @Test public void placeRequest_invalidInput () new null null This test will fail deep inside the base abstract connector because we are not validating the input for . placeRequest() We should always validate input and business constraints. Note that doing defensive programming everywhere is not preferable, though. A good approach is to validate the input and business constraints only at the entry point(s) of a subsystem or module. The entry point should be at well-defined place inside that subsystem or module. Let's add a happy-case test now: { Connector1 underTest = Connector1(); underTest.placeRequest( , ); } @Test public void placeRequest () new "payload ***" "s3209" This simple test should pass, right? Well, no... High-coupling between objects Should we run it, we would see the connector tries to contact an external service because the connector is highly coupled to a real HTTP Client implementation we don't have control over. That's bad news... In the example, is forced to use particular implementations for and . actually Connector1 HttpClient ObjectMapper High-coupling threatens testability and flexibility, and is a sign of bad design. An alternative to Connector1 Based on the previous discussion, we could rework like this: Connector1 { ; { ; ; { ; } } } interface Connector2 Try placeRequest (String payload, String itemId) interface Response String result () Context context () interface Context String trackingId () Now we have added an interface that specifies the contract between the connector and the rest of the app. Note that here are no implementation details. This interface just specifies that clients need to pass a and in (both of type String). It also specifies there be a response: . That response is in terms of the business ( ) and it is independent of the response representation received by the connector. payload itemId could Try Response [ is a Scala-like construction ( see ) that (and I quote) 'represents a computation that may either result in an exception, or return a successfully computed value... An important property of is its ability to pipeline, or chain, operations, catching exceptions along the way.'] Try here Try Note also that is declared inside because has meaning only for (and is used in the context of) . This organization puts the intents clear, mitigates bad class usages and helps avoiding namespace pollution in the app. Response Connector2 Response Connector2 Let's compare with : now the is the abstraction for 's result. Note that interface only declares what it offers in simple terms: , , and so on. can evolve without affecting connector's clients, and there is no implementation details (like JSON configuration, for example) in . Note also we use terms like and (nouns) and not terms like and (verbs). Unfortunately the use of getters and setters (accessors) is widespread: the problem is they encapsulation. Plus, clients of an object should not be able to distinguish whether a method invocation over that object is carried out by performing some computation internally or just by accessing some field. Setters play against immutability, and fields should be publicly exposed. Response ResponseDTO Response interface placeRequest() result() context() ResponseDTO Response result() context() getResult() getContext() break never Avoid using getters and setters whenever possible. Designing for testability In this example, we just took an existing connector implementation ( ), tested it, and reworked it. But it is better to design the other way around: When adding a new functionality, we declare the for it and then add all applicable test cases. This approach forces us to consciously think about how that functionality will be used. Once we have that interface and the associated tests reflect the expected results, we start adding functionality (in the form of a class that implements that interface) until all tests are . The same also applies to change to the existing code: maybe we need to add some new test cases; whichever the case, all the existing tests (old and new) should pass. Connector1 interface(s) actually green any Good designs are testable. The opposite is not necessarily true, though. Going back to , following is (a fragment of) an implementation of that interface: Connector2 { HttpClient httpClient; ObjectMapper objectMapper; String uriHost; { .httpClient = httpClient; .objectMapper = objectMapper; .uriHost = uriHost; } Try placeRequest(String payload, String itemId) { requireNonNull(payload, ); requireNonNull(itemId, ); String uri = uriHost + EXTERNAL_SERVICE_PATH; Try.ofFailable(() -> { HttpPost post = HttpPost(uri); post.setEntity( StringEntity(payload, ContentType.APPLICATION_JSON)); httpClient.execute(post); }) .flatMap(response -> { Try rawResponseTry = rawResponse(response.getEntity()); String rawBody = rawResponseTry.orElse( ); LOGGER.debug( , () -> itemId, () -> rawBody); (response.getStatusLine().getStatusCode() == ) { LOGGER.info( , itemId); toResponse(mapRawResponse(rawBody)); } { LOGGER.error( , () -> response.getStatusLine().getStatusCode(), () -> itemId, () -> rawBody); ; } }); } { Try.ofFailable(() -> { (BufferedReader responseReader = BufferedReader( InputStreamReader(responseEntity.getContent()))) { responseReader.lines().collect(Collectors.joining()); } }); } { Try.ofFailable(() -> objectMapper.readValue(rawBody, ResponseDTO.class)); } Try toResponse(Try dtoTry) { dtoTry.map(dto -> Response() { { dto.item.result; } { () -> dto.item.context.trackingID; } }); } { ResponseItemDTO item; ...below, the remaing DTOs } } public class Connector2Impl implements Connector2 private final private final private final public Connector2Impl (HttpClient httpClient, ObjectMapper objectMapper, String uriHost) this this this public "payload can not be null!" "itemId can not be null!" return new new return " " "[EXTERNAL->LOCAL] item_id={} | Raw response body=[{}]" if 200 "[EXTERNAL->LOCAL] Successful response for item_id={}" return else "[EXTERNAL->LOCAL] Unexpected response (HTTP {}) for item_id={}. Raw response body=[{}]" return null Try private rawResponse (HttpEntity responseEntity) return try new new return Try private mapRawResponse (String rawBody) return private return new @Override String public result () return @Override Context public context () return // external response format private static class ResponseDTO private Note that now the collaborators are passed in in the constructor. We use on every computation that may fail, chaining different transformations until we get the expected response... or an exception. Anyway, we still need to get rid of that when we have an unexpected response (see below). We can see (and its dependencies) only lives in the context of . See how we anonymously implement and interfaces. We don't need to actually have classes that implement those interfaces! Try null ResponseDTO Connector2Impl Response Context Please also note that is closing the . This is another common mistake when dealing with resources. We should always ensure we are releasing resources upon usage, including when some exception is thrown. It is a really good idea to always read the javadocs for every class we use. For classes that implement we can call for releasing the backed resources but, even better, use the Java (7+) offers. rawResponse(entity) BufferedReader java.io.Closeable close() try-with-resources This is the test for : Connector2Impl { { String uriHost = ; HttpClient httpClientMock = mock(HttpClient.class); ObjectMapper objectMapperMock = mock(ObjectMapper.class); HttpResponse httpResponseMock = mock(HttpResponse.class); StatusLine statusLineMock = mock(StatusLine.class); HttpEntity httpEntityMock = mock(HttpEntity.class); when(httpEntityMock.getContent()).thenReturn( ByteArrayInputStream( .getBytes())); when(statusLineMock.getStatusCode()).thenReturn( ); when(httpResponseMock.getEntity()).thenReturn(httpEntityMock); when(httpResponseMock.getStatusLine()).thenReturn(statusLineMock); when(httpClientMock.execute(any())).thenReturn(httpResponseMock); Connector2 underTest = Connector2Impl(httpClientMock, objectMapperMock, uriHost); Try responseTry = underTest.placeRequest( , ); Mockito.verify(httpClientMock, times( )).execute(any()); Mockito.verify(objectMapperMock, times( )).readValue(anyString(), any(Class.class)); assertNotNull(responseTry); assertTrue(responseTry.isSuccess()); } { String uriHost = ; HttpClient httpClientMock = mock(HttpClient.class); ObjectMapper objectMapperMock = mock(ObjectMapper.class); when(httpClientMock.execute(any())).thenThrow(SocketException.class); Connector2 underTest = Connector2Impl(httpClientMock, objectMapperMock, uriHost); Try responseTry = underTest.placeRequest( , ); Mockito.verify(httpClientMock, times( )).execute(any()); Mockito.verify(objectMapperMock, never()).readValue(anyString(), any(Class.class)); assertNotNull(responseTry); assertFalse(responseTry.isSuccess()); } } public class Connector2ImplTest @Test IOException public void placeRequest () throws "http://test.com" new "raw response" 200 new "payload ***" "s3209" 1 1 @Test IOException public void placeRequest_whenHTTPClientException_shouldReturnFailure () throws "http://test.com" new "payload ***" "s3209" 1 This test uses for state testing and for behavior testing. JUnit Mockito Recalling from the previous code snippet: Connector2 underTest = Connector2Impl(httpClientMock, objectMapperMock, uriHost); new Please note we are checking the operations declared in the contract/interface . We don't care how those operations are actually carried out; we do care only operations are available. In other words, we test an implementation ( , in this case) through the interface it ( , in this case) because we are testing the public interface. Connector2 which Connector2Impl implements Connector2 An alternative to Connector2 Remember the returned for unexpected responses? One way for getting rid of that could be by adding a new abstraction that holds a possible response and the context of the received response, whichever it is, in terms useful for the clients: null ExternalResponse { Optional maybeResponse; String rawResponse; { .maybeResponse = Optional.of(response); .rawResponse = rawResponse; } { .maybeResponse = Optional.empty(); .rawResponse = rawResponse; } { requireNonNull(response, ); ExternalResponse (response, rawResponse); } { ExternalResponse (rawResponse); } { maybeResponse; } } public class ExternalResponse T private final private final // this could be extended to ANY context private ExternalResponse (T response, String rawResponse) this this private ExternalResponse (String rawResponse) this this public static ExternalResponse of (T response, String rawResponse) "response can not be null!" return new public static ExternalResponse ofEmpty (String rawResponse) return new Optional public maybeResponse () return Note that now we have an wrapper over the possible response. Also, we have the raw response there. It could be extended to context, including remote call time, status/result code, and so on. Optional any Method complexity Maybe you noticed that receives the just for logging: this goes against simplicity and clarity. Why? When we see that method signature, maybe is not so clear how those parameters play together. In general, the more parameters a method have, the more difficult to understand that method is and harder to test and cover all the possible cases. In this example, is not actually required. In cases like this, an alternative could be to have this kind of data as part of the context (for example, in the executing thread's ). placeRequest(payload, itemId) itemId itemId local storage Based on the last discussion, a new method signature could be: Try > placeRequest(String payload); More on logging Maybe you noticed we are logging the raw response in this example. In some cases, those logged responses (requests and responses, in general) could contain classified/confidential information; that kind of information should not be logged. comes in handy for those cases. A client could for example take the raw response and save it to some storage. ExternalResponse Another important point while using loggers is about computational cost. Suppose in the previous example: LOGGER.debug( , itemId, rawBody); "[EXTERNAL->LOCAL] item_id={} | Raw response body=[{}]" that is computationally expensive to get. We could use the logging-level guards like or similar, or make use of the constructions some loggers like now offer: . With , the previous sentence would change to: itemId isDebugEnabled() log4j suppliers suppliers LOGGER.debug( , () -> itemId, () -> rawBody); "[EXTERNAL->LOCAL] item_id={} | Raw response body=[{}]" The advantage of using is that arguments are calculated/resolved when they are needed. By using suppliers we get rid of those guards. suppliers [ () -> {} ] actually Libraries used in this example org.apache.httpcomponents/httpclient version 4.5.13 com.fasterxml.jackson.core/jackson-databind version 2.12.0-rc2 org.apache.logging.log4j/log4j-core version 2.14.0 junit/junit version 4.13.1 (test) org.mockito/mockito-core version 3.5.10 (test) https://github.com/jasongoodwin/better-java-monads Related bibliography https://java-design-patterns.com/principles/ https://en.wikipedia.org/wiki/GRASP_(object-oriented_design) https://hackernoon.com/5-benefits-of-immutable-objects-worth-considering-for-your-next-project-f98e7e85b6ac https://rackandstack-tech.blog/2019/10/22/the-deep-synergy-between-testability-and-good-design-michael-feathers/ (not Java) https://www.infoq.com/articles/Testability https://en.wikipedia.org/wiki/Test-driven_development Functional Thinking: Paradigm Over Syntax (ISBN-13 : 978-1449365516 https://dzone.com/articles/getter-setter-use-or-not-use-0 https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html https://docs.oracle.com/javase/8/docs/api/java/util/function/Supplier.html https://docs.oracle.com/javase/7/docs/api/java/lang/ThreadLocal.html

Object-Oriented Design: Refactoring a REST Connector

Too Long; Didn't Read

Companies Mentioned

Coin Mentioned

@brunomasci

RELATED STORIES