Skip to content

Conversation

liubao68
Copy link
Contributor

No description provided.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 82.062% when pulling 9b31c6f on liubao68:discover into 9085931 on ServiceComb:master.

*/
public class RestTemplateWrapper extends RestTemplate {
private static RestTemplate cseRestTemplate = new CseRestTemplate();
private static List<AcceptableRestTemplate> acceptableRestTemplates = new ArrayList<>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a better design is to declare default rest template and avoid null checking all together

class RestTemplateWrapper implements RestOperations {
  private static final RestTemplate defaultTemplate =  new RestTemplate();
  private static List<AcceptableRestTemplate> acceptableRestTemplates = asList(new CseRestTemplate());

    private RestTemplate getRestTemplate(String url) {
        for (AcceptableRestTemplate template : acceptableRestTemplates) {
            if (template.isAcceptable(url)) {
                return template;
            }
        }
        return defaultTemplate;
    }
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and i don't understand why these methods need to be static. we have way too many static classes/methods. this kind of programming model creates tons of tight coupling.

RestTemplate template = RestTemplateBuilder.create();
try {
template.delete("http://test");
Assert.assertFalse(true);
Copy link
Member

@seanyinx seanyinx Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test only covers the happy path. what about no acceptable rest template found?

@coveralls
Copy link

Coverage Status

Coverage increased (+1.5%) to 83.462% when pulling 415236a on liubao68:discover into 9085931 on ServiceComb:master.

try {
template.delete("cse://test/a/b/c");
Assert.assertFalse(true);
} catch (NullPointerException e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be better if we can wrap the NPE with some kind of Runtime exception.

@WillemJiang WillemJiang merged commit 8d6f9c5 into apache:master Jun 28, 2017
@liubao68 liubao68 deleted the discover branch July 5, 2017 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants