Hackernoon logoObject-Oriented Design: Refactoring a REST Connector by@brunomasci

Object-Oriented Design: Refactoring a REST Connector

Author profile picture

@brunomasciBruno Masci

Software and aerospace passionate.

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 here.
Following is the source code for our connector (

Connector1
):

public class Connector1 extends AbstractConnector {

    private static final Logger LOGGER = LogManager.getLogger(Connector1.class);

    private static final String URI = "http://someURI.com/path1/path2";

    private final ObjectMapper objectMapper;


    public Connector1() {
        super.init();   // don't forget to call this! :(
        this.objectMapper = new ObjectMapper();
    }

    public ResponseDTO placeRequest(String payload, String itemId) {
        try {
            HttpResponse response = POST(URI, payload, ContentType.APPLICATION_JSON);   // just a simple and toy API for the example

            String rawBody = rawResponse(response.getEntity());
            LOGGER.debug("[EXTERNAL->LOCAL] item_id={} | raw response body: [{}]", itemId, rawBody);

            if (response.getStatusLine().getStatusCode() == 200) {
                LOGGER.info("[EXTERNAL->LOCAL] Successful response for item_id={}. Raw response body: [{}]", itemId, rawBody);
                return mapRawResponse(rawBody);
            } else {
                LOGGER.error("[EXTERNAL->LOCAL] Unexpected response (HTTP {}) for item_id={}. Raw response body: [{}]",
                        response.getStatusLine().getStatusCode(), itemId, rawBody);
                return null;
            }
        } catch (IOException e) {
            e.printStackTrace();
            LOGGER.error(format("[EXTERNAL->LOCAL] Error while processing item_id=%s", itemId), e);
            return null;    // swallowed exception!
        }
    }

    private String rawResponse(HttpEntity responseEntity) throws IOException {
        BufferedReader responseReader = new BufferedReader(new InputStreamReader(responseEntity.getContent()));
        return responseReader.lines().collect(Collectors.joining());
    }

    private ResponseDTO mapRawResponse(String rawBody) throws JsonProcessingException {
        return objectMapper.readValue(rawBody, ResponseDTO.class);
    }
}

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

AbstractConnector
base class. Why is that used here? Let's see how that abstract class looks like:

public abstract class AbstractConnector {

    private HttpClient httpClient;


    public AbstractConnector() {
        super();
    }

    public void init() {
        this.httpClient = createHttpClient();
    }

    public HttpResponse POST(String uri, String payload, ContentType contentType) throws IOException {
        HttpPost post = new HttpPost(uri);
        post.setEntity(new StringEntity(payload, contentType));
        HttpResponse response = httpClient.execute(post);
        return response;
    }

    private HttpClient createHttpClient() {
        return HttpClients.createDefault();
    }

    public HttpClient getHttpClient() {
        return httpClient;
    }
}

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

getHttpClient()
- at the same time!
Please also note that hard-wiring of the
HttpClient
instance.

(Im) mutability

And why is that

init()
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
init()
at any time! Those kind of organizations lead to unexpected (and bad) behaviors in general.

We should favor object immutability whenever possible.

This idea doesn't apply to object initialization itself but to maintaining objects immutable for their whole life. Some of the benefits of working with truly immutable objects are:

  • 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 really the best option for a particular scenario.

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

Please note that

placeRequest()
on
Connector1
returns a
ResponseDTO
:

public class ResponseDTO {
    private ResponseItemDTO item;

    // accessors...
}

public class ResponseItemDTO {
    private String result;

    @JsonProperty(value = "#context$$1234")   // just a non-usual JSON field name to show implementation details
    private ResponseContextDTO context;
    
    // accessors...
}

public class ResponseContextDTO {
    private String trackingID;
    private String someOtherContext;

    // 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. That interface should act as a contract. 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 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

placeRequest()
responds an ID. And that ID representation should make sense to the application.

It is a good idea to write interfaces in business terms.

In our example, connector's clients shouldn't be working with a

ResponseDTO
but with an abstraction that is simple and useful to the app (more on this, later).

We should program against interfaces and not implementations.

Working with 'null 's

Let's recall (a fragment of) the body of

placeRequest()
:

    public ResponseDTO placeRequest(String payload, String itemId) {
        try {
            HttpResponse response = ...
            String rawBody = ...

            if (response.getStatusLine().getStatusCode() == 200) {
                return mapRawResponse(rawBody);
            } else {
                return null;
            }
        } catch (IOException e) {
            e.printStackTrace();
            return null;
        }
    }

Note that this method returns the expected response or null otherwise.
Some of the issues with null are:

  • null is NOT an object,
  • null is a value or the absence of a value, depending on the case,
  • null forces a particular handling (special-casing)
  • null is NOT polymorphic with anything!
  • hard-to-find bugs

In our example, null 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.

We should carefully think of it before using null

Logging to the console

Please note that

e.printStackTrace()
in the code. It is a good idea to avoid using things like
printStackTrace()
or
System.out.println()
. Logging to the console has some drawbacks:

  • 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:

    @Test
    public void placeRequest_invalidInput() {
        Connector1 underTest = new Connector1();

        underTest.placeRequest(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:

    @Test
    public void placeRequest() {
        Connector1 underTest = new Connector1();

        underTest.placeRequest("payload ***", "s3209");
    }

This simple test should pass, right? Well, no...

High-coupling between objects

Should we run it, we would see the connector actually 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,

Connector1
is forced to use particular implementations for
HttpClient
and
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

Connector1
like this:

interface Connector2 {

    Try<Response> 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

payload
and
itemId
in (both of type String). It also specifies there could be a response:
Try<Response>
. That response is in terms of the business (
Response
) and it is independent of the response representation received by the connector.

[

Try
is a Scala-like construction (see here) that (and I quote) 'represents a computation that may either result in an exception, or return a successfully computed value... An important property of 
Try
 is its ability to pipeline, or chain, operations, catching exceptions along the way.']

Note also that

Response
is declared inside
Connector2
because
Response
has meaning only for (and is used in the context of)
Connector2
. This organization puts the intents clear, mitigates bad class usages and helps avoiding namespace pollution in the app.

Let's compare

Response
with
ResponseDTO
: now the
Response
interface is the abstraction for
placeRequest()
's result. Note that interface only declares what it offers in simple terms:
result()
,
context()
, and so on.
ResponseDTO
can evolve without affecting connector's clients, and there is no implementation details (like JSON configuration, for example) in
Response
. Note also we use terms like
result()
and
context()
(nouns) and not terms like
getResult()
and
getContext()
(verbs). Unfortunately the use of getters and setters (accessors) is widespread: the problem is they break 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 never be publicly exposed.

Avoid using getters and setters whenever possible.

Designing for testability

In this example, we just took an existing connector implementation (

Connector1
), tested it, and reworked it. But it is better to design the other way around: When adding a new functionality, we declare the interface(s) for it and then add all applicable test cases. This approach forces us to consciously think about how that functionality will be actually 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 green. The same also applies to any 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.

Good designs are testable.

The opposite is not necessarily true, though.

Going back to

Connector2
, following is (a fragment of) an implementation of that interface:

public class Connector2Impl implements Connector2 {

    private final HttpClient httpClient;
    private final ObjectMapper objectMapper;
    private final String uriHost;

    public Connector2Impl(HttpClient httpClient, ObjectMapper objectMapper, String uriHost) {
        this.httpClient = httpClient;
        this.objectMapper = objectMapper;
        this.uriHost = uriHost;
    }

    public Try<Connector2.Response> placeRequest(String payload, String itemId) {
        requireNonNull(payload, "payload can not be null!");
        requireNonNull(itemId, "itemId can not be null!");

        String uri = uriHost + EXTERNAL_SERVICE_PATH;

        return Try.ofFailable(() -> {
                    HttpPost post = new HttpPost(uri);
                    post.setEntity(new StringEntity(payload, ContentType.APPLICATION_JSON));
                    return httpClient.execute(post);
                })
                .flatMap(response -> {
                    Try<String> rawResponseTry = rawResponse(response.getEntity());
                    String rawBody = rawResponseTry.orElse("<empty>");
                    LOGGER.debug("[EXTERNAL->LOCAL] item_id={} | Raw response body=[{}]", () -> itemId, () -> rawBody);

                    if (response.getStatusLine().getStatusCode() == 200) {
                        LOGGER.info("[EXTERNAL->LOCAL] Successful response for item_id={}", itemId);
                        return toResponse(mapRawResponse(rawBody));
                    } else {
                        LOGGER.error("[EXTERNAL->LOCAL] Unexpected response (HTTP {}) for item_id={}. Raw response body=[{}]",
                                () -> response.getStatusLine().getStatusCode(), () -> itemId, () -> rawBody);
                        return null;
                    }
                });
    }

    private Try<String> rawResponse(HttpEntity responseEntity) {
        return Try.ofFailable(() -> {
            try (BufferedReader responseReader = new BufferedReader(new InputStreamReader(responseEntity.getContent()))) {
                return responseReader.lines().collect(Collectors.joining());
            }
        });
    }

    private Try<ResponseDTO> mapRawResponse(String rawBody) {
        return Try.ofFailable(() -> objectMapper.readValue(rawBody, ResponseDTO.class));
    }

    private Try<Connector2.Response> toResponse(Try<ResponseDTO> dtoTry) {
        return dtoTry.map(dto -> new Response() {
            @Override
            public String result() {
                return dto.item.result;
            }

            @Override
            public Context context() {
                return () -> dto.item.context.trackingID;
            }
        });
    }

    // external response format

    private static class ResponseDTO {

        private ResponseItemDTO item;

        ...below, the remaing DTOs
    }
}

Note that now the collaborators are passed in in the constructor. We use

Try
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 null when we have an unexpected response (see below). We can see
ResponseDTO
(and its dependencies) only lives in the context of
Connector2Impl
. See how we anonymously implement
Response
and
Context
interfaces. We don't need to actually have classes that implement those interfaces!

Please also note that

rawResponse(entity)
is closing the
BufferedReader
. 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
java.io.Closeable
we can call
close()
for releasing the backed resources but, even better, use the try-with-resources Java (7+) offers.

This is the test for

Connector2Impl
:

public class Connector2ImplTest {

    @Test
    public void placeRequest() throws IOException {
        String uriHost = "http://test.com";

        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(new ByteArrayInputStream("raw response".getBytes()));

        when(statusLineMock.getStatusCode()).thenReturn(200);

        when(httpResponseMock.getEntity()).thenReturn(httpEntityMock);
        when(httpResponseMock.getStatusLine()).thenReturn(statusLineMock);

        when(httpClientMock.execute(any())).thenReturn(httpResponseMock);

        Connector2 underTest = new Connector2Impl(httpClientMock, objectMapperMock, uriHost);

        Try<Connector2.Response> responseTry = underTest.placeRequest("payload ***", "s3209");

        Mockito.verify(httpClientMock, times(1)).execute(any());

        Mockito.verify(objectMapperMock, times(1)).readValue(anyString(), any(Class.class));

        assertNotNull(responseTry);
        assertTrue(responseTry.isSuccess());
    }

    @Test
    public void placeRequest_whenHTTPClientException_shouldReturnFailure() throws IOException {
        String uriHost = "http://test.com";

        HttpClient httpClientMock = mock(HttpClient.class);
        ObjectMapper objectMapperMock = mock(ObjectMapper.class);

        when(httpClientMock.execute(any())).thenThrow(SocketException.class);

        Connector2 underTest = new Connector2Impl(httpClientMock, objectMapperMock, uriHost);

        Try<Connector2.Response> responseTry = underTest.placeRequest("payload ***", "s3209");

        Mockito.verify(httpClientMock, times(1)).execute(any());

        Mockito.verify(objectMapperMock, never()).readValue(anyString(), any(Class.class));

        assertNotNull(responseTry);
        assertFalse(responseTry.isSuccess());
    }
}

This test uses JUnit for state testing and Mockito for behavior testing.

Recalling from the previous code snippet:

Connector2 underTest = new Connector2Impl(httpClientMock, objectMapperMock, uriHost);

Please note we are checking the operations declared in the contract/interface

Connector2
. We don't care how those operations are actually carried out; we do care only which operations are available.
In other words, we test an implementation (
Connector2Impl
, in this case) through the interface it implements (
Connector2
, in this case) because we are testing the public interface.

An alternative to Connector2

Remember the null returned for unexpected responses? One way for getting rid of that could be by adding a new abstraction

ExternalResponse<T>
that holds a possible response and the context of the received response, whichever it is, in terms useful for the clients:

public class ExternalResponse<T> {

    private final Optional<T> maybeResponse;

    private final String rawResponse;     // this could be extended to ANY context


    private ExternalResponse(T response, String rawResponse) {
        this.maybeResponse = Optional.of(response);
        this.rawResponse = rawResponse;
    }

    private ExternalResponse(String rawResponse) {
        this.maybeResponse = Optional.empty();
        this.rawResponse = rawResponse;
    }

    public static <T> ExternalResponse<T> of(T response, String rawResponse) {
        requireNonNull(response, "response can not be null!");

        return new ExternalResponse<T>(response, rawResponse);
    }

    public static <T> ExternalResponse<T> ofEmpty(String rawResponse) {
        return new ExternalResponse<T>(rawResponse);
    }

    public Optional<T> maybeResponse() {
        return maybeResponse;
    }
}

Note that now we have an

Optional
wrapper over the possible response.
Also, we have the raw response there. It could be extended to any context, including remote call time, status/result code, and so on.

Method complexity

Maybe you noticed that

placeRequest(payload, itemId)
receives the
itemId
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,
itemId
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 local storage).

Based on the last discussion, a new method signature could be:

Try<ExternalResponse<Response>> 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.

ExternalResponse<T>
comes in handy for those cases. A client could for example take the raw response and save it to some storage.

Another important point while using loggers is about computational cost.
Suppose in the previous example:

LOGGER.debug("[EXTERNAL->LOCAL] item_id={} | Raw response body=[{}]", itemId, rawBody);

that

itemId
is computationally expensive to get. We could use the logging-level guards like
isDebugEnabled()
or similar, or make use of the constructions some loggers like log4j now offer: suppliers. With suppliers, the previous sentence would change to:

LOGGER.debug("[EXTERNAL->LOCAL] item_id={} | Raw response body=[{}]", () -> itemId, () -> rawBody);

The advantage of using suppliers [ () -> {} ] is that arguments are calculated/resolved when they are actually needed. By using suppliers we get rid of those guards.

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

Tags

Join Hacker Noon

Create your free account to unlock your custom reading experience.