Win a copy of TensorFlow 2.0 in Action this week in the Artificial Intelligence and Machine Learning forum!
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
programming forums Java Mobile Certification Databases Caching Books Engineering Micro Controllers OS Languages Paradigms IDEs Build Tools Frameworks Application Servers Open Source This Site Careers Other all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Campbell Ritchie
  • Liutauras Vilda
  • Paul Clapham
  • Bear Bibeault
  • Jeanne Boyarsky
Sheriffs:
  • Ron McLeod
  • Tim Cooke
  • Devaka Cooray
Saloon Keepers:
  • Tim Moores
  • Tim Holloway
  • Jj Roberts
  • Stephan van Hulst
  • Carey Brown
Bartenders:
  • salvin francis
  • Scott Selikoff
  • fred rosenberger

Code Review: Generic Service Class for Calling External REST APIs with RestTemplate

 
Ranch Hand
Posts: 189
14
Hibernate Eclipse IDE Spring
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hi folks,

There’s a third-party API with four endpoints that I’ve written a service class to interact with.  For the purposes of the example let’s say the endpoints are

/api/v1/dogs
/api/v1/cats
/api/v1/ducks
/api/v1/crocodiles

Each of the four endpoints are called using a RestTemplate and I’ve written a separate “wrapper” POJO to interpret the response from each endpoint.  Each of these four POJOs extends a base wrapper class that has three properties - a HttpStatus variable, a String variable that contains potential HTTP response error descriptions and a List<> of custom POJOs that I’m mapping errors to.  So my wrapper classes look like this











The third-party APIs are written in such a way that if anything other than a 2xx response is sent, they will respond with a JSON array of error objects, so I’ve also created a wrapper for this response too




Here is my service class that does all of the heavy lifting of calling these external endpoints.  I’ve left out the import statements and the MyCustomException code as I think it’s probably enough to understand how the code works without seeing those




Despite the fact that this is the best I could do, I’m not happy in the slightest.  There are a number of problems.  First, I tried to make the class as generic as possible, which is why I delegated all the RestTemplate calls to a single method called getResponse() that returns a generic ResponseEntity object.  But this has had the effect of making all of the methods getDogs, getCats, getDucks, getCrocodiles look the same if not very similar, which could confuse another developer.

Secondly, I don’t think I’m using generics very well here.  Can someone enlighten me and tell me if it could be improved on?

Thirdly, I’m not too sure that using two methods ensure…() to throw exceptions is clean code.  For a start, despite the fact that if the responseEntity.getBody() method returns null then a MyCustomException will be thrown but my IDE still says that setting the HttpStatus variable in the get…Wrapper() methods might still produce a NullPointerException.  Why is that?

There are probably dozens of things wrong with this class but I really want to learn and want to know what is the best way to write a service that calls an external REST API.  Any help would be welcome.
 
Marshal
Posts: 7794
536
Mac OS X VI Editor BSD Linux
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Cowgratulations, your post has been published in CodeRanch's August 2020 journal.
Staff note (Liutauras Vilda) :

@OP

 
Simon Ritchie
Ranch Hand
Posts: 189
14
Hibernate Eclipse IDE Spring
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thanks!
 
Sheriff
Posts: 15952
265
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
What's definitely smelly to me are all these classes that extend BaseWrapper. What are you "wrapping" with these classes anyway? What do you gain from making all these classes extend BaseWrapper? How can a DogWrapper be in the same class hierarchy as an ErrorWrapper? This hierarchy seems like an improper use of inheritance to me and I suspect there are many instances of violating the Liskov Substitution Principle

Also, what exactly is your understanding of "clean code"? For me, the basic qualities of clean code is that it makes sense, that it's intent is clear, and that it's easy to work with. At the very least, the clarity of intent quality is lacking in this code.
 
Simon Ritchie
Ranch Hand
Posts: 189
14
Hibernate Eclipse IDE Spring
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Junilu Lacar wrote:What's definitely smelly to me are all these classes that extend BaseWrapper. What are you "wrapping" with these classes anyway?



I'm wrapping the responses from a third party API.

Junilu Lacar wrote:What do you gain from making all these classes extend BaseWrapper?



Each of the response objects from the four different APIs I'm calling are different.  But they will all have a HttpStatus, a text field containing a description of the HttpStatus (i.e. "404 Not Found") and then, if the there was an error on the client side, a list of MyErrorDTO objects.  So rather than have those fields duplicated in the wrappers for the Dog, Cat, Crocodile APIs I thought they could all extend a base wrapper class to avoid duplication.

Junilu Lacar wrote:How can a DogWrapper be in the same class hierarchy as an ErrorWrapper?



Yes, that's a good point.

Junilu Lacar wrote:Also, what exactly is your understanding of "clean code"? For me, the basic qualities of clean code is that it makes sense, that it's intent is clear, and that it's easy to work with. At the very least, the clarity of intent quality is lacking in this code.



My understanding would be the same as yours in that clean code should first of all be easy to understand, should avoid unnecessary duplication and be easy to maintain.  I acknowledge that this code I've written is not very clear, which is why I decided to put it up for review.  I had toyed with the idea of writing four different methods, one for each client API call, but I thought this would lead to duplication of code where I was calling the RestTemplate.  I thought "Why not just have a generic method that calls the RestTemplate and then convert the response data to the relevant wrapper?"


 
Junilu Lacar
Sheriff
Posts: 15952
265
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I think the wrapper concept is not appropriate here. It's abstracting at the wrong level. The third party API is an external concern and calling out to it via a common  getResponse() method is more of an infrastructure concern than a domain concern. What you end up with are two different kinds of ideas (domain: Dogs, Cats, Crocodiles, etc. versus infrastructure) more tightly coupled. What happens when you need to support additional domain types like Chickens or Pigs? It would be pretty intrusive to this code, wouldn't it? It violates the Single Responsibility and the Open-Closed Principles.

In similiar situations, I would try to figure out a design where I only need to add new classes instead of adding new methods to an existing class to support additional domain types. The design would also either allow configuration-based registration of the new domain classes or have some way of discovering domain classes as they are added to the system. If you go the discovery route, it would also adapt the system when types have been removed or obsoleted.
 
Junilu Lacar
Sheriff
Posts: 15952
265
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
One other thing about clean code: it's easy to test. Think about what kind of challenges the current design poses with regard to testing. Then make the design so that testing becomes easier to do.
 
Simon Ritchie
Ranch Hand
Posts: 189
14
Hibernate Eclipse IDE Spring
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
That's a thoughtful response, thank you.  I've some further questions, if that's alright.

Junilu Lacar wrote:I think the wrapper concept is not appropriate here. It's abstracting at the wrong level.



What would you recommend I use instead?  I thought I only had two choices when it comes to using a RestTemplate -
1) creating a wrapper class to contain the response
2) mapping the response to a Map<String, Object>

I went with the wrapper approach because I thought the alternative (using a Map) would lead to lots of code where I would have to pick through the map looking for properties with specific names and then try and map those properties to DTOs or POJOs, etc.  

Junilu Lacar wrote:In similiar situations, I would try to figure out a design where I only need to add new classes instead of adding new methods to an existing class to support additional domain types.



Do you think then that a better design would be to have a separate class (with its own RestTemplate) for each external API call instead of cramming them all into one service as I've done here?  It makes sense but it leads to more classes...

Junilu Lacar wrote:The design would also either allow configuration-based registration of the new domain classes or have some way of discovering domain classes as they are added to the system. If you go the discovery route, it would also adapt the system when types have been removed or obsoleted.



Oh, that's interesting.  So basically, design it in such a way that on application startup the code should get the format of the responses from a configuration file?
 
Junilu Lacar
Sheriff
Posts: 15952
265
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Simon Ritchie wrote:Oh, that's interesting.  So basically, design it in such a way that on application startup the code should get the format of the responses from a configuration file?


That's not what I was thinking. More along the lines of a custom annotation that would tag a class as being one of these, for lack of a better term, "wrapper" classes. On startup, the application will scan for classes that are marked with that annotation. That would probably be the end goal because there are simpler ways that you can implement fairly quickly. I wouldn't go the "get the format fo the responses from a configuration file" route though. That seems more complicated than it needs to be. I assume the format of each response is stable and known so you can just code that into the wrapper class itself. I still don't think "wrapper" is the appropriate term for these things.
 
It's fun to be me, and still legal in 9 states! Wanna see my tiny ad?
Thread Boost feature
https://coderanch.com/t/674455/Thread-Boost-feature
reply
    Bookmark Topic Watch Topic
  • New Topic