Kdtyjb

The original code is using member variables like arguments. When he says to minimize the number of arguments, what he really means is to minimize the amount of data that the methods requires in order to function. Putting that data into member variables doesn't improve anything.

"It contradicts what someone's uncle thinks" is NEVER a good argument. NEVER. Don't take wisdom from uncles, think for yourself.

That said, instance variables should be used to store information that is actually required to be stored permanently or semi-permanently. The information here isn't. It is very simple to live without the instance variables, so they can go.

Test: Write a documentation comment for each instance variable. Can you write anything that isn't completely pointless? And write a documentation comment to the four accessors. They are equally pointless.

Worst is, assume the way to decrypt changes, because you use a different cryptoService. Instead of having to change four lines of code, you have to replace four instance variables with different ones, four getters with different ones, and change four lines of code.

But of course the first version is preferable if you are paid by the line of code. 31 lines instead of 11 lines. Three times more lines to write, and to maintain forever, to read when you are debugging something, to adapt when changes are needed, to duplicate if you support a second cryptoService.

(Missed the important point that using local variables forces you to make the calls in the right order).

Discussing just process(...), your colleagues example is far more legible in the sense of business logic. Conversely your counter example takes more than a cursory glance to extract any meaning.

That being said, clean code is both legible and good quality - pushing local state out into a more global space is just high-level assembly, so a zero for quality.

public class SomeBusinessProcess {

  @Inject private Router router;

  @Inject private ServiceClient serviceClient;

  @Inject private CryptoService cryptoService;



  public EncryptedResponse process(EncryptedRequest request) {

    checkNotNull(encryptedRequest);



    return encryptResponse

      (routeTo

         ( destination()

         , requestData(request)

         , destinationEncryption()

         )

      );

  }



  private byte[] requestData(EncryptedRequest encryptedRequest) {

    return cryptoService.decryptRequest(encryptedRequest, byte[].class);

  }



  private EncryptionInfo destinationEncryption() {

    return cryptoService.getEncryptionInfoForDefaultClient();

  }



  private URI destination() {

    return router.getDestination().getUri();

  }



  private EncryptedObject routeTo(URI destinationURI, byte[] encodedData, EncryptionInfo encryptionInfo) {

    return serviceClient.handle(destinationURI, encodedData, encryptionInfo);

  }



  private void encryptResponse(EncryptedObject payloadOfResponse) {

    return cryptoService.encryptResponse(payloadOfResponse);

  }

}

This is a rendition that removes the need for variables at any scope. Yes the compiler will generate them but the important part is that it controls that so the code will be efficient. While also being relatively legible.

Just a point on naming. You want the shortest name that is meaningful and expands on the information already available. ie. destinationURI, the 'URI' is already known by the type signature.

Other answers have already explained the benefits of local variables perfectly, so all that remains is this part of your question:

Yet despite these benefits I still cannot seem to convince the original developer that this refactoring is warranted, as it appears to contradict the practices of Uncle Bob mentioned above.

That should be easy. Simply point him to the following quote in Uncle Bob's Clean Code:

Have No Side Effects

Side effects are lies. Your function promises to do one thing, but it also does other hidden things. Sometimes it will make unexpected changes to the variables of its own class. Sometimes it will make them to the parameters passed into the function or to system globals. In either case they are devious and damaging mistruths that often result in strange temporal couplings and order dependencies.

(example omitted)

This side effect creates a temporal coupling. That is, checkPassword can only be called at certain times (in other words, when it is safe to initialize the session). If it is called out of order, session data may be inadvertently lost. Temporal couplings are confusing, especially when hidden as a side effect. If you must have a temporal coupling, you should make it clear in the name of the function. In this case we might rename the function checkPasswordAndInitializeSession, though that certainly violates “Do one thing.”

That is, Uncle Bob doesn't just say that a function should take few arguments, he also says that functions should avoid interacting with non-local state whenever possible.

Flater's answer covers issues of scoping quite well, but I think that there's another issue here too.

Note that there's a difference between a function that processes data and a function that simply accesses data.

The former executes actual business logic, whereas the latter saves typing and perhaps adds safety by adding a simpler and more reusable interface.

In this case it seems that the data-access functions don't save typing, and aren't reused anywhere (or there would be other issues in removing them). So these functions simply shouldn't exist.

By keeping only the business logic in named functions, we get the best of both worlds (somewhere between Flater's answer and imel96's answer):

public EncryptedResponse process(EncryptedRequest encryptedRequest) {



    byte[] requestData = decryptRequest(encryptedRequest);

    EncryptedObject responseData = handleRequest(router.getDestination().getUri(), requestData, cryptoService.getEncryptionInfoForDefaultClient());

    EncryptedResponse response = encryptResponse(responseData);



    return response;

}



// define: decryptRequest(), handleRequest(), encryptResponse()

What is the objective, scientific rationale to favor local variables
over instance variables? I can't quite seem to put my finger on it. My
intuition tells me that hidden couplings are bad and that a narrow
scope is better than a broad one. But what is the science to back this
up?

Instance variables are for representing the properties of their host object, not for representing properties specific to threads of computation more narrowly scoped than the object itself. Some of the reasons for drawing such a distinction that appear not to already have been covered revolve around concurrency and reentrancy. If methods exchange data by setting the values of instance variables, then two concurrent threads can easily clobber each other's values for those instances variables, yielding intermittent, hard-to-find bugs.

Even one thread alone can encounter problems along those lines, because there is a high risk that a data exchange pattern relying on instance variables makes methods non-reentrant. Similarly, if the same variables are used to convey data between different pairs of methods, then there is a risk that a single thread executing even a non-recursive chain of method invocations will run into bugs revolving around unexpected modifications of the instance variables involved.

In order to get reliably correct results in such a scenario, you need either to use separate variables to communicate between each pair of methods where one invokes the other, or else to have every method implementation take into account all the implementation details of all the other methods it invokes, whether directly or indirectly. This is brittle, and it scales poorly.

I would just remove these variables and private methods altogether. Here's my refactor:

public class SomeBusinessProcess {

  @Inject private Router router;

  @Inject private ServiceClient serviceClient;

  @Inject private CryptoService cryptoService;



  public EncryptedResponse process(EncryptedRequest encryptedRequest) {

    return cryptoService.encryptResponse(

        serviceClient.handle(router.getDestination().getUri(),

        cryptoService.decryptRequest(encryptedRequest, byte[].class),

        cryptoService.getEncryptionInfoForDefaultClient()));

  }

}

For private method, e.g. router.getDestination().getUri() is clearer and more readable than getDestinationURI(). I would even just repeat that if I use the same line twice in the same class. To look at it another way, if there's a need for a getDestinationURI(), then it probably belongs in some other class, not in SomeBusinessProcess class.

For variables and properties, the common need for them is to hold values to be used later in time. If the class has no public interface for the properties, they probably shouldn't be properties. The worst kind of class properties use is probably for passing values between private methods by way of side effects.

Anyway, the class only needs to do process() and then the object will be thrown away, there's no need to keep any state in memory. Further refactor potential would be to take out the CryptoService out of that class.

Based on comments, I want to add this answer is based on real world practice. Indeed, in code review, the first thing that I'd pick out is to refactor the class and move out the encrypt/decrypt work. Once that's done, then I'd ask if the methods and variables are needed, are they named correctly and so on. The final code will probably closer to this:

public class SomeBusinessProcess {

  @Inject private Router router;

  @Inject private ServiceClient serviceClient;



  public Response process(Request request) {

    return serviceClient.handle(router.getDestination().getUri());

  }

}

With above code, I don't think it needs further refactor. As with the rules, I think it takes experience to know when and when not to apply them. Rules are not theories that are proven to work in all situations.

Code review on the other hand has real impact on how long before a piece of code can pass. My trick is to have less code and make it easy to understand. A variable name can be a point of discussion, if I can remove it reviewers wouldn't even need to think about it.

The first and most important thing: Uncle Bob seems to be like a preacher sometimes, but states that there are exceptions to his rules.

The whole idea of Clean Code is to improve readability and to avoid errors. There are several rules that are violating each other.

His argument on functions is that niladic functions are best, however that up to three parameters are acceptable. I personally think that 4 are also ok.

When instance variables are used, they should make a coherent class. That means, the variables should be used in many, if not all non-static methods.

Variables that are not used in many places of the class, should be moved.

I would neither consider the original nor the refactored version optimal, and @Flater stated already very well what can be done with return values. It improves readability and reduces errors to use return values.

Local variables reduce scope hence limits the ways in which the variables can be used and hence helps prevent certain classes of error, and improves readability.

Instance variable reduce the ways in which the function can be called which also helps reduce certain classes of error, and improves readability.

To say one is right and the other is wrong may well be a valid conclusion in any one particular case, but as general advice...

TL;DR: I think the reason you smell too much zeal is, there's too much zeal.

Despite the fact that methods starting with get... should not return void, the separation of levels of abstractions within the methods is given in the first solution. Although the second solution is more scoped it is still harder to reason about what is going on in the method. The assignments of local variables are not needed here. I would keep the method names and refactor the code to something like that:

public class SomeBusinessProcess {

  @Inject private Router router;

  @Inject private ServiceClient serviceClient;

  @Inject private CryptoService cryptoService;



  public EncryptedResponse process(EncryptedRequest encryptedRequest) {

    checkNotNull(encryptedRequest);



    return getEncryptedResponse(

            passRequestToServiceClient(getDestinationURI(), getEncodedData(encryptedRequest) getEncryptionInfo())

        );

  }



  private EncryptedResponse getEncryptedResponse(EncryptedObject encryptedObject) {

    return cryptoService.encryptResponse(encryptedObject);

  }



  private byte[] getEncodedData(EncryptedRequest encryptedRequest) {

    return cryptoService.decryptRequest(encryptedRequest, byte[].class);

  }



  private EncryptionInfo getEncryptionInfo() {

    return cryptoService.getEncryptionInfoForDefaultClient();

  }



  private URI getDestinationURI() {

    return router.getDestination().getUri();

  }



  private EncryptedObject passRequestToServiceClient(URI destinationURI, byte[] encodedData, EncryptionInfo encryptionInfo) {

    return serviceClient.handle(destinationURI, encodedData, encryptionInfo);

  }

}

Control of state change, which leads to less coupling and/or the accidental introduction of side effects.

I would have added an example, but I think everyone else in this thread has already provided one.