5

I received the code review comment for below code snippet -

public void doWork(String a, Integer b) {
    ..
    ..
    SomeService service = getService();
    for (Integer i : numbers) {
      doMoreWork(a, b, service);
    }
}
private void doMoreWork(String a, Integer b, SomeService service) {
    ...
    ...
    ...
    service.doingMoreWork(a, b);
}

review suggestion -

  1. SomeService service = getService(); should be called within doMoreWork for signature clarity. so signature becomes doMoreWork(a, b) which is more clear to understand.

My question -

Since doMoreWork() is happening in a loop, I am passing a service object to it. doMoreWork() is just a private logical unit within a code, it should be fine to carry "service" as method argument. This method is never going to be public. What is the guideline in this case? How the clarity or readability is affected here?

(NOTE :

  1. There is not much performance overhead to getService() call so performance is not a criteria for me here.
  2. Service injection is not an option here. It needs to be fetched the way shown in the sample.
  3. doMoreWork() is not just about calling service.doingMoreWork(). It has some pre-steps to follow.

)

3
  • 2
    If this is the real code, I would inline "doMoreWork", I don't see why you need a method here (it's 1 LoC)
    – user180100
    May 7, 2015 at 5:40
  • I have edited the sample code now. doMoreWork() is doing some more work!
    – Bruso
    May 7, 2015 at 7:58
  • I have asked the question to know the best practice or even fact behind this suggestion. This is something I could not find out from books so I posted it here. I am not sure if it is really a candidate for "hold". Even responses are reasonable and not just opinion based.
    – Bruso
    May 7, 2015 at 8:54

7 Answers 7

2

If you refactor doMoreWork() to the following:

private void doMoreWork(String a, Integer b) {
    SomeService service = getService();
    service.doingMoreWork(a, b);
}

then you are making doMoreWork() dependent on getService() for its functionality. If you ever want to use a different method to obtain a SomeService object you will have to refactor this code. So you might want to stick with the original implementation for this reason.

If you are using a framework like Spring, you could have the SomeService injected into your class like this:

@Autowired
private SomeService service;

// use the injected service here
private void doMoreWork(String a, Integer b) {
    service.doingMoreWork(a, b);
}
1
  • 1
    +1 This to me is the cleanest solution - nominal code and greatest flexibility. The doMoreWork method shouldn't care about the service used to do more work. You can swap out the service in a modular fashion with DI. May 7, 2015 at 6:01
0

As suggestion is right you need Service object only in the doMoreWork method. So it is good to get the Service object only in the doMoreWork method perform your operations.

If more statements are added after the loop, then Service object will last for long until its scope is there (until closing } is not encountered), it will not be garbage collected therefore you should call getService() in doMoreWork and pass 2 parameters in doMoreWork(a,b). It will be more easier to understand.

0

From a code clarity perspective, doMoreWork is an instance method in a class which knows its service (i.e., it has a getService() method). Putting the Service in the signature makes the reader wonder what Service this method is referring to, so it's somewhat confusing. Such a signature would make perfect sense, though, in a class which has multiple services and has to do the same type of work on several of them.

1
  • yes this could be one of the guideline to decide relevance of input argument. Thanks.
    – Bruso
    May 8, 2015 at 9:19
0

In all honesty, it does one no good to have that variable declared except within the doMoreWork method. Here's why:

  • No (apparent) mutations are being done to it inside of doWork
  • getService() appears to always return the same instance of SomeService

Depending on if getService is just a getter to a field, then it might be better to use the field directly instead of going through the method. But, you really don't need to pass this value in to the method.

0

I agree with the code review, getService() should be in doMoreWork().

Moreover, I suggest to make the service as your dependency of your class, that is, service can be a field of your class. Some DI framework can inject the service for you, such as Spring framework.

If so, this.service can be referenced in each method of your class.

1
  • yeah we did consider service injection. But in my case we can not do it. It has to be fetched the way I have done.
    – Bruso
    May 7, 2015 at 6:51
0

Passing Service as a parameter would have made sense, if getService() returned a new instance every time but then its name should have been createNewService() instead.

In current scenario it seems that getService() is always returning the same instance, so passing Service as a parameter does not make any difference but to confuse the reader.

Further adding, getService() is defined in the same class and doMoreWork() is a private method, so it would always refer to the same service probably than why add an extra step to pass Service every time!

Another thing, if all doMoreWork() does is to call service.doingMoreWork(a, b); nothing else than it would be better to just delete this method and call it directly in the loop like this:

public void doWork(String a, Integer b) {
    ..
    ..
    for (Integer i : numbers) {
        getService().doingMoreWork(a, b);
    }
}
0

It does not matter because:

doMoreWork() is marked private it is a hidden detail of the class. Since it is not public it is not part of the public API or Contract, it is an implementation detail that can change.

So it really does not matter what the signature is the reason given in your question is just personal preference.

You would never create a new JDBC connection inside a loop it would be extremely inefficient, even with a good connection pool it would be the wrong thing to do.

All that said I would do it this way.

The private method is not needed and just adds confusion and opinion.

public void doWork(@Nonnull final String s, @Nonnull final Integer i) 
{
    final SomeService service = getService();
    for (final Integer n : numbers) 
    {
      service.doingMoreWork(s, i);
    }
}
1
  • doMoreWork() has been extracted out as it has logical steps related to each other. It is not straight call to service.doMoreWork(). Let me edit the sample code.
    – Bruso
    May 7, 2015 at 7:29

Not the answer you're looking for? Browse other questions tagged or ask your own question.