Rationale to prefer local variables over instance variables?2019 Community Moderator ElectionClean code: consequences of short methods with few parametersParameterizing vs property assignmentPure functional vs tell, don't ask?Does not testing internals entail diligent refactoring and/or rely on developer talent?Naming conventions for instance, local and parameter variablesAbstract methods vs instance variables for reusable objectsHow to restructure Python frameworksDid the gradual shift in methodology of writing code affect system performance? And Should I care?Why prefer non-static inner classes over static ones?What are the differences between class variables and instance variables in Java?Class instance while retaining variables between activitiesAssigning local variables in a bulk wayUsing local variables vs Making multiple function/method calls

Is there a full canon version of Tyrion's jackass/honeycomb joke?

Should I use HTTPS on a domain that will only be used for redirection?

What is better: yes / no radio, or simple checkbox?

How can neutral atoms have exactly zero electric field when there is a difference in the positions of the charges?

How to kill a localhost:8080

Create chunks from an array

Difference between 'stomach' and 'uterus'

Where is the fallacy here?

PTIJ: What dummy is the Gemara referring to?

Practical reasons to have both a large police force and bounty hunting network?

Split a number into equal parts given the number of parts

I encountered my boss during an on-site interview at another company. Should I bring it up when seeing him next time?

How to disable or uninstall iTunes under High Sierra without disabling SIP

If there are any 3nion, 5nion, 7nion, 9nion, 10nion, etc.

Is there a math equivalent to the conditional ternary operator?

Are small insurances worth it

Draw bounding region by list of points

Would the melodic leap of the opening phrase of Mozart's K545 be considered dissonant?

“I had a flat in the centre of town, but I didn’t like living there, so …”

When was drinking water recognized as crucial in marathon running?

PTIJ: Is all laundering forbidden during the 9 days?

Book about a time-travel war fought by computers

I've given my players a lot of magic items. Is it reasonable for me to give them harder encounters?

Was it really inappropriate to write a pull request for the company I interviewed with?



Rationale to prefer local variables over instance variables?



2019 Community Moderator ElectionClean code: consequences of short methods with few parametersParameterizing vs property assignmentPure functional vs tell, don't ask?Does not testing internals entail diligent refactoring and/or rely on developer talent?Naming conventions for instance, local and parameter variablesAbstract methods vs instance variables for reusable objectsHow to restructure Python frameworksDid the gradual shift in methodology of writing code affect system performance? And Should I care?Why prefer non-static inner classes over static ones?What are the differences between class variables and instance variables in Java?Class instance while retaining variables between activitiesAssigning local variables in a bulk wayUsing local variables vs Making multiple function/method calls










61















The codebase I'm working on frequently uses instance variables to share data between various trivial methods. The original developer is adamant that this adheres to the best practices stated in the Clean Code book by Uncle Bob/Robert Martin: "The first rule of functions is that they should be small." and "The ideal number of arguments for a function is
zero (niladic). (...) Arguments are hard. They take a lot of conceptual power."



An example:



public class SomeBusinessProcess 
@Inject private Router router;
@Inject private ServiceClient serviceClient;
@Inject private CryptoService cryptoService;

private byte[] encodedData;
private EncryptionInfo encryptionInfo;
private EncryptedObject payloadOfResponse;
private URI destinationURI;

public EncryptedResponse process(EncryptedRequest encryptedRequest)
checkNotNull(encryptedRequest);

getEncodedData(encryptedRequest);
getEncryptionInfo();
getDestinationURI();
passRequestToServiceClient();

return cryptoService.encryptResponse(payloadOfResponse);


private void getEncodedData(EncryptedRequest encryptedRequest)
encodedData = cryptoService.decryptRequest(encryptedRequest, byte[].class);


private void getEncryptionInfo()
encryptionInfo = cryptoService.getEncryptionInfoForDefaultClient();


private void getDestinationURI()
destinationURI = router.getDestination().getUri();


private void passRequestToServiceClient()
payloadOfResponse = serviceClient.handle(destinationURI, encodedData, encryptionInfo);




I would refactor that into the following using local variables:



public class SomeBusinessProcess 
@Inject private Router router;
@Inject private ServiceClient serviceClient;
@Inject private CryptoService cryptoService;

public EncryptedResponse process(EncryptedRequest encryptedRequest)
checkNotNull(encryptedRequest);

byte[] encodedData = cryptoService.decryptRequest(encryptedRequest, byte[].class);
EncryptionInfo encryptionInfo = cryptoService.getEncryptionInfoForDefaultClient();
URI destinationURI = router.getDestination().getUri();
EncryptedObject payloadOfResponse = serviceClient.handle(destinationURI, encodedData,
encryptionInfo);

return cryptoService.encryptResponse(payloadOfResponse);




This is shorter, it eliminates the implicit data coupling between the various trivial methods and it limits the variable scopes to the minimum required. 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.



Hence my questions:
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?



And conversely, are there any downsides to this refactoring that I have possibly overlooked?










share|improve this question









New contributor




Alexander is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.















  • 148





    "The ideal number of arguments for a function is zero" is plain wrong. The ideal number of arguments is exactly the number needed to enable your function to be side-effect free. Less than that and you needlessly cause your functions to be impure thus forcing you to steer away from the pit of success and climb the gradient of pain. Sometimes "Uncle Bob" is spot on with his advice. Sometimes he is spectacularly wrong. His zero arguments advice is an example of the latter.

    – David Arno
    yesterday







  • 57





    Your colleague has fallen victim to Robert Martin's successful marketing. You will have a hard time convincing him that the teachings spread in that book really have a very narrow context and are not generally applicable to software engineering. To admit that would mean to give up an ideology. You are asking about scientific arguments in favour of your approach, but Robert Martin has never been about science. There is no critical, open exchange of viewpoints in his writings, it's always all about absolute rules and dogmas. You are facing a purely psychological problem here.

    – Christian Hackl
    yesterday






  • 20





    @R.Schmitz: The problem with Martin is his big influence, especially on junior developers. When you work a lot with others, then you have to deal with his ideas sooner or later, and that is becoming tedious, because his arguments are usually presented as magister dixit, just like the other developer did in the OP's question. Call that a personal business if you like.

    – Christian Hackl
    yesterday






  • 39





    Your coworker hasn't reduced the number of arguments needed to do the work, he's just hidden the arguments inside the class. This is just an awful practice all around.

    – 17 of 26
    yesterday






  • 21





    Wow, I think it's the first time I've ever seen "getter" methods not returning anything. As a bonus, it means they also have side-effects, which I'd expect getter methods not to have. It's surprising how bad the code is considering it supposedly comes from Clean Code advices.

    – Eric Duminil
    yesterday















61















The codebase I'm working on frequently uses instance variables to share data between various trivial methods. The original developer is adamant that this adheres to the best practices stated in the Clean Code book by Uncle Bob/Robert Martin: "The first rule of functions is that they should be small." and "The ideal number of arguments for a function is
zero (niladic). (...) Arguments are hard. They take a lot of conceptual power."



An example:



public class SomeBusinessProcess 
@Inject private Router router;
@Inject private ServiceClient serviceClient;
@Inject private CryptoService cryptoService;

private byte[] encodedData;
private EncryptionInfo encryptionInfo;
private EncryptedObject payloadOfResponse;
private URI destinationURI;

public EncryptedResponse process(EncryptedRequest encryptedRequest)
checkNotNull(encryptedRequest);

getEncodedData(encryptedRequest);
getEncryptionInfo();
getDestinationURI();
passRequestToServiceClient();

return cryptoService.encryptResponse(payloadOfResponse);


private void getEncodedData(EncryptedRequest encryptedRequest)
encodedData = cryptoService.decryptRequest(encryptedRequest, byte[].class);


private void getEncryptionInfo()
encryptionInfo = cryptoService.getEncryptionInfoForDefaultClient();


private void getDestinationURI()
destinationURI = router.getDestination().getUri();


private void passRequestToServiceClient()
payloadOfResponse = serviceClient.handle(destinationURI, encodedData, encryptionInfo);




I would refactor that into the following using local variables:



public class SomeBusinessProcess 
@Inject private Router router;
@Inject private ServiceClient serviceClient;
@Inject private CryptoService cryptoService;

public EncryptedResponse process(EncryptedRequest encryptedRequest)
checkNotNull(encryptedRequest);

byte[] encodedData = cryptoService.decryptRequest(encryptedRequest, byte[].class);
EncryptionInfo encryptionInfo = cryptoService.getEncryptionInfoForDefaultClient();
URI destinationURI = router.getDestination().getUri();
EncryptedObject payloadOfResponse = serviceClient.handle(destinationURI, encodedData,
encryptionInfo);

return cryptoService.encryptResponse(payloadOfResponse);




This is shorter, it eliminates the implicit data coupling between the various trivial methods and it limits the variable scopes to the minimum required. 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.



Hence my questions:
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?



And conversely, are there any downsides to this refactoring that I have possibly overlooked?










share|improve this question









New contributor




Alexander is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.















  • 148





    "The ideal number of arguments for a function is zero" is plain wrong. The ideal number of arguments is exactly the number needed to enable your function to be side-effect free. Less than that and you needlessly cause your functions to be impure thus forcing you to steer away from the pit of success and climb the gradient of pain. Sometimes "Uncle Bob" is spot on with his advice. Sometimes he is spectacularly wrong. His zero arguments advice is an example of the latter.

    – David Arno
    yesterday







  • 57





    Your colleague has fallen victim to Robert Martin's successful marketing. You will have a hard time convincing him that the teachings spread in that book really have a very narrow context and are not generally applicable to software engineering. To admit that would mean to give up an ideology. You are asking about scientific arguments in favour of your approach, but Robert Martin has never been about science. There is no critical, open exchange of viewpoints in his writings, it's always all about absolute rules and dogmas. You are facing a purely psychological problem here.

    – Christian Hackl
    yesterday






  • 20





    @R.Schmitz: The problem with Martin is his big influence, especially on junior developers. When you work a lot with others, then you have to deal with his ideas sooner or later, and that is becoming tedious, because his arguments are usually presented as magister dixit, just like the other developer did in the OP's question. Call that a personal business if you like.

    – Christian Hackl
    yesterday






  • 39





    Your coworker hasn't reduced the number of arguments needed to do the work, he's just hidden the arguments inside the class. This is just an awful practice all around.

    – 17 of 26
    yesterday






  • 21





    Wow, I think it's the first time I've ever seen "getter" methods not returning anything. As a bonus, it means they also have side-effects, which I'd expect getter methods not to have. It's surprising how bad the code is considering it supposedly comes from Clean Code advices.

    – Eric Duminil
    yesterday













61












61








61


9






The codebase I'm working on frequently uses instance variables to share data between various trivial methods. The original developer is adamant that this adheres to the best practices stated in the Clean Code book by Uncle Bob/Robert Martin: "The first rule of functions is that they should be small." and "The ideal number of arguments for a function is
zero (niladic). (...) Arguments are hard. They take a lot of conceptual power."



An example:



public class SomeBusinessProcess 
@Inject private Router router;
@Inject private ServiceClient serviceClient;
@Inject private CryptoService cryptoService;

private byte[] encodedData;
private EncryptionInfo encryptionInfo;
private EncryptedObject payloadOfResponse;
private URI destinationURI;

public EncryptedResponse process(EncryptedRequest encryptedRequest)
checkNotNull(encryptedRequest);

getEncodedData(encryptedRequest);
getEncryptionInfo();
getDestinationURI();
passRequestToServiceClient();

return cryptoService.encryptResponse(payloadOfResponse);


private void getEncodedData(EncryptedRequest encryptedRequest)
encodedData = cryptoService.decryptRequest(encryptedRequest, byte[].class);


private void getEncryptionInfo()
encryptionInfo = cryptoService.getEncryptionInfoForDefaultClient();


private void getDestinationURI()
destinationURI = router.getDestination().getUri();


private void passRequestToServiceClient()
payloadOfResponse = serviceClient.handle(destinationURI, encodedData, encryptionInfo);




I would refactor that into the following using local variables:



public class SomeBusinessProcess 
@Inject private Router router;
@Inject private ServiceClient serviceClient;
@Inject private CryptoService cryptoService;

public EncryptedResponse process(EncryptedRequest encryptedRequest)
checkNotNull(encryptedRequest);

byte[] encodedData = cryptoService.decryptRequest(encryptedRequest, byte[].class);
EncryptionInfo encryptionInfo = cryptoService.getEncryptionInfoForDefaultClient();
URI destinationURI = router.getDestination().getUri();
EncryptedObject payloadOfResponse = serviceClient.handle(destinationURI, encodedData,
encryptionInfo);

return cryptoService.encryptResponse(payloadOfResponse);




This is shorter, it eliminates the implicit data coupling between the various trivial methods and it limits the variable scopes to the minimum required. 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.



Hence my questions:
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?



And conversely, are there any downsides to this refactoring that I have possibly overlooked?










share|improve this question









New contributor




Alexander is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.












The codebase I'm working on frequently uses instance variables to share data between various trivial methods. The original developer is adamant that this adheres to the best practices stated in the Clean Code book by Uncle Bob/Robert Martin: "The first rule of functions is that they should be small." and "The ideal number of arguments for a function is
zero (niladic). (...) Arguments are hard. They take a lot of conceptual power."



An example:



public class SomeBusinessProcess 
@Inject private Router router;
@Inject private ServiceClient serviceClient;
@Inject private CryptoService cryptoService;

private byte[] encodedData;
private EncryptionInfo encryptionInfo;
private EncryptedObject payloadOfResponse;
private URI destinationURI;

public EncryptedResponse process(EncryptedRequest encryptedRequest)
checkNotNull(encryptedRequest);

getEncodedData(encryptedRequest);
getEncryptionInfo();
getDestinationURI();
passRequestToServiceClient();

return cryptoService.encryptResponse(payloadOfResponse);


private void getEncodedData(EncryptedRequest encryptedRequest)
encodedData = cryptoService.decryptRequest(encryptedRequest, byte[].class);


private void getEncryptionInfo()
encryptionInfo = cryptoService.getEncryptionInfoForDefaultClient();


private void getDestinationURI()
destinationURI = router.getDestination().getUri();


private void passRequestToServiceClient()
payloadOfResponse = serviceClient.handle(destinationURI, encodedData, encryptionInfo);




I would refactor that into the following using local variables:



public class SomeBusinessProcess 
@Inject private Router router;
@Inject private ServiceClient serviceClient;
@Inject private CryptoService cryptoService;

public EncryptedResponse process(EncryptedRequest encryptedRequest)
checkNotNull(encryptedRequest);

byte[] encodedData = cryptoService.decryptRequest(encryptedRequest, byte[].class);
EncryptionInfo encryptionInfo = cryptoService.getEncryptionInfoForDefaultClient();
URI destinationURI = router.getDestination().getUri();
EncryptedObject payloadOfResponse = serviceClient.handle(destinationURI, encodedData,
encryptionInfo);

return cryptoService.encryptResponse(payloadOfResponse);




This is shorter, it eliminates the implicit data coupling between the various trivial methods and it limits the variable scopes to the minimum required. 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.



Hence my questions:
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?



And conversely, are there any downsides to this refactoring that I have possibly overlooked?







java refactoring






share|improve this question









New contributor




Alexander is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.











share|improve this question









New contributor




Alexander is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.









share|improve this question




share|improve this question








edited yesterday







Alexander













New contributor




Alexander is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.









asked yesterday









AlexanderAlexander

40327




40327




New contributor




Alexander is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.





New contributor





Alexander is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.






Alexander is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.







  • 148





    "The ideal number of arguments for a function is zero" is plain wrong. The ideal number of arguments is exactly the number needed to enable your function to be side-effect free. Less than that and you needlessly cause your functions to be impure thus forcing you to steer away from the pit of success and climb the gradient of pain. Sometimes "Uncle Bob" is spot on with his advice. Sometimes he is spectacularly wrong. His zero arguments advice is an example of the latter.

    – David Arno
    yesterday







  • 57





    Your colleague has fallen victim to Robert Martin's successful marketing. You will have a hard time convincing him that the teachings spread in that book really have a very narrow context and are not generally applicable to software engineering. To admit that would mean to give up an ideology. You are asking about scientific arguments in favour of your approach, but Robert Martin has never been about science. There is no critical, open exchange of viewpoints in his writings, it's always all about absolute rules and dogmas. You are facing a purely psychological problem here.

    – Christian Hackl
    yesterday






  • 20





    @R.Schmitz: The problem with Martin is his big influence, especially on junior developers. When you work a lot with others, then you have to deal with his ideas sooner or later, and that is becoming tedious, because his arguments are usually presented as magister dixit, just like the other developer did in the OP's question. Call that a personal business if you like.

    – Christian Hackl
    yesterday






  • 39





    Your coworker hasn't reduced the number of arguments needed to do the work, he's just hidden the arguments inside the class. This is just an awful practice all around.

    – 17 of 26
    yesterday






  • 21





    Wow, I think it's the first time I've ever seen "getter" methods not returning anything. As a bonus, it means they also have side-effects, which I'd expect getter methods not to have. It's surprising how bad the code is considering it supposedly comes from Clean Code advices.

    – Eric Duminil
    yesterday












  • 148





    "The ideal number of arguments for a function is zero" is plain wrong. The ideal number of arguments is exactly the number needed to enable your function to be side-effect free. Less than that and you needlessly cause your functions to be impure thus forcing you to steer away from the pit of success and climb the gradient of pain. Sometimes "Uncle Bob" is spot on with his advice. Sometimes he is spectacularly wrong. His zero arguments advice is an example of the latter.

    – David Arno
    yesterday







  • 57





    Your colleague has fallen victim to Robert Martin's successful marketing. You will have a hard time convincing him that the teachings spread in that book really have a very narrow context and are not generally applicable to software engineering. To admit that would mean to give up an ideology. You are asking about scientific arguments in favour of your approach, but Robert Martin has never been about science. There is no critical, open exchange of viewpoints in his writings, it's always all about absolute rules and dogmas. You are facing a purely psychological problem here.

    – Christian Hackl
    yesterday






  • 20





    @R.Schmitz: The problem with Martin is his big influence, especially on junior developers. When you work a lot with others, then you have to deal with his ideas sooner or later, and that is becoming tedious, because his arguments are usually presented as magister dixit, just like the other developer did in the OP's question. Call that a personal business if you like.

    – Christian Hackl
    yesterday






  • 39





    Your coworker hasn't reduced the number of arguments needed to do the work, he's just hidden the arguments inside the class. This is just an awful practice all around.

    – 17 of 26
    yesterday






  • 21





    Wow, I think it's the first time I've ever seen "getter" methods not returning anything. As a bonus, it means they also have side-effects, which I'd expect getter methods not to have. It's surprising how bad the code is considering it supposedly comes from Clean Code advices.

    – Eric Duminil
    yesterday







148




148





"The ideal number of arguments for a function is zero" is plain wrong. The ideal number of arguments is exactly the number needed to enable your function to be side-effect free. Less than that and you needlessly cause your functions to be impure thus forcing you to steer away from the pit of success and climb the gradient of pain. Sometimes "Uncle Bob" is spot on with his advice. Sometimes he is spectacularly wrong. His zero arguments advice is an example of the latter.

– David Arno
yesterday






"The ideal number of arguments for a function is zero" is plain wrong. The ideal number of arguments is exactly the number needed to enable your function to be side-effect free. Less than that and you needlessly cause your functions to be impure thus forcing you to steer away from the pit of success and climb the gradient of pain. Sometimes "Uncle Bob" is spot on with his advice. Sometimes he is spectacularly wrong. His zero arguments advice is an example of the latter.

– David Arno
yesterday





57




57





Your colleague has fallen victim to Robert Martin's successful marketing. You will have a hard time convincing him that the teachings spread in that book really have a very narrow context and are not generally applicable to software engineering. To admit that would mean to give up an ideology. You are asking about scientific arguments in favour of your approach, but Robert Martin has never been about science. There is no critical, open exchange of viewpoints in his writings, it's always all about absolute rules and dogmas. You are facing a purely psychological problem here.

– Christian Hackl
yesterday





Your colleague has fallen victim to Robert Martin's successful marketing. You will have a hard time convincing him that the teachings spread in that book really have a very narrow context and are not generally applicable to software engineering. To admit that would mean to give up an ideology. You are asking about scientific arguments in favour of your approach, but Robert Martin has never been about science. There is no critical, open exchange of viewpoints in his writings, it's always all about absolute rules and dogmas. You are facing a purely psychological problem here.

– Christian Hackl
yesterday




20




20





@R.Schmitz: The problem with Martin is his big influence, especially on junior developers. When you work a lot with others, then you have to deal with his ideas sooner or later, and that is becoming tedious, because his arguments are usually presented as magister dixit, just like the other developer did in the OP's question. Call that a personal business if you like.

– Christian Hackl
yesterday





@R.Schmitz: The problem with Martin is his big influence, especially on junior developers. When you work a lot with others, then you have to deal with his ideas sooner or later, and that is becoming tedious, because his arguments are usually presented as magister dixit, just like the other developer did in the OP's question. Call that a personal business if you like.

– Christian Hackl
yesterday




39




39





Your coworker hasn't reduced the number of arguments needed to do the work, he's just hidden the arguments inside the class. This is just an awful practice all around.

– 17 of 26
yesterday





Your coworker hasn't reduced the number of arguments needed to do the work, he's just hidden the arguments inside the class. This is just an awful practice all around.

– 17 of 26
yesterday




21




21





Wow, I think it's the first time I've ever seen "getter" methods not returning anything. As a bonus, it means they also have side-effects, which I'd expect getter methods not to have. It's surprising how bad the code is considering it supposedly comes from Clean Code advices.

– Eric Duminil
yesterday





Wow, I think it's the first time I've ever seen "getter" methods not returning anything. As a bonus, it means they also have side-effects, which I'd expect getter methods not to have. It's surprising how bad the code is considering it supposedly comes from Clean Code advices.

– Eric Duminil
yesterday










12 Answers
12






active

oldest

votes


















114















What is the objective, scientific rationale to favor local variables over instance variables?




Scope isn't a binary state, it's a gradient. You can rank these from largest to smallest:



Global > Class > Local (method) > Local (code block, e.g. if, for, ...)


Edit: what I call a "class scope" is what you mean by "instance variable". To my knowledge, those are synonymous, but I'm a C# dev, not a Java dev. For the sake of brevity, I've lumped all statics into the global category since statics are not the topic of the question.



The smaller the scope, the better. The rationale is that variables should live in the smallest scope possible. There are many benefits to this:



  • It forces you to think about the current class' responsibility and helps you stick to SRP.

  • It enables you to not have to avoid global naming conflicts, e.g. if two or more classes have a Name property, you're not forced to prefix them like FooName, BarName, ... Thus keeping your variable names as clean and terse as possible.

  • It declutters the code by limiting the available variables (e.g. for Intellisense) to those that are contextually relevant.

  • It enables some form of access control so your data can't be manipulated by some actor you don't know about (e.g. a different class developed by a colleague).

  • It makes the code more readable as you ensure that the declaration of these variables tries to stay as close to the actual usage of these variables as is possible.

  • Wantonly declaring variables in an overly wide scope is often indicative of a developer who doesn't quite grasp OOP or how to implement it. Seeing overly widely scoped variables acts as a red flag that there's probably something going wrong with the OOP approach (either with the developer in general or the codebase in specific).

  • (Comment by Kevin) Using locals forces you to do things in the right order. In the original (class variable) code, you could wrongly move passRequestToServiceClient() to the top of the method, and it would still compile. With locals, you could only make that mistake if you passed an uninitialized variable, which is hopefully obvious enough that you don't actually do it.


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.



And conversely, are there any downsides to this refactoring that I have possibly overlooked?




The issue here is that your argument for local variables is valid, but you've also made additional changes which are not correct and cause your suggested fix to fail the smell test.



While I understand your "no class variable" suggestion and there's merit to it, you've actually also removed the methods themselves, and that's a whole different ballgame. The methods should have stayed, and instead you should've altered them to return their value rather than store it in a class variable:



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


private EncryptionInfo getEncryptionInfo()
return cryptoService.getEncryptionInfoForDefaultClient();


// and so on...


I do agree with what you've done in the process method, but you should've been calling the private submethods rather than executing their bodies directly.



public EncryptedResponse process(EncryptedRequest encryptedRequest) 
checkNotNull(encryptedRequest);

byte[] encodedData = getEncodedData();
EncryptionInfo encryptionInfo = getEncryptionInfo();

//and so on...

return cryptoService.encryptResponse(payloadOfResponse);



You'd want that extra layer of abstraction, especially when you run into methods that need to be reused several times. Even if you don't currently reuse your methods, it's still a matter of good practice to already create submethods where relevant, even if only to aid code readability.



Regardless of the local variable argument, I immediately noticed that your suggested fix is considerably less readable than the original. I do concede that wanton use of class variables also detracts from code readability, but not at first sight compared to you having stacked all the logic in a single (now long-winded) method.






share|improve this answer




















  • 46





    You're missing a key benefit: Using locals forces you to do things in the right order. In the original (class variable) code, you could wrongly move passRequestToServiceClient() to the top of the method, and it would still compile. With locals, you could only make that mistake if you passed an uninitialized variable, which is hopefully obvious enough that you don't actually do it.

    – Kevin
    yesterday






  • 2





    @Kevin: Good point. To be fair (and maybe a bit pedantic), that's more a matter of compile-time errors over runtime errors. You'd expect that the developer who wrote the class-variable-code to test their runtime and work at it until the runtime does what it needs to. Even bad practice coders still tend to have ways of coping with their (unknowingly bad) practice. But I'll add your point to the list.

    – Flater
    yesterday







  • 2





    To be nitty, I think the first method needs a parameter private byte[] getEncodedData(EncryptedRequest encryptedRequest) (in the original code, I would expect the example to contain a member variable encryptedRequest).

    – Doc Brown
    yesterday







  • 3





    One additional benefit of the smallest scope is that it helps prevent resource leaks.

    – chrylis
    yesterday






  • 3





    I like most of this answer, but I disagree that @Alexander was wrong to eliminate the private submethods. We have no idea what the business requirements are behind this contrived example, but we can't assume that any inheritance will ever be needed. Based on the en.wikipedia.org/wiki/You_aren%27t_gonna_need_it principle, I would say it's overengineering to try to break apart literally 4 lines of code into more methods. It just makes the code harder to read.

    – Jordan Rieger
    yesterday



















41














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.






share|improve this answer








New contributor




Alex is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.















  • 12





    Totally agree! Those member variables are simply implicit function arguments. In fact it's worse since now there is no explicit link between the value of those variable and the function usage (from an external POV)

    – Rémi
    yesterday






  • 1





    I would say this is not what the book meant. How many functions require exactly zero input data to run? This one of the bits I think the book had wrong.

    – Qwertie
    yesterday


















22














"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).






share|improve this answer




















  • 11





    Thinking for yourself is of course good. But your opening paragraph effectively includes learners or juniors dismissing the input of teachers or seniors; which goes too far.

    – Flater
    yesterday






  • 6





    @Flater After thinking about the input of teachers or seniors, and seeing they are wrong, dismissing their input is the only right thing. In the end, it is not about dismissing, but about questioning it and only dismissing it if it definitely proves wrong.

    – glglgl
    yesterday











  • @Flater: A good teacher or senior should encourage such thinking. "Here are my teachings, make them better."

    – Christian Hackl
    yesterday







  • 7





    @ChristianHackl: I'm fully on board with not blindly following traditionalism, but I wouldn't blindly dismiss it either. The answer seemingly suggests to eschew acquired knowledge in favor of your own view, and that's not a healthy approach to take. Blindly following others' opinions, no. Questioning something when you disagree, obviously yes. Outright dismissing it because you disagree, no. As I read it, the first paragraph of the answer at least seems to suggest the latter. It very much depends on what gnasher means with "think for yourself", which needs elaboration.

    – Flater
    yesterday







  • 5





    On a sharing wisdom platform, this seems a little out of place...

    – drjpizzle
    yesterday


















13














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.






share|improve this answer
































    9














    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.






    share|improve this answer




















    • 2





      Eliminating all variables doesn't necessarily make code easier to read.

      – Pharap
      18 hours ago











    • Eliminate all variables completely with pointfree style en.wikipedia.org/wiki/Tacit_programming

      – Marcin
      7 hours ago











    • @Pharap True, a lack of variables does not ensure legibility. In some cases it even makes debugging more difficult. The point is that well chosen names, a clear usage of expression, can communicate an idea very clearly, while still being efficient.

      – Kain0_0
      5 hours ago



















    6















    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.






    share|improve this answer


















    • 2





      So far, this is the only answer that mentions thread-safety and concurrency. That's kind of amazing given the specific code example in the question: an instance of SomeBusinessProcess cannot safely process multiple encrypted requests at once. The method public EncryptedResponse process(EncryptedRequest encryptedRequest) is not synchronized, and concurrent calls would quite possibly clobber the values of the instance variables. This is a good point to bring up.

      – Joshua Taylor
      yesterday


















    5














    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.






    share|improve this answer

























    • My upvote, though many will hesitate here. Of course some abstractions, make sense. (BTW a method called "process" is terrible.) But here the logic is minimal. The OP's question however is on an entire code style, and there the case may be more complex.

      – Joop Eggen
      yesterday











    • One clear issue with chaining it all into one method call is sheer readability. It also doesn't work if you need more than one operation on a given object. Also, this is nigh impossible to debug because you can't step through the operations and inspect the objects. While this works on a technical level, I would not advocate for this as it massively neglects non-runtime aspects of software development.

      – Flater
      yesterday












    • @Flater I agree with your comments, we don't want to apply this everywhere. I edited my answer to clarify my practical position. What I want to show is that in practice we only apply rules when it's appropriate. Chaining method call is fine in this instance, and if I need to debug, I'd invoke the tests for the chained methods.

      – imel96
      yesterday











    • @JoopEggen Yes, abstractions make sense. In the example, the private methods don't give any abstractions anyway, users of the class don't even know about them

      – imel96
      yesterday


















    4














    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()





    share|improve this answer










    New contributor




    user673679 is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
    Check out our Code of Conduct.



























      1














      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.






      share|improve this answer








      New contributor




      kap is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
      Check out our Code of Conduct.



























        1














        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.






        share|improve this answer






























          0














          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);







          share|improve this answer








          New contributor




          Roman Weis is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
          Check out our Code of Conduct.



























            -3














            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.






            share|improve this answer




















            • 4





              So what's the point of your answer, exactly?

              – Eric Duminil
              yesterday











            • To provide a different perspective/wording of the potential problem at hand when preferring shared state over local state. Next.

              – luis.espinal
              yesterday






            • 2





              It should be a comment IMHO. Let's wait for the community to decide if your answer is helpful.

              – Eric Duminil
              yesterday











            • Perhaps so, and perhaps then you should have let the community decide first. ¯_(ツ)_/¯. Anyways/whatevs, the community can choose as they please. #shrugs

              – luis.espinal
              yesterday









            protected by gnat 21 hours ago



            Thank you for your interest in this question.
            Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).



            Would you like to answer one of these unanswered questions instead?














            12 Answers
            12






            active

            oldest

            votes








            12 Answers
            12






            active

            oldest

            votes









            active

            oldest

            votes






            active

            oldest

            votes









            114















            What is the objective, scientific rationale to favor local variables over instance variables?




            Scope isn't a binary state, it's a gradient. You can rank these from largest to smallest:



            Global > Class > Local (method) > Local (code block, e.g. if, for, ...)


            Edit: what I call a "class scope" is what you mean by "instance variable". To my knowledge, those are synonymous, but I'm a C# dev, not a Java dev. For the sake of brevity, I've lumped all statics into the global category since statics are not the topic of the question.



            The smaller the scope, the better. The rationale is that variables should live in the smallest scope possible. There are many benefits to this:



            • It forces you to think about the current class' responsibility and helps you stick to SRP.

            • It enables you to not have to avoid global naming conflicts, e.g. if two or more classes have a Name property, you're not forced to prefix them like FooName, BarName, ... Thus keeping your variable names as clean and terse as possible.

            • It declutters the code by limiting the available variables (e.g. for Intellisense) to those that are contextually relevant.

            • It enables some form of access control so your data can't be manipulated by some actor you don't know about (e.g. a different class developed by a colleague).

            • It makes the code more readable as you ensure that the declaration of these variables tries to stay as close to the actual usage of these variables as is possible.

            • Wantonly declaring variables in an overly wide scope is often indicative of a developer who doesn't quite grasp OOP or how to implement it. Seeing overly widely scoped variables acts as a red flag that there's probably something going wrong with the OOP approach (either with the developer in general or the codebase in specific).

            • (Comment by Kevin) Using locals forces you to do things in the right order. In the original (class variable) code, you could wrongly move passRequestToServiceClient() to the top of the method, and it would still compile. With locals, you could only make that mistake if you passed an uninitialized variable, which is hopefully obvious enough that you don't actually do it.


            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.



            And conversely, are there any downsides to this refactoring that I have possibly overlooked?




            The issue here is that your argument for local variables is valid, but you've also made additional changes which are not correct and cause your suggested fix to fail the smell test.



            While I understand your "no class variable" suggestion and there's merit to it, you've actually also removed the methods themselves, and that's a whole different ballgame. The methods should have stayed, and instead you should've altered them to return their value rather than store it in a class variable:



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


            private EncryptionInfo getEncryptionInfo()
            return cryptoService.getEncryptionInfoForDefaultClient();


            // and so on...


            I do agree with what you've done in the process method, but you should've been calling the private submethods rather than executing their bodies directly.



            public EncryptedResponse process(EncryptedRequest encryptedRequest) 
            checkNotNull(encryptedRequest);

            byte[] encodedData = getEncodedData();
            EncryptionInfo encryptionInfo = getEncryptionInfo();

            //and so on...

            return cryptoService.encryptResponse(payloadOfResponse);



            You'd want that extra layer of abstraction, especially when you run into methods that need to be reused several times. Even if you don't currently reuse your methods, it's still a matter of good practice to already create submethods where relevant, even if only to aid code readability.



            Regardless of the local variable argument, I immediately noticed that your suggested fix is considerably less readable than the original. I do concede that wanton use of class variables also detracts from code readability, but not at first sight compared to you having stacked all the logic in a single (now long-winded) method.






            share|improve this answer




















            • 46





              You're missing a key benefit: Using locals forces you to do things in the right order. In the original (class variable) code, you could wrongly move passRequestToServiceClient() to the top of the method, and it would still compile. With locals, you could only make that mistake if you passed an uninitialized variable, which is hopefully obvious enough that you don't actually do it.

              – Kevin
              yesterday






            • 2





              @Kevin: Good point. To be fair (and maybe a bit pedantic), that's more a matter of compile-time errors over runtime errors. You'd expect that the developer who wrote the class-variable-code to test their runtime and work at it until the runtime does what it needs to. Even bad practice coders still tend to have ways of coping with their (unknowingly bad) practice. But I'll add your point to the list.

              – Flater
              yesterday







            • 2





              To be nitty, I think the first method needs a parameter private byte[] getEncodedData(EncryptedRequest encryptedRequest) (in the original code, I would expect the example to contain a member variable encryptedRequest).

              – Doc Brown
              yesterday







            • 3





              One additional benefit of the smallest scope is that it helps prevent resource leaks.

              – chrylis
              yesterday






            • 3





              I like most of this answer, but I disagree that @Alexander was wrong to eliminate the private submethods. We have no idea what the business requirements are behind this contrived example, but we can't assume that any inheritance will ever be needed. Based on the en.wikipedia.org/wiki/You_aren%27t_gonna_need_it principle, I would say it's overengineering to try to break apart literally 4 lines of code into more methods. It just makes the code harder to read.

              – Jordan Rieger
              yesterday
















            114















            What is the objective, scientific rationale to favor local variables over instance variables?




            Scope isn't a binary state, it's a gradient. You can rank these from largest to smallest:



            Global > Class > Local (method) > Local (code block, e.g. if, for, ...)


            Edit: what I call a "class scope" is what you mean by "instance variable". To my knowledge, those are synonymous, but I'm a C# dev, not a Java dev. For the sake of brevity, I've lumped all statics into the global category since statics are not the topic of the question.



            The smaller the scope, the better. The rationale is that variables should live in the smallest scope possible. There are many benefits to this:



            • It forces you to think about the current class' responsibility and helps you stick to SRP.

            • It enables you to not have to avoid global naming conflicts, e.g. if two or more classes have a Name property, you're not forced to prefix them like FooName, BarName, ... Thus keeping your variable names as clean and terse as possible.

            • It declutters the code by limiting the available variables (e.g. for Intellisense) to those that are contextually relevant.

            • It enables some form of access control so your data can't be manipulated by some actor you don't know about (e.g. a different class developed by a colleague).

            • It makes the code more readable as you ensure that the declaration of these variables tries to stay as close to the actual usage of these variables as is possible.

            • Wantonly declaring variables in an overly wide scope is often indicative of a developer who doesn't quite grasp OOP or how to implement it. Seeing overly widely scoped variables acts as a red flag that there's probably something going wrong with the OOP approach (either with the developer in general or the codebase in specific).

            • (Comment by Kevin) Using locals forces you to do things in the right order. In the original (class variable) code, you could wrongly move passRequestToServiceClient() to the top of the method, and it would still compile. With locals, you could only make that mistake if you passed an uninitialized variable, which is hopefully obvious enough that you don't actually do it.


            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.



            And conversely, are there any downsides to this refactoring that I have possibly overlooked?




            The issue here is that your argument for local variables is valid, but you've also made additional changes which are not correct and cause your suggested fix to fail the smell test.



            While I understand your "no class variable" suggestion and there's merit to it, you've actually also removed the methods themselves, and that's a whole different ballgame. The methods should have stayed, and instead you should've altered them to return their value rather than store it in a class variable:



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


            private EncryptionInfo getEncryptionInfo()
            return cryptoService.getEncryptionInfoForDefaultClient();


            // and so on...


            I do agree with what you've done in the process method, but you should've been calling the private submethods rather than executing their bodies directly.



            public EncryptedResponse process(EncryptedRequest encryptedRequest) 
            checkNotNull(encryptedRequest);

            byte[] encodedData = getEncodedData();
            EncryptionInfo encryptionInfo = getEncryptionInfo();

            //and so on...

            return cryptoService.encryptResponse(payloadOfResponse);



            You'd want that extra layer of abstraction, especially when you run into methods that need to be reused several times. Even if you don't currently reuse your methods, it's still a matter of good practice to already create submethods where relevant, even if only to aid code readability.



            Regardless of the local variable argument, I immediately noticed that your suggested fix is considerably less readable than the original. I do concede that wanton use of class variables also detracts from code readability, but not at first sight compared to you having stacked all the logic in a single (now long-winded) method.






            share|improve this answer




















            • 46





              You're missing a key benefit: Using locals forces you to do things in the right order. In the original (class variable) code, you could wrongly move passRequestToServiceClient() to the top of the method, and it would still compile. With locals, you could only make that mistake if you passed an uninitialized variable, which is hopefully obvious enough that you don't actually do it.

              – Kevin
              yesterday






            • 2





              @Kevin: Good point. To be fair (and maybe a bit pedantic), that's more a matter of compile-time errors over runtime errors. You'd expect that the developer who wrote the class-variable-code to test their runtime and work at it until the runtime does what it needs to. Even bad practice coders still tend to have ways of coping with their (unknowingly bad) practice. But I'll add your point to the list.

              – Flater
              yesterday







            • 2





              To be nitty, I think the first method needs a parameter private byte[] getEncodedData(EncryptedRequest encryptedRequest) (in the original code, I would expect the example to contain a member variable encryptedRequest).

              – Doc Brown
              yesterday







            • 3





              One additional benefit of the smallest scope is that it helps prevent resource leaks.

              – chrylis
              yesterday






            • 3





              I like most of this answer, but I disagree that @Alexander was wrong to eliminate the private submethods. We have no idea what the business requirements are behind this contrived example, but we can't assume that any inheritance will ever be needed. Based on the en.wikipedia.org/wiki/You_aren%27t_gonna_need_it principle, I would say it's overengineering to try to break apart literally 4 lines of code into more methods. It just makes the code harder to read.

              – Jordan Rieger
              yesterday














            114












            114








            114








            What is the objective, scientific rationale to favor local variables over instance variables?




            Scope isn't a binary state, it's a gradient. You can rank these from largest to smallest:



            Global > Class > Local (method) > Local (code block, e.g. if, for, ...)


            Edit: what I call a "class scope" is what you mean by "instance variable". To my knowledge, those are synonymous, but I'm a C# dev, not a Java dev. For the sake of brevity, I've lumped all statics into the global category since statics are not the topic of the question.



            The smaller the scope, the better. The rationale is that variables should live in the smallest scope possible. There are many benefits to this:



            • It forces you to think about the current class' responsibility and helps you stick to SRP.

            • It enables you to not have to avoid global naming conflicts, e.g. if two or more classes have a Name property, you're not forced to prefix them like FooName, BarName, ... Thus keeping your variable names as clean and terse as possible.

            • It declutters the code by limiting the available variables (e.g. for Intellisense) to those that are contextually relevant.

            • It enables some form of access control so your data can't be manipulated by some actor you don't know about (e.g. a different class developed by a colleague).

            • It makes the code more readable as you ensure that the declaration of these variables tries to stay as close to the actual usage of these variables as is possible.

            • Wantonly declaring variables in an overly wide scope is often indicative of a developer who doesn't quite grasp OOP or how to implement it. Seeing overly widely scoped variables acts as a red flag that there's probably something going wrong with the OOP approach (either with the developer in general or the codebase in specific).

            • (Comment by Kevin) Using locals forces you to do things in the right order. In the original (class variable) code, you could wrongly move passRequestToServiceClient() to the top of the method, and it would still compile. With locals, you could only make that mistake if you passed an uninitialized variable, which is hopefully obvious enough that you don't actually do it.


            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.



            And conversely, are there any downsides to this refactoring that I have possibly overlooked?




            The issue here is that your argument for local variables is valid, but you've also made additional changes which are not correct and cause your suggested fix to fail the smell test.



            While I understand your "no class variable" suggestion and there's merit to it, you've actually also removed the methods themselves, and that's a whole different ballgame. The methods should have stayed, and instead you should've altered them to return their value rather than store it in a class variable:



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


            private EncryptionInfo getEncryptionInfo()
            return cryptoService.getEncryptionInfoForDefaultClient();


            // and so on...


            I do agree with what you've done in the process method, but you should've been calling the private submethods rather than executing their bodies directly.



            public EncryptedResponse process(EncryptedRequest encryptedRequest) 
            checkNotNull(encryptedRequest);

            byte[] encodedData = getEncodedData();
            EncryptionInfo encryptionInfo = getEncryptionInfo();

            //and so on...

            return cryptoService.encryptResponse(payloadOfResponse);



            You'd want that extra layer of abstraction, especially when you run into methods that need to be reused several times. Even if you don't currently reuse your methods, it's still a matter of good practice to already create submethods where relevant, even if only to aid code readability.



            Regardless of the local variable argument, I immediately noticed that your suggested fix is considerably less readable than the original. I do concede that wanton use of class variables also detracts from code readability, but not at first sight compared to you having stacked all the logic in a single (now long-winded) method.






            share|improve this answer
















            What is the objective, scientific rationale to favor local variables over instance variables?




            Scope isn't a binary state, it's a gradient. You can rank these from largest to smallest:



            Global > Class > Local (method) > Local (code block, e.g. if, for, ...)


            Edit: what I call a "class scope" is what you mean by "instance variable". To my knowledge, those are synonymous, but I'm a C# dev, not a Java dev. For the sake of brevity, I've lumped all statics into the global category since statics are not the topic of the question.



            The smaller the scope, the better. The rationale is that variables should live in the smallest scope possible. There are many benefits to this:



            • It forces you to think about the current class' responsibility and helps you stick to SRP.

            • It enables you to not have to avoid global naming conflicts, e.g. if two or more classes have a Name property, you're not forced to prefix them like FooName, BarName, ... Thus keeping your variable names as clean and terse as possible.

            • It declutters the code by limiting the available variables (e.g. for Intellisense) to those that are contextually relevant.

            • It enables some form of access control so your data can't be manipulated by some actor you don't know about (e.g. a different class developed by a colleague).

            • It makes the code more readable as you ensure that the declaration of these variables tries to stay as close to the actual usage of these variables as is possible.

            • Wantonly declaring variables in an overly wide scope is often indicative of a developer who doesn't quite grasp OOP or how to implement it. Seeing overly widely scoped variables acts as a red flag that there's probably something going wrong with the OOP approach (either with the developer in general or the codebase in specific).

            • (Comment by Kevin) Using locals forces you to do things in the right order. In the original (class variable) code, you could wrongly move passRequestToServiceClient() to the top of the method, and it would still compile. With locals, you could only make that mistake if you passed an uninitialized variable, which is hopefully obvious enough that you don't actually do it.


            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.



            And conversely, are there any downsides to this refactoring that I have possibly overlooked?




            The issue here is that your argument for local variables is valid, but you've also made additional changes which are not correct and cause your suggested fix to fail the smell test.



            While I understand your "no class variable" suggestion and there's merit to it, you've actually also removed the methods themselves, and that's a whole different ballgame. The methods should have stayed, and instead you should've altered them to return their value rather than store it in a class variable:



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


            private EncryptionInfo getEncryptionInfo()
            return cryptoService.getEncryptionInfoForDefaultClient();


            // and so on...


            I do agree with what you've done in the process method, but you should've been calling the private submethods rather than executing their bodies directly.



            public EncryptedResponse process(EncryptedRequest encryptedRequest) 
            checkNotNull(encryptedRequest);

            byte[] encodedData = getEncodedData();
            EncryptionInfo encryptionInfo = getEncryptionInfo();

            //and so on...

            return cryptoService.encryptResponse(payloadOfResponse);



            You'd want that extra layer of abstraction, especially when you run into methods that need to be reused several times. Even if you don't currently reuse your methods, it's still a matter of good practice to already create submethods where relevant, even if only to aid code readability.



            Regardless of the local variable argument, I immediately noticed that your suggested fix is considerably less readable than the original. I do concede that wanton use of class variables also detracts from code readability, but not at first sight compared to you having stacked all the logic in a single (now long-winded) method.







            share|improve this answer














            share|improve this answer



            share|improve this answer








            edited 10 hours ago

























            answered yesterday









            FlaterFlater

            7,96031524




            7,96031524







            • 46





              You're missing a key benefit: Using locals forces you to do things in the right order. In the original (class variable) code, you could wrongly move passRequestToServiceClient() to the top of the method, and it would still compile. With locals, you could only make that mistake if you passed an uninitialized variable, which is hopefully obvious enough that you don't actually do it.

              – Kevin
              yesterday






            • 2





              @Kevin: Good point. To be fair (and maybe a bit pedantic), that's more a matter of compile-time errors over runtime errors. You'd expect that the developer who wrote the class-variable-code to test their runtime and work at it until the runtime does what it needs to. Even bad practice coders still tend to have ways of coping with their (unknowingly bad) practice. But I'll add your point to the list.

              – Flater
              yesterday







            • 2





              To be nitty, I think the first method needs a parameter private byte[] getEncodedData(EncryptedRequest encryptedRequest) (in the original code, I would expect the example to contain a member variable encryptedRequest).

              – Doc Brown
              yesterday







            • 3





              One additional benefit of the smallest scope is that it helps prevent resource leaks.

              – chrylis
              yesterday






            • 3





              I like most of this answer, but I disagree that @Alexander was wrong to eliminate the private submethods. We have no idea what the business requirements are behind this contrived example, but we can't assume that any inheritance will ever be needed. Based on the en.wikipedia.org/wiki/You_aren%27t_gonna_need_it principle, I would say it's overengineering to try to break apart literally 4 lines of code into more methods. It just makes the code harder to read.

              – Jordan Rieger
              yesterday













            • 46





              You're missing a key benefit: Using locals forces you to do things in the right order. In the original (class variable) code, you could wrongly move passRequestToServiceClient() to the top of the method, and it would still compile. With locals, you could only make that mistake if you passed an uninitialized variable, which is hopefully obvious enough that you don't actually do it.

              – Kevin
              yesterday






            • 2





              @Kevin: Good point. To be fair (and maybe a bit pedantic), that's more a matter of compile-time errors over runtime errors. You'd expect that the developer who wrote the class-variable-code to test their runtime and work at it until the runtime does what it needs to. Even bad practice coders still tend to have ways of coping with their (unknowingly bad) practice. But I'll add your point to the list.

              – Flater
              yesterday







            • 2





              To be nitty, I think the first method needs a parameter private byte[] getEncodedData(EncryptedRequest encryptedRequest) (in the original code, I would expect the example to contain a member variable encryptedRequest).

              – Doc Brown
              yesterday







            • 3





              One additional benefit of the smallest scope is that it helps prevent resource leaks.

              – chrylis
              yesterday






            • 3





              I like most of this answer, but I disagree that @Alexander was wrong to eliminate the private submethods. We have no idea what the business requirements are behind this contrived example, but we can't assume that any inheritance will ever be needed. Based on the en.wikipedia.org/wiki/You_aren%27t_gonna_need_it principle, I would say it's overengineering to try to break apart literally 4 lines of code into more methods. It just makes the code harder to read.

              – Jordan Rieger
              yesterday








            46




            46





            You're missing a key benefit: Using locals forces you to do things in the right order. In the original (class variable) code, you could wrongly move passRequestToServiceClient() to the top of the method, and it would still compile. With locals, you could only make that mistake if you passed an uninitialized variable, which is hopefully obvious enough that you don't actually do it.

            – Kevin
            yesterday





            You're missing a key benefit: Using locals forces you to do things in the right order. In the original (class variable) code, you could wrongly move passRequestToServiceClient() to the top of the method, and it would still compile. With locals, you could only make that mistake if you passed an uninitialized variable, which is hopefully obvious enough that you don't actually do it.

            – Kevin
            yesterday




            2




            2





            @Kevin: Good point. To be fair (and maybe a bit pedantic), that's more a matter of compile-time errors over runtime errors. You'd expect that the developer who wrote the class-variable-code to test their runtime and work at it until the runtime does what it needs to. Even bad practice coders still tend to have ways of coping with their (unknowingly bad) practice. But I'll add your point to the list.

            – Flater
            yesterday






            @Kevin: Good point. To be fair (and maybe a bit pedantic), that's more a matter of compile-time errors over runtime errors. You'd expect that the developer who wrote the class-variable-code to test their runtime and work at it until the runtime does what it needs to. Even bad practice coders still tend to have ways of coping with their (unknowingly bad) practice. But I'll add your point to the list.

            – Flater
            yesterday





            2




            2





            To be nitty, I think the first method needs a parameter private byte[] getEncodedData(EncryptedRequest encryptedRequest) (in the original code, I would expect the example to contain a member variable encryptedRequest).

            – Doc Brown
            yesterday






            To be nitty, I think the first method needs a parameter private byte[] getEncodedData(EncryptedRequest encryptedRequest) (in the original code, I would expect the example to contain a member variable encryptedRequest).

            – Doc Brown
            yesterday





            3




            3





            One additional benefit of the smallest scope is that it helps prevent resource leaks.

            – chrylis
            yesterday





            One additional benefit of the smallest scope is that it helps prevent resource leaks.

            – chrylis
            yesterday




            3




            3





            I like most of this answer, but I disagree that @Alexander was wrong to eliminate the private submethods. We have no idea what the business requirements are behind this contrived example, but we can't assume that any inheritance will ever be needed. Based on the en.wikipedia.org/wiki/You_aren%27t_gonna_need_it principle, I would say it's overengineering to try to break apart literally 4 lines of code into more methods. It just makes the code harder to read.

            – Jordan Rieger
            yesterday






            I like most of this answer, but I disagree that @Alexander was wrong to eliminate the private submethods. We have no idea what the business requirements are behind this contrived example, but we can't assume that any inheritance will ever be needed. Based on the en.wikipedia.org/wiki/You_aren%27t_gonna_need_it principle, I would say it's overengineering to try to break apart literally 4 lines of code into more methods. It just makes the code harder to read.

            – Jordan Rieger
            yesterday














            41














            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.






            share|improve this answer








            New contributor




            Alex is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
            Check out our Code of Conduct.















            • 12





              Totally agree! Those member variables are simply implicit function arguments. In fact it's worse since now there is no explicit link between the value of those variable and the function usage (from an external POV)

              – Rémi
              yesterday






            • 1





              I would say this is not what the book meant. How many functions require exactly zero input data to run? This one of the bits I think the book had wrong.

              – Qwertie
              yesterday















            41














            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.






            share|improve this answer








            New contributor




            Alex is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
            Check out our Code of Conduct.















            • 12





              Totally agree! Those member variables are simply implicit function arguments. In fact it's worse since now there is no explicit link between the value of those variable and the function usage (from an external POV)

              – Rémi
              yesterday






            • 1





              I would say this is not what the book meant. How many functions require exactly zero input data to run? This one of the bits I think the book had wrong.

              – Qwertie
              yesterday













            41












            41








            41







            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.






            share|improve this answer








            New contributor




            Alex is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
            Check out our Code of Conduct.










            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.







            share|improve this answer








            New contributor




            Alex is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
            Check out our Code of Conduct.









            share|improve this answer



            share|improve this answer






            New contributor




            Alex is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
            Check out our Code of Conduct.









            answered yesterday









            AlexAlex

            48124




            48124




            New contributor




            Alex is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
            Check out our Code of Conduct.





            New contributor





            Alex is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
            Check out our Code of Conduct.






            Alex is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
            Check out our Code of Conduct.







            • 12





              Totally agree! Those member variables are simply implicit function arguments. In fact it's worse since now there is no explicit link between the value of those variable and the function usage (from an external POV)

              – Rémi
              yesterday






            • 1





              I would say this is not what the book meant. How many functions require exactly zero input data to run? This one of the bits I think the book had wrong.

              – Qwertie
              yesterday












            • 12





              Totally agree! Those member variables are simply implicit function arguments. In fact it's worse since now there is no explicit link between the value of those variable and the function usage (from an external POV)

              – Rémi
              yesterday






            • 1





              I would say this is not what the book meant. How many functions require exactly zero input data to run? This one of the bits I think the book had wrong.

              – Qwertie
              yesterday







            12




            12





            Totally agree! Those member variables are simply implicit function arguments. In fact it's worse since now there is no explicit link between the value of those variable and the function usage (from an external POV)

            – Rémi
            yesterday





            Totally agree! Those member variables are simply implicit function arguments. In fact it's worse since now there is no explicit link between the value of those variable and the function usage (from an external POV)

            – Rémi
            yesterday




            1




            1





            I would say this is not what the book meant. How many functions require exactly zero input data to run? This one of the bits I think the book had wrong.

            – Qwertie
            yesterday





            I would say this is not what the book meant. How many functions require exactly zero input data to run? This one of the bits I think the book had wrong.

            – Qwertie
            yesterday











            22














            "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).






            share|improve this answer




















            • 11





              Thinking for yourself is of course good. But your opening paragraph effectively includes learners or juniors dismissing the input of teachers or seniors; which goes too far.

              – Flater
              yesterday






            • 6





              @Flater After thinking about the input of teachers or seniors, and seeing they are wrong, dismissing their input is the only right thing. In the end, it is not about dismissing, but about questioning it and only dismissing it if it definitely proves wrong.

              – glglgl
              yesterday











            • @Flater: A good teacher or senior should encourage such thinking. "Here are my teachings, make them better."

              – Christian Hackl
              yesterday







            • 7





              @ChristianHackl: I'm fully on board with not blindly following traditionalism, but I wouldn't blindly dismiss it either. The answer seemingly suggests to eschew acquired knowledge in favor of your own view, and that's not a healthy approach to take. Blindly following others' opinions, no. Questioning something when you disagree, obviously yes. Outright dismissing it because you disagree, no. As I read it, the first paragraph of the answer at least seems to suggest the latter. It very much depends on what gnasher means with "think for yourself", which needs elaboration.

              – Flater
              yesterday







            • 5





              On a sharing wisdom platform, this seems a little out of place...

              – drjpizzle
              yesterday















            22














            "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).






            share|improve this answer




















            • 11





              Thinking for yourself is of course good. But your opening paragraph effectively includes learners or juniors dismissing the input of teachers or seniors; which goes too far.

              – Flater
              yesterday






            • 6





              @Flater After thinking about the input of teachers or seniors, and seeing they are wrong, dismissing their input is the only right thing. In the end, it is not about dismissing, but about questioning it and only dismissing it if it definitely proves wrong.

              – glglgl
              yesterday











            • @Flater: A good teacher or senior should encourage such thinking. "Here are my teachings, make them better."

              – Christian Hackl
              yesterday







            • 7





              @ChristianHackl: I'm fully on board with not blindly following traditionalism, but I wouldn't blindly dismiss it either. The answer seemingly suggests to eschew acquired knowledge in favor of your own view, and that's not a healthy approach to take. Blindly following others' opinions, no. Questioning something when you disagree, obviously yes. Outright dismissing it because you disagree, no. As I read it, the first paragraph of the answer at least seems to suggest the latter. It very much depends on what gnasher means with "think for yourself", which needs elaboration.

              – Flater
              yesterday







            • 5





              On a sharing wisdom platform, this seems a little out of place...

              – drjpizzle
              yesterday













            22












            22








            22







            "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).






            share|improve this answer















            "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).







            share|improve this answer














            share|improve this answer



            share|improve this answer








            edited yesterday

























            answered yesterday









            gnasher729gnasher729

            20.7k22562




            20.7k22562







            • 11





              Thinking for yourself is of course good. But your opening paragraph effectively includes learners or juniors dismissing the input of teachers or seniors; which goes too far.

              – Flater
              yesterday






            • 6





              @Flater After thinking about the input of teachers or seniors, and seeing they are wrong, dismissing their input is the only right thing. In the end, it is not about dismissing, but about questioning it and only dismissing it if it definitely proves wrong.

              – glglgl
              yesterday











            • @Flater: A good teacher or senior should encourage such thinking. "Here are my teachings, make them better."

              – Christian Hackl
              yesterday







            • 7





              @ChristianHackl: I'm fully on board with not blindly following traditionalism, but I wouldn't blindly dismiss it either. The answer seemingly suggests to eschew acquired knowledge in favor of your own view, and that's not a healthy approach to take. Blindly following others' opinions, no. Questioning something when you disagree, obviously yes. Outright dismissing it because you disagree, no. As I read it, the first paragraph of the answer at least seems to suggest the latter. It very much depends on what gnasher means with "think for yourself", which needs elaboration.

              – Flater
              yesterday







            • 5





              On a sharing wisdom platform, this seems a little out of place...

              – drjpizzle
              yesterday












            • 11





              Thinking for yourself is of course good. But your opening paragraph effectively includes learners or juniors dismissing the input of teachers or seniors; which goes too far.

              – Flater
              yesterday






            • 6





              @Flater After thinking about the input of teachers or seniors, and seeing they are wrong, dismissing their input is the only right thing. In the end, it is not about dismissing, but about questioning it and only dismissing it if it definitely proves wrong.

              – glglgl
              yesterday











            • @Flater: A good teacher or senior should encourage such thinking. "Here are my teachings, make them better."

              – Christian Hackl
              yesterday







            • 7





              @ChristianHackl: I'm fully on board with not blindly following traditionalism, but I wouldn't blindly dismiss it either. The answer seemingly suggests to eschew acquired knowledge in favor of your own view, and that's not a healthy approach to take. Blindly following others' opinions, no. Questioning something when you disagree, obviously yes. Outright dismissing it because you disagree, no. As I read it, the first paragraph of the answer at least seems to suggest the latter. It very much depends on what gnasher means with "think for yourself", which needs elaboration.

              – Flater
              yesterday







            • 5





              On a sharing wisdom platform, this seems a little out of place...

              – drjpizzle
              yesterday







            11




            11





            Thinking for yourself is of course good. But your opening paragraph effectively includes learners or juniors dismissing the input of teachers or seniors; which goes too far.

            – Flater
            yesterday





            Thinking for yourself is of course good. But your opening paragraph effectively includes learners or juniors dismissing the input of teachers or seniors; which goes too far.

            – Flater
            yesterday




            6




            6





            @Flater After thinking about the input of teachers or seniors, and seeing they are wrong, dismissing their input is the only right thing. In the end, it is not about dismissing, but about questioning it and only dismissing it if it definitely proves wrong.

            – glglgl
            yesterday





            @Flater After thinking about the input of teachers or seniors, and seeing they are wrong, dismissing their input is the only right thing. In the end, it is not about dismissing, but about questioning it and only dismissing it if it definitely proves wrong.

            – glglgl
            yesterday













            @Flater: A good teacher or senior should encourage such thinking. "Here are my teachings, make them better."

            – Christian Hackl
            yesterday






            @Flater: A good teacher or senior should encourage such thinking. "Here are my teachings, make them better."

            – Christian Hackl
            yesterday





            7




            7





            @ChristianHackl: I'm fully on board with not blindly following traditionalism, but I wouldn't blindly dismiss it either. The answer seemingly suggests to eschew acquired knowledge in favor of your own view, and that's not a healthy approach to take. Blindly following others' opinions, no. Questioning something when you disagree, obviously yes. Outright dismissing it because you disagree, no. As I read it, the first paragraph of the answer at least seems to suggest the latter. It very much depends on what gnasher means with "think for yourself", which needs elaboration.

            – Flater
            yesterday






            @ChristianHackl: I'm fully on board with not blindly following traditionalism, but I wouldn't blindly dismiss it either. The answer seemingly suggests to eschew acquired knowledge in favor of your own view, and that's not a healthy approach to take. Blindly following others' opinions, no. Questioning something when you disagree, obviously yes. Outright dismissing it because you disagree, no. As I read it, the first paragraph of the answer at least seems to suggest the latter. It very much depends on what gnasher means with "think for yourself", which needs elaboration.

            – Flater
            yesterday





            5




            5





            On a sharing wisdom platform, this seems a little out of place...

            – drjpizzle
            yesterday





            On a sharing wisdom platform, this seems a little out of place...

            – drjpizzle
            yesterday











            13














            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.






            share|improve this answer





























              13














              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.






              share|improve this answer



























                13












                13








                13







                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.






                share|improve this answer















                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.







                share|improve this answer














                share|improve this answer



                share|improve this answer








                edited yesterday

























                answered yesterday









                meritonmeriton

                1,949713




                1,949713





















                    9














                    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.






                    share|improve this answer




















                    • 2





                      Eliminating all variables doesn't necessarily make code easier to read.

                      – Pharap
                      18 hours ago











                    • Eliminate all variables completely with pointfree style en.wikipedia.org/wiki/Tacit_programming

                      – Marcin
                      7 hours ago











                    • @Pharap True, a lack of variables does not ensure legibility. In some cases it even makes debugging more difficult. The point is that well chosen names, a clear usage of expression, can communicate an idea very clearly, while still being efficient.

                      – Kain0_0
                      5 hours ago
















                    9














                    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.






                    share|improve this answer




















                    • 2





                      Eliminating all variables doesn't necessarily make code easier to read.

                      – Pharap
                      18 hours ago











                    • Eliminate all variables completely with pointfree style en.wikipedia.org/wiki/Tacit_programming

                      – Marcin
                      7 hours ago











                    • @Pharap True, a lack of variables does not ensure legibility. In some cases it even makes debugging more difficult. The point is that well chosen names, a clear usage of expression, can communicate an idea very clearly, while still being efficient.

                      – Kain0_0
                      5 hours ago














                    9












                    9








                    9







                    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.






                    share|improve this answer















                    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.







                    share|improve this answer














                    share|improve this answer



                    share|improve this answer








                    edited yesterday

























                    answered yesterday









                    Kain0_0Kain0_0

                    3,514417




                    3,514417







                    • 2





                      Eliminating all variables doesn't necessarily make code easier to read.

                      – Pharap
                      18 hours ago











                    • Eliminate all variables completely with pointfree style en.wikipedia.org/wiki/Tacit_programming

                      – Marcin
                      7 hours ago











                    • @Pharap True, a lack of variables does not ensure legibility. In some cases it even makes debugging more difficult. The point is that well chosen names, a clear usage of expression, can communicate an idea very clearly, while still being efficient.

                      – Kain0_0
                      5 hours ago













                    • 2





                      Eliminating all variables doesn't necessarily make code easier to read.

                      – Pharap
                      18 hours ago











                    • Eliminate all variables completely with pointfree style en.wikipedia.org/wiki/Tacit_programming

                      – Marcin
                      7 hours ago











                    • @Pharap True, a lack of variables does not ensure legibility. In some cases it even makes debugging more difficult. The point is that well chosen names, a clear usage of expression, can communicate an idea very clearly, while still being efficient.

                      – Kain0_0
                      5 hours ago








                    2




                    2





                    Eliminating all variables doesn't necessarily make code easier to read.

                    – Pharap
                    18 hours ago





                    Eliminating all variables doesn't necessarily make code easier to read.

                    – Pharap
                    18 hours ago













                    Eliminate all variables completely with pointfree style en.wikipedia.org/wiki/Tacit_programming

                    – Marcin
                    7 hours ago





                    Eliminate all variables completely with pointfree style en.wikipedia.org/wiki/Tacit_programming

                    – Marcin
                    7 hours ago













                    @Pharap True, a lack of variables does not ensure legibility. In some cases it even makes debugging more difficult. The point is that well chosen names, a clear usage of expression, can communicate an idea very clearly, while still being efficient.

                    – Kain0_0
                    5 hours ago






                    @Pharap True, a lack of variables does not ensure legibility. In some cases it even makes debugging more difficult. The point is that well chosen names, a clear usage of expression, can communicate an idea very clearly, while still being efficient.

                    – Kain0_0
                    5 hours ago












                    6















                    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.






                    share|improve this answer


















                    • 2





                      So far, this is the only answer that mentions thread-safety and concurrency. That's kind of amazing given the specific code example in the question: an instance of SomeBusinessProcess cannot safely process multiple encrypted requests at once. The method public EncryptedResponse process(EncryptedRequest encryptedRequest) is not synchronized, and concurrent calls would quite possibly clobber the values of the instance variables. This is a good point to bring up.

                      – Joshua Taylor
                      yesterday















                    6















                    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.






                    share|improve this answer


















                    • 2





                      So far, this is the only answer that mentions thread-safety and concurrency. That's kind of amazing given the specific code example in the question: an instance of SomeBusinessProcess cannot safely process multiple encrypted requests at once. The method public EncryptedResponse process(EncryptedRequest encryptedRequest) is not synchronized, and concurrent calls would quite possibly clobber the values of the instance variables. This is a good point to bring up.

                      – Joshua Taylor
                      yesterday













                    6












                    6








                    6








                    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.






                    share|improve this answer














                    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.







                    share|improve this answer












                    share|improve this answer



                    share|improve this answer










                    answered yesterday









                    John BollingerJohn Bollinger

                    29915




                    29915







                    • 2





                      So far, this is the only answer that mentions thread-safety and concurrency. That's kind of amazing given the specific code example in the question: an instance of SomeBusinessProcess cannot safely process multiple encrypted requests at once. The method public EncryptedResponse process(EncryptedRequest encryptedRequest) is not synchronized, and concurrent calls would quite possibly clobber the values of the instance variables. This is a good point to bring up.

                      – Joshua Taylor
                      yesterday












                    • 2





                      So far, this is the only answer that mentions thread-safety and concurrency. That's kind of amazing given the specific code example in the question: an instance of SomeBusinessProcess cannot safely process multiple encrypted requests at once. The method public EncryptedResponse process(EncryptedRequest encryptedRequest) is not synchronized, and concurrent calls would quite possibly clobber the values of the instance variables. This is a good point to bring up.

                      – Joshua Taylor
                      yesterday







                    2




                    2





                    So far, this is the only answer that mentions thread-safety and concurrency. That's kind of amazing given the specific code example in the question: an instance of SomeBusinessProcess cannot safely process multiple encrypted requests at once. The method public EncryptedResponse process(EncryptedRequest encryptedRequest) is not synchronized, and concurrent calls would quite possibly clobber the values of the instance variables. This is a good point to bring up.

                    – Joshua Taylor
                    yesterday





                    So far, this is the only answer that mentions thread-safety and concurrency. That's kind of amazing given the specific code example in the question: an instance of SomeBusinessProcess cannot safely process multiple encrypted requests at once. The method public EncryptedResponse process(EncryptedRequest encryptedRequest) is not synchronized, and concurrent calls would quite possibly clobber the values of the instance variables. This is a good point to bring up.

                    – Joshua Taylor
                    yesterday











                    5














                    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.






                    share|improve this answer

























                    • My upvote, though many will hesitate here. Of course some abstractions, make sense. (BTW a method called "process" is terrible.) But here the logic is minimal. The OP's question however is on an entire code style, and there the case may be more complex.

                      – Joop Eggen
                      yesterday











                    • One clear issue with chaining it all into one method call is sheer readability. It also doesn't work if you need more than one operation on a given object. Also, this is nigh impossible to debug because you can't step through the operations and inspect the objects. While this works on a technical level, I would not advocate for this as it massively neglects non-runtime aspects of software development.

                      – Flater
                      yesterday












                    • @Flater I agree with your comments, we don't want to apply this everywhere. I edited my answer to clarify my practical position. What I want to show is that in practice we only apply rules when it's appropriate. Chaining method call is fine in this instance, and if I need to debug, I'd invoke the tests for the chained methods.

                      – imel96
                      yesterday











                    • @JoopEggen Yes, abstractions make sense. In the example, the private methods don't give any abstractions anyway, users of the class don't even know about them

                      – imel96
                      yesterday















                    5














                    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.






                    share|improve this answer

























                    • My upvote, though many will hesitate here. Of course some abstractions, make sense. (BTW a method called "process" is terrible.) But here the logic is minimal. The OP's question however is on an entire code style, and there the case may be more complex.

                      – Joop Eggen
                      yesterday











                    • One clear issue with chaining it all into one method call is sheer readability. It also doesn't work if you need more than one operation on a given object. Also, this is nigh impossible to debug because you can't step through the operations and inspect the objects. While this works on a technical level, I would not advocate for this as it massively neglects non-runtime aspects of software development.

                      – Flater
                      yesterday












                    • @Flater I agree with your comments, we don't want to apply this everywhere. I edited my answer to clarify my practical position. What I want to show is that in practice we only apply rules when it's appropriate. Chaining method call is fine in this instance, and if I need to debug, I'd invoke the tests for the chained methods.

                      – imel96
                      yesterday











                    • @JoopEggen Yes, abstractions make sense. In the example, the private methods don't give any abstractions anyway, users of the class don't even know about them

                      – imel96
                      yesterday













                    5












                    5








                    5







                    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.






                    share|improve this answer















                    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.







                    share|improve this answer














                    share|improve this answer



                    share|improve this answer








                    edited yesterday

























                    answered yesterday









                    imel96imel96

                    2,47911020




                    2,47911020












                    • My upvote, though many will hesitate here. Of course some abstractions, make sense. (BTW a method called "process" is terrible.) But here the logic is minimal. The OP's question however is on an entire code style, and there the case may be more complex.

                      – Joop Eggen
                      yesterday











                    • One clear issue with chaining it all into one method call is sheer readability. It also doesn't work if you need more than one operation on a given object. Also, this is nigh impossible to debug because you can't step through the operations and inspect the objects. While this works on a technical level, I would not advocate for this as it massively neglects non-runtime aspects of software development.

                      – Flater
                      yesterday












                    • @Flater I agree with your comments, we don't want to apply this everywhere. I edited my answer to clarify my practical position. What I want to show is that in practice we only apply rules when it's appropriate. Chaining method call is fine in this instance, and if I need to debug, I'd invoke the tests for the chained methods.

                      – imel96
                      yesterday











                    • @JoopEggen Yes, abstractions make sense. In the example, the private methods don't give any abstractions anyway, users of the class don't even know about them

                      – imel96
                      yesterday

















                    • My upvote, though many will hesitate here. Of course some abstractions, make sense. (BTW a method called "process" is terrible.) But here the logic is minimal. The OP's question however is on an entire code style, and there the case may be more complex.

                      – Joop Eggen
                      yesterday











                    • One clear issue with chaining it all into one method call is sheer readability. It also doesn't work if you need more than one operation on a given object. Also, this is nigh impossible to debug because you can't step through the operations and inspect the objects. While this works on a technical level, I would not advocate for this as it massively neglects non-runtime aspects of software development.

                      – Flater
                      yesterday












                    • @Flater I agree with your comments, we don't want to apply this everywhere. I edited my answer to clarify my practical position. What I want to show is that in practice we only apply rules when it's appropriate. Chaining method call is fine in this instance, and if I need to debug, I'd invoke the tests for the chained methods.

                      – imel96
                      yesterday











                    • @JoopEggen Yes, abstractions make sense. In the example, the private methods don't give any abstractions anyway, users of the class don't even know about them

                      – imel96
                      yesterday
















                    My upvote, though many will hesitate here. Of course some abstractions, make sense. (BTW a method called "process" is terrible.) But here the logic is minimal. The OP's question however is on an entire code style, and there the case may be more complex.

                    – Joop Eggen
                    yesterday





                    My upvote, though many will hesitate here. Of course some abstractions, make sense. (BTW a method called "process" is terrible.) But here the logic is minimal. The OP's question however is on an entire code style, and there the case may be more complex.

                    – Joop Eggen
                    yesterday













                    One clear issue with chaining it all into one method call is sheer readability. It also doesn't work if you need more than one operation on a given object. Also, this is nigh impossible to debug because you can't step through the operations and inspect the objects. While this works on a technical level, I would not advocate for this as it massively neglects non-runtime aspects of software development.

                    – Flater
                    yesterday






                    One clear issue with chaining it all into one method call is sheer readability. It also doesn't work if you need more than one operation on a given object. Also, this is nigh impossible to debug because you can't step through the operations and inspect the objects. While this works on a technical level, I would not advocate for this as it massively neglects non-runtime aspects of software development.

                    – Flater
                    yesterday














                    @Flater I agree with your comments, we don't want to apply this everywhere. I edited my answer to clarify my practical position. What I want to show is that in practice we only apply rules when it's appropriate. Chaining method call is fine in this instance, and if I need to debug, I'd invoke the tests for the chained methods.

                    – imel96
                    yesterday





                    @Flater I agree with your comments, we don't want to apply this everywhere. I edited my answer to clarify my practical position. What I want to show is that in practice we only apply rules when it's appropriate. Chaining method call is fine in this instance, and if I need to debug, I'd invoke the tests for the chained methods.

                    – imel96
                    yesterday













                    @JoopEggen Yes, abstractions make sense. In the example, the private methods don't give any abstractions anyway, users of the class don't even know about them

                    – imel96
                    yesterday





                    @JoopEggen Yes, abstractions make sense. In the example, the private methods don't give any abstractions anyway, users of the class don't even know about them

                    – imel96
                    yesterday











                    4














                    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()





                    share|improve this answer










                    New contributor




                    user673679 is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                    Check out our Code of Conduct.
























                      4














                      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()





                      share|improve this answer










                      New contributor




                      user673679 is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                      Check out our Code of Conduct.






















                        4












                        4








                        4







                        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()





                        share|improve this answer










                        New contributor




                        user673679 is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                        Check out our Code of Conduct.










                        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()






                        share|improve this answer










                        New contributor




                        user673679 is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                        Check out our Code of Conduct.









                        share|improve this answer



                        share|improve this answer








                        edited yesterday





















                        New contributor




                        user673679 is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                        Check out our Code of Conduct.









                        answered yesterday









                        user673679user673679

                        1497




                        1497




                        New contributor




                        user673679 is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                        Check out our Code of Conduct.





                        New contributor





                        user673679 is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                        Check out our Code of Conduct.






                        user673679 is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                        Check out our Code of Conduct.





















                            1














                            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.






                            share|improve this answer








                            New contributor




                            kap is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                            Check out our Code of Conduct.
























                              1














                              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.






                              share|improve this answer








                              New contributor




                              kap is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                              Check out our Code of Conduct.






















                                1












                                1








                                1







                                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.






                                share|improve this answer








                                New contributor




                                kap is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                                Check out our Code of Conduct.










                                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.







                                share|improve this answer








                                New contributor




                                kap is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                                Check out our Code of Conduct.









                                share|improve this answer



                                share|improve this answer






                                New contributor




                                kap is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                                Check out our Code of Conduct.









                                answered yesterday









                                kapkap

                                1113




                                1113




                                New contributor




                                kap is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                                Check out our Code of Conduct.





                                New contributor





                                kap is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                                Check out our Code of Conduct.






                                kap is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                                Check out our Code of Conduct.





















                                    1














                                    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.






                                    share|improve this answer



























                                      1














                                      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.






                                      share|improve this answer

























                                        1












                                        1








                                        1







                                        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.






                                        share|improve this answer













                                        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.







                                        share|improve this answer












                                        share|improve this answer



                                        share|improve this answer










                                        answered yesterday









                                        drjpizzledrjpizzle

                                        24416




                                        24416





















                                            0














                                            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);







                                            share|improve this answer








                                            New contributor




                                            Roman Weis is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                                            Check out our Code of Conduct.
























                                              0














                                              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);







                                              share|improve this answer








                                              New contributor




                                              Roman Weis is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                                              Check out our Code of Conduct.






















                                                0












                                                0








                                                0







                                                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);







                                                share|improve this answer








                                                New contributor




                                                Roman Weis is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                                                Check out our Code of Conduct.










                                                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);








                                                share|improve this answer








                                                New contributor




                                                Roman Weis is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                                                Check out our Code of Conduct.









                                                share|improve this answer



                                                share|improve this answer






                                                New contributor




                                                Roman Weis is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                                                Check out our Code of Conduct.









                                                answered yesterday









                                                Roman WeisRoman Weis

                                                341




                                                341




                                                New contributor




                                                Roman Weis is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                                                Check out our Code of Conduct.





                                                New contributor





                                                Roman Weis is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                                                Check out our Code of Conduct.






                                                Roman Weis is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                                                Check out our Code of Conduct.





















                                                    -3














                                                    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.






                                                    share|improve this answer




















                                                    • 4





                                                      So what's the point of your answer, exactly?

                                                      – Eric Duminil
                                                      yesterday











                                                    • To provide a different perspective/wording of the potential problem at hand when preferring shared state over local state. Next.

                                                      – luis.espinal
                                                      yesterday






                                                    • 2





                                                      It should be a comment IMHO. Let's wait for the community to decide if your answer is helpful.

                                                      – Eric Duminil
                                                      yesterday











                                                    • Perhaps so, and perhaps then you should have let the community decide first. ¯_(ツ)_/¯. Anyways/whatevs, the community can choose as they please. #shrugs

                                                      – luis.espinal
                                                      yesterday















                                                    -3














                                                    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.






                                                    share|improve this answer




















                                                    • 4





                                                      So what's the point of your answer, exactly?

                                                      – Eric Duminil
                                                      yesterday











                                                    • To provide a different perspective/wording of the potential problem at hand when preferring shared state over local state. Next.

                                                      – luis.espinal
                                                      yesterday






                                                    • 2





                                                      It should be a comment IMHO. Let's wait for the community to decide if your answer is helpful.

                                                      – Eric Duminil
                                                      yesterday











                                                    • Perhaps so, and perhaps then you should have let the community decide first. ¯_(ツ)_/¯. Anyways/whatevs, the community can choose as they please. #shrugs

                                                      – luis.espinal
                                                      yesterday













                                                    -3












                                                    -3








                                                    -3







                                                    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.






                                                    share|improve this answer















                                                    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.







                                                    share|improve this answer














                                                    share|improve this answer



                                                    share|improve this answer








                                                    edited 20 hours ago









                                                    Matt Ellen

                                                    2,91332537




                                                    2,91332537










                                                    answered yesterday









                                                    luis.espinalluis.espinal

                                                    2,39611517




                                                    2,39611517







                                                    • 4





                                                      So what's the point of your answer, exactly?

                                                      – Eric Duminil
                                                      yesterday











                                                    • To provide a different perspective/wording of the potential problem at hand when preferring shared state over local state. Next.

                                                      – luis.espinal
                                                      yesterday






                                                    • 2





                                                      It should be a comment IMHO. Let's wait for the community to decide if your answer is helpful.

                                                      – Eric Duminil
                                                      yesterday











                                                    • Perhaps so, and perhaps then you should have let the community decide first. ¯_(ツ)_/¯. Anyways/whatevs, the community can choose as they please. #shrugs

                                                      – luis.espinal
                                                      yesterday












                                                    • 4





                                                      So what's the point of your answer, exactly?

                                                      – Eric Duminil
                                                      yesterday











                                                    • To provide a different perspective/wording of the potential problem at hand when preferring shared state over local state. Next.

                                                      – luis.espinal
                                                      yesterday






                                                    • 2





                                                      It should be a comment IMHO. Let's wait for the community to decide if your answer is helpful.

                                                      – Eric Duminil
                                                      yesterday











                                                    • Perhaps so, and perhaps then you should have let the community decide first. ¯_(ツ)_/¯. Anyways/whatevs, the community can choose as they please. #shrugs

                                                      – luis.espinal
                                                      yesterday







                                                    4




                                                    4





                                                    So what's the point of your answer, exactly?

                                                    – Eric Duminil
                                                    yesterday





                                                    So what's the point of your answer, exactly?

                                                    – Eric Duminil
                                                    yesterday













                                                    To provide a different perspective/wording of the potential problem at hand when preferring shared state over local state. Next.

                                                    – luis.espinal
                                                    yesterday





                                                    To provide a different perspective/wording of the potential problem at hand when preferring shared state over local state. Next.

                                                    – luis.espinal
                                                    yesterday




                                                    2




                                                    2





                                                    It should be a comment IMHO. Let's wait for the community to decide if your answer is helpful.

                                                    – Eric Duminil
                                                    yesterday





                                                    It should be a comment IMHO. Let's wait for the community to decide if your answer is helpful.

                                                    – Eric Duminil
                                                    yesterday













                                                    Perhaps so, and perhaps then you should have let the community decide first. ¯_(ツ)_/¯. Anyways/whatevs, the community can choose as they please. #shrugs

                                                    – luis.espinal
                                                    yesterday





                                                    Perhaps so, and perhaps then you should have let the community decide first. ¯_(ツ)_/¯. Anyways/whatevs, the community can choose as they please. #shrugs

                                                    – luis.espinal
                                                    yesterday





                                                    protected by gnat 21 hours ago



                                                    Thank you for your interest in this question.
                                                    Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).



                                                    Would you like to answer one of these unanswered questions instead?



                                                    Popular posts from this blog

                                                    Can't initialize raids on a new ASUS Prime B360M-A motherboard2019 Community Moderator ElectionSimilar to RAID config yet more like mirroring solution?Can't get motherboard serial numberWhy does the BIOS entry point start with a WBINVD instruction?UEFI performance Asus Maximus V Extreme

                                                    Identity Server 4 is not redirecting to Angular app after login2019 Community Moderator ElectionIdentity Server 4 and dockerIdentityserver implicit flow unauthorized_clientIdentityServer Hybrid Flow - Access Token is null after user successful loginIdentity Server to MVC client : Page Redirect After loginLogin with Steam OpenId(oidc-client-js)Identity Server 4+.NET Core 2.0 + IdentityIdentityServer4 post-login redirect not working in Edge browserCall to IdentityServer4 generates System.NullReferenceException: Object reference not set to an instance of an objectIdentityServer4 without HTTPS not workingHow to get Authorization code from identity server without login form

                                                    2005 Ahvaz unrest Contents Background Causes Casualties Aftermath See also References Navigation menue"At Least 10 Are Killed by Bombs in Iran""Iran"Archived"Arab-Iranians in Iran to make April 15 'Day of Fury'"State of Mind, State of Order: Reactions to Ethnic Unrest in the Islamic Republic of Iran.10.1111/j.1754-9469.2008.00028.x"Iran hangs Arab separatists"Iran Overview from ArchivedConstitution of the Islamic Republic of Iran"Tehran puzzled by forged 'riots' letter""Iran and its minorities: Down in the second class""Iran: Handling Of Ahvaz Unrest Could End With Televised Confessions""Bombings Rock Iran Ahead of Election""Five die in Iran ethnic clashes""Iran: Need for restraint as anniversary of unrest in Khuzestan approaches"Archived"Iranian Sunni protesters killed in clashes with security forces"Archived