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
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
New contributor
|
show 27 more comments
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
New contributor
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
|
show 27 more comments
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
New contributor
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
java refactoring
New contributor
New contributor
edited yesterday
Alexander
New contributor
asked yesterday
AlexanderAlexander
40327
40327
New contributor
New contributor
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
|
show 27 more comments
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
|
show 27 more comments
12 Answers
12
active
oldest
votes
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 likeFooName
,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.
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 movepassRequestToServiceClient()
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 parameterprivate byte[] getEncodedData(EncryptedRequest encryptedRequest)
(in the original code, I would expect the example to contain a member variableencryptedRequest
).
– 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
|
show 17 more comments
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.
New contributor
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
add a comment |
"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).
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
|
show 3 more comments
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.
add a comment |
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.
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
add a comment |
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.
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 methodpublic 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
add a comment |
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.
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
add a comment |
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()
New contributor
add a comment |
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.
New contributor
add a comment |
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.
add a comment |
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);
New contributor
add a comment |
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.
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
add a comment |
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
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 likeFooName
,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.
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 movepassRequestToServiceClient()
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 parameterprivate byte[] getEncodedData(EncryptedRequest encryptedRequest)
(in the original code, I would expect the example to contain a member variableencryptedRequest
).
– 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
|
show 17 more comments
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 likeFooName
,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.
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 movepassRequestToServiceClient()
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 parameterprivate byte[] getEncodedData(EncryptedRequest encryptedRequest)
(in the original code, I would expect the example to contain a member variableencryptedRequest
).
– 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
|
show 17 more comments
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 likeFooName
,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.
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 likeFooName
,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.
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 movepassRequestToServiceClient()
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 parameterprivate byte[] getEncodedData(EncryptedRequest encryptedRequest)
(in the original code, I would expect the example to contain a member variableencryptedRequest
).
– 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
|
show 17 more comments
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 movepassRequestToServiceClient()
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 parameterprivate byte[] getEncodedData(EncryptedRequest encryptedRequest)
(in the original code, I would expect the example to contain a member variableencryptedRequest
).
– 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
|
show 17 more comments
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.
New contributor
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
add a comment |
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.
New contributor
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
add a comment |
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.
New contributor
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.
New contributor
New contributor
answered yesterday
AlexAlex
48124
48124
New contributor
New contributor
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
add a comment |
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
add a comment |
"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).
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
|
show 3 more comments
"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).
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
|
show 3 more comments
"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).
"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).
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
|
show 3 more comments
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
|
show 3 more comments
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.
add a comment |
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.
add a comment |
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.
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.
edited yesterday
answered yesterday
meritonmeriton
1,949713
1,949713
add a comment |
add a comment |
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.
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
add a comment |
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.
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
add a comment |
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.
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.
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
add a comment |
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
add a comment |
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.
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 methodpublic 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
add a comment |
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.
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 methodpublic 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
add a comment |
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.
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.
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 methodpublic 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
add a comment |
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 methodpublic 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
add a comment |
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.
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
add a comment |
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.
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
add a comment |
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.
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.
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
add a comment |
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
add a comment |
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()
New contributor
add a comment |
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()
New contributor
add a comment |
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()
New contributor
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()
New contributor
edited yesterday
New contributor
answered yesterday
user673679user673679
1497
1497
New contributor
New contributor
add a comment |
add a comment |
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.
New contributor
add a comment |
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.
New contributor
add a comment |
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.
New contributor
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.
New contributor
New contributor
answered yesterday
kapkap
1113
1113
New contributor
New contributor
add a comment |
add a comment |
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.
add a comment |
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.
add a comment |
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.
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.
answered yesterday
drjpizzledrjpizzle
24416
24416
add a comment |
add a comment |
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);
New contributor
add a comment |
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);
New contributor
add a comment |
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);
New contributor
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);
New contributor
New contributor
answered yesterday
Roman WeisRoman Weis
341
341
New contributor
New contributor
add a comment |
add a comment |
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.
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
add a comment |
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.
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
add a comment |
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.
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.
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
add a comment |
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
add a comment |
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?
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