Is inserting service logic in its own class considered bad?

Question

For example, let's say I have this wonderful interface:

public interface BestService   {
    public String whoIsAGoodService(String serviceName);
}

Going down it's implementation, we can find the following method:

@Override
public String whoIsAGoodService(String serviceName)   {
    String ipFromName = this.findOutIpFromName(serviceName);
    boolean isItReallyGood = this.findOutGoodService(ipFromName);
    String intermediateResponse = this.findOutGoodServiceMessage(isItReallyGood);
    String finalResponse = this.buildFinalResponse(intermediateResponse);

    return finalResponse;
}

The question there is not about code quality ⁽¹⁾. My question is:

Is it considered a good/bad practice to keep them in the ServiceImpl class ? Or should we create some class called BestServiceLogic to handle all those processes ? I would like some pieces of information about what is considered a best practice. My point of view would be that it tends to make service implementation classes very long, clustering all the sub-methods for all the methods being called by the client. But I would like some objective facts about this one.

For some context. I am re-writing legacy code ⁽²⁾ and I would like to figure out the best way to keep things clean and clear on the long-term.

I already read about this question but it treats more about class injection than method dispatching through classes.

---

^{(1) I know I could directly return the result of finalResponse and I don't know about using this.method() to call private members being a good or bad thing (if you want to enlighten me on the latter, however, I'd be delighted). It is an arbitrary, on-the-fly example.}

^{(2) do not worry, I won't break anything, I wrote tests first.}

Answer 1

Answer to the question in the title: No. But there are trade-offs.

The first question you have to ask yourself is this:

Is this same exact logic needed in more than one place?

If not, leave it where it is. No harm in it being in one place and additional complexity won't give you any benefit for your efforts.

If you have the same exact logic sprinkled in different places, you have copy/paste code which is a maintenance nightmare. You'll have to do a little more digging in.

Should my BestService be used everywhere that logic is needed, or is there a new service I need to extract?

If so, figure out how much of a service you really need. Is it just the IP discovery? Or do I have more than one service here?

In general, composition is a better solution than inheritance. It leads to less fragile code that has natural extension points if you ever need to change behavior.

---

RE: this.callMethod() is largely a stylistic thing. For methods it doesn't add or disambiguate anything, so it was probably just the habit of the original implementer of your class.

For disambiguating between a parameter and a class variable within a method it can be necessary. this.memberVariable = memberVariable assigns the class variable to the value of the parameter passed in with the same name. It's considered better practice to simply have a naming convention where you don't have conflicts. You can either name your parameter with the suffix In or Out depending on the direction (C# has an out parameter), or member variables all get an underscore (_) prefix.

It depends what looks more natural to you:

• memberVariable = memberVariableIn
• _memberVariable = memberVariable

If your language platform has a convention for this, it's probably a good idea to adopt it since it's lower friction for new members on your team to get familiar with the code.

Answer 2

For what I understand from your snippet, you have right there a scenario where you can benefit from the Decorator Pattern. You don't need a service-like layer, all you need is to add capabilities to your initial object.

There are a few advantages if you replace those private methods for real objects:

• You can perform unit testing against those objects.
• Logic can be shared across the codebase without having one place with all the logic;

Although, it only makes sense to extract those methods to decorators, if you really need to share the behaviour across the application. Otherwise, I would go with your solution.