Simplified a little, I have this code:
public class TaskProcessCases{
private Assessment assessment ;
public void execute() { // the actual name
for (Case case : cases) {
assessment = new Assessment();
// some more code (which a.o. sets 'id')
retrieveData(id); // the actual name
if (assessment.getMyField() == null) {
reopen();
}
// some more code
}
}
private void retrieveData(int id) {
// some more code which uses 'id'
assessment.setMyField(Integer.valueOf(42)); // just example
}
}
public class Assessment {
Integer myField;
public Integer getMyField() { return myField; }
public void setMyField(Integer myField) { this.myField = myField; }
}
My problem with this is that reading execute, I have to check all methods called (like retrieveData) to know where assessment is changed.
Is this the right way to write this?
Should I pass assessment to retrieveData like retrieveData(assessment ) to make it more explicit? Should retrieveData be a method of assessment? With help I thought I might initialize everything in the constructor and make the class immutable, which is great, but rather intrusive.
EDIT: My aim is to have code that is readable to a typical programmer, which does not invite the making of wrong assumptions and which has few ways to be used/changed incorrectly. Small differences in performance don't matter here.
EDIT: I made the code more specific. Here is the old version which the first answers were based on:
class Alpha {
Beta inst;
void doThing() {
inst = new Beta();
// some more code
fill();
// some more code
inst.getMyField();
}
void fill() {
// some more code
inst.setMyField(Integer.valueOf(42));
}
}
class Beta {
Integer myField;
public Integer getMyField() { return myField; }
public void setMyField(Integer myField) { this.myField = myField; }
}
5 Answers 5
It really depends on a few things:
- Is the caller responsible to know what the value is supposed to be?
- Are there multiple setters being called in the method?
- What is the ultimate purpose of the method being called?
In short we are trying to balance readability with convenience. I know sample code uses generic names on purpose, but I have seen real examples where a method was named something generic like fill(). I would have a real problem with that.
Your internal method should be named descriptively enough that I can guess what's going on. As a midway point, if a method I'm calling does multiple things on an instance, I might argue that you should pass that instance in to the method like this:
private void fill(Beta beta) {
// do all kinds of stuff
beta.setMyField(Integer.valueOf(42));
}
That at least clues me in that the Beta I pass in is going to change. I would much rather have that clue than the fill() method changing class state without any clue as to what state is being changed.
In your example, there is no real reason to have a Beta inst class field. It is created new within doSomething() and passed behind the scenes to fill(). If you make the change to fill(Beta), you can easily remove the class field. Your new class looks like this:
class Alpha {
void doSomething() {
Beta beta = new Beta();
// Do stuff
fill(beta);
// Do more stuff;
beta.getMyField();
}
void fill(Beta beta) {
// do stuff
beta.setMyField(Integer.valueOf(42));
}
}
The key difference here is that Beta is no longer a class field at all. That improves the likelihood that doSomething() is re-entrant--which it most definitely was not before.
In short:
- Pass instances explicitly, having a class field to do that implicitly causes concurrency problems and other subtle bugs
- Name methods appropriately so that you can have a good idea as to what they are really doing
I think the answer to you question is: it depends.
First of all looking at this example your problem is related to too many responsibilities of doThinkg(), that's why it's unclear what it does and what gets modified.
Possible solution
1) Try to introduce single responsibility to your methods
2) Provide private method/util function (Service with static method or utils function if your
language is capable of):
Example of util static helpers:
class Alpha {
Beta inst;
public void initilize() {
this.inst = AlphaHelper.prepareBeta();
}
public Integer getBetaField() {
return this.inst.getMyField()
}
}
class AlphaHelper {
public static Beta prepareBeta() {
Beta beta = AlphaHelper.getBetaInstance();
AlphaHelper.initilizeBeta(beta);
return beta;
}
private static getBetaInstance() {
return new Beta();
}
private static initilizeBeta(Beta beta) {
beta.setMyField(Integer.valueOf(42))
}
}
Example of private method
class Alpha {
Beta inst;
public void initilize() {
this.inst = new Beta();
// call other private methods or do something
this.fillBetaInstance(this.inst);
}
public Integer getBetaField() {
return this.inst.getMyField();
}
private void fillBetaInstance(Beta beta) {
beta.setMyField(Integer.valueOf(42));
}
}
This code snippet and its usage omits some important details. So, the answer it depends makes sense.
One significant way to look at it is by the lifetime of state: in particular here, the value assigned to the field (and the usage of the field) vs. the lifetime of whole the object itself.
If the field is a way of passing temporary state from doThing to fill, then it is probably in the wrong place. Here, we cannot know for sure due to the simplicity of the (albeit contrived) example.
Or does the field capture long running state that is initialized in the constructor and has a valid value the whole lifetime of the object (even though may change)?
At this point, I would start looking at another important perspective, which is the abstraction being provided to a consuming client programmer (perhaps you), who, on usage, shouldn't be concerned with implementation but rather the interface — which we hope to be dead simple to use and hard to get wrong.
I might ask: is fill essentially private but not marked as such because of the incomplete example? If not, what do callers expect when the use fill (perhaps what do they need)?
Is it possible for field inst to be null (uninitialized)? What is the expected value of inst after construction?
Construction is not just for raw initialization of fields, but also for binding objects together into a single abstraction. For many purposes, construction can bind objects together into a single abstraction that lasts the entire lifetime of the constructed entity. Whenever the client programmer can consume one abstraction this an improvement over them juggling multiple objects at the same time, e.g. as a pair (or more) of objects.
This is one reason that the immutable approach you mention works well. However, we can have some mutable state in our objects while still following an approach that uses construction for binding several objects into a single abstraction for the client to use (and preferably perhaps that at least the bindings from construction are immutable). This style suggests that changing the bindings means creating another object — it is a different instance of the same abstraction, rather than an abstraction whose bindings are mutable.
Setting fields without argument passing is not bad by itself. Magic numbers like 42 are almost always a bad idea.
Your problem is inherent to the fact that instances of TaskProcessCases have mutable state so every method call could potentially change the state, it has nothing to do with passing or not passing arguments to the method.
The problem is usually mitigated by good naming. retrieveData does not sountd to me like the name of something that is changing state but in your domain it could be different.
You could use custom exceptions to ensure some required scenario, and to make it explicit in your code:
void doThing() throws NotConfiguredException {
if (inst != null && inst.x > 0) { //or some other validations
//...do your stuf here...
//it's very explicit what are the conditions
//required for this logic to work
} else {
throw new NotConfiguredException("Beta.x <= 0");
}
}
Explore related questions
See similar questions with these tags.
fillcan be renamed to "initMyFieldOfBeta", then it should be clear what it does, but does this make sense in context of the other abstractions? Impossible to say.