Which method should change the field when you call the private method hierarchy?
When a public method of a class should call a private method that causes a field to change, which method should change the field? Is there a general agreement for this? Is one approach preferred over the other?
Consider these two pieces of code:
public class boolHolder {
private boolean theBool = false;
public void doYourThing() {
// Do a lot of other stuff
setBool();
}
private void setBool() {
// Do a lot of other stuff, justifying a private method for this
this.theBool = true;
}
}
VS
public class boolHolder {
private boolean theBool = false;
public void doYourThing() {
// Do a lot of other stuff
theBool = setBool();
}
private boolean setBool() {
// Do a lot of other stuff, justifying a private method for this
return true;
}
}
These two times were of course very simple, but I'm sure I'm not the only one who has public methods calling a huge private method tree. Should the field be set at the end of the branch or should a value be passed?
source to share
I think it makes sense that only one place will set the field value, and that should be the last method called. This makes the code easier to understand. Your first snippet looks much more readable to me.
Here's another snippet that I think supports this convention:
Suppose we have an int member with two setters - one takes an int and the other takes a string representation of that int (which is used, for example, if we de-serialize an instance from an XML string).
int value;
public void setIntField (String value)
throws SomeException
{
if (value == null)
throw new SomeException();
try {
int val = Integer.parseInt (value);
setIntField (val);
}
catch (NumberFormatException ex) {
throw new SomeException();
}
}
public void setIntField (int value)
throws SomeException ()
{
if (value < MIN_ALLOWED || value > MAX_ALLOWED)
throw new SomeException ();
this.value = value;
}
source to share
It won't change much, but you can try to use a more convenient name for your methods: I don't like that you call your second method setBool ().
If you write "Do a lot of other things, justifying a private method for this," you can try to relate the verb to what you are doing. Let's say you update the status of an account and want to signal with a boolean status when done, well use something like what you did, but name it in a meaningful way, eg. updateAccount () and either return true if the update went well or installed it internally:
public class boolHolder {
private boolean accountUpdated = false;
public void doYourThing() {
// Do a lot of preliminary stuff
updateAccount();
}
private void updateAccount() {
// try to update account
// if update went fine
this.accountUpdated = true;
}
}
or
public class boolHolder {
private boolean accountUpdated = false;
public void doYourThing() {
// Do a lot of preliminary stuff
this.accountUpdated = updateAccount();
}
private boolean updateAccount() {
// try to update account
// if error happens, rollback change and set
return false;
// else (update went fine)
return true;
}
}
both are fine, but make your method say what they do, since updating the bool is not the main action, since you are "Doing a lot of other things, justifying a private method for this."
The parameter value is internally more compact if you use the default as false like you do, and the other is more explicit in what it does. So I prefer to: return the result for you.
source to share