If I modify a private field using another setter, is it a bad practice?

Question

I have a java class where I have few booleans and one arraylist. If the boolean field is set to true, I need to add it to the arraylist. One of the ways I can do this is to use the setters of each boolean to add elements to the arraylist.

But is it a good programming practice? I couldn't find concrete information on Google on the same. E.g.

public class Thisismyclass implements Serializable {
private boolean abc;
private boolean xyz;
private boolean pqr;
private ArrayList<String> stringList;

public ArrayList<String> getStringList() {
    if (stringList==null){
        return new ArrayList<>();
    }
    return stringList;
}

public void setStringList(ArrayList<String> stringList) {
    this.stringList = stringList;
}

public boolean isAbc() {
    return abc;
}

public void setAbc(boolean abc) {
    this.abc = abc;
    this.getStringList().add("abc");
} }

The example's ``getStringList`` is inconsistent in whether it returns a live list or snapshot. Not really related to the subject but it bothers me. — HAEM, Jul 26 '18 at 12:35
This code has a severe bug resulting from the violation of the *encapsulation/information hiding* principle: when calling `setAbc()` before `setStringList()` the state change will be lost! — Timothy Truckle, Jul 26 '18 at 12:52

score 4 · Answer 1 · edited Jul 31 '18 at 20:16

The example you provided would probably be considered a bad practice, but a more common example would be maintaining a dirty flag. The dirty flag lets your persistence layer know that there are pending changes to save. (If you use ORM tools then this logic is handled for you in the code generated by that tool). Example:

public class MyThing {
    private boolean changed = false;
    private String value;

    public String getValue() {
        return value;
    }

    public void setValue(String newValue) {
        value = newValue;
        changed = true;
    }

    public boolean isChanged() {
        return changed;
    }
}

This pattern is where you have additional state that isn't fully exposed to code using the class. The persistence layer will happily skip trying to persist the record if it already exists and hasn't change.

That said, providing a mechanism to change the value of the changed field from outside the class would definitely be an antipattern, because you can no longer guarantee that changed corresponds to the actual condition of the object.

JimmyJames · Answer 2 · 2018-07-31T22:05:24.587

There's nothing inherently wrong with modifying more than one private variable in your setter. The variable names in your example lack any useful meaning or context so I can only guess at what this is about. Based on the fact that you are adding the name of the property ("abc") to the 'string list', I figure this is some sort of observable pattern implementation. Nothing wrong with that, in itself.

There is a major issue with this class, however. It's that you have a getter and setter for a list. This is a terrible, terrible approach. One part of the code can call the getter for the list and be able to see all the "abc" strings you are adding to it and starting adding their own values such as "butt" and "poop" (or any object really) but only up until some other part of the program replaces the list object that your object is using internally with their own list. This is a tremendously egregious breaking of encapsulation. It will definitely be a debugging nightmare. Did I mention that this is a terrible approach?

You should never ever return or accept a collection or array in a setter. Returning a modifiable collection or array from a getter is also a big mistake. If you give a little more context to what you are trying to accomplish, I will offer some alternative approaches.

Here's a little code to demonstrate how your current code behaves. I added a print to setAbc that counts the calls to help make it more clear what is happening:

final Thisismyclass instance = new Thisismyclass();

ArrayList<String> listA = instance.getStringList();
ArrayList<String> listB = instance.getStringList();

System.out.println();
System.out.println("listA: " + listA);
System.out.println("listB: " + listB);

instance.setStringList(listB);

ArrayList<String> listC = instance.getStringList();

System.out.println();
System.out.println("listA is same as listB? " + (listA == listB));
System.out.println("listB is same as listC? " + (listB == listC));

listB.add("yipee!");
instance.setAbc(true);

System.out.println();
System.out.println("listA: " + listA); 
System.out.println("listB: " + listB); 
System.out.println("listC: " + listC); 

instance.setStringList(null); // HA HA!

instance.setAbc(true);

ArrayList<String> listD = instance.getStringList();

System.out.println();
System.out.println("listA: " + listA); 
System.out.println("listB: " + listB); 
System.out.println("listC: " + listC); 
System.out.println("listD: " + listD); 

System.out.println();
System.out.println("listD is same as listA? " + (listD == listA)); 
System.out.println("listD is same as listB? " + (listD == listB)); 
System.out.println("listD is same as listC? " + (listD == listC)); 

System.out.println();
System.out.println("listA: " + listA); 
System.out.println("listB: " + listB); 
System.out.println("listC: " + listC); 
System.out.println("listD: " + listD); 

instance.setAbc(true);

System.out.println();
System.out.println("listA: " + listA); 
System.out.println("listB: " + listB); 
System.out.println("listC: " + listC); 
System.out.println("listD: " + listD); 

ArrayList<String> listE = instance.getStringList();
ArrayList<String> listF = instance.getStringList();

listE.add("nananananananananana Batman!");

System.out.println();
System.out.println("listA: " + listA); 
System.out.println("listB: " + listB); 
System.out.println("listC: " + listC); 
System.out.println("listD: " + listD); 
System.out.println("listE: " + listE); 
System.out.println("listF: " + listF); 

instance.setAbc(true);

System.out.println();
System.out.println("listA: " + listA); 
System.out.println("listB: " + listB); 
System.out.println("listC: " + listC); 
System.out.println("listD: " + listD); 
System.out.println("listE: " + listE); 
System.out.println("listF: " + listF); 

instance.setStringList(listE);

ArrayList<String> listG = instance.getStringList();

System.out.println();
System.out.println("listA: " + listA); 
System.out.println("listB: " + listB); 
System.out.println("listC: " + listC); 
System.out.println("listD: " + listD); 
System.out.println("listE: " + listE);
System.out.println("listF: " + listF); 
System.out.println("listG: " + listG);

instance.setAbc(true);

System.out.println();
System.out.println("listA: " + listA); 
System.out.println("listB: " + listB); 
System.out.println("listC: " + listC); 
System.out.println("listD: " + listD); 
System.out.println("listE: " + listE); 
System.out.println("listF: " + listF); 
System.out.println("listG: " + listG);

*[shrug]* Collections are exposed this way all the time in the MVVM pattern for data binding purposes. — Robert Harvey, Jul 31 '18 at 20:21
@RobertHarvey Such that you can replace the instance of the collection inside the object? — JimmyJames, Jul 31 '18 at 20:22
Yes. You simply refrain from doing so. It's ordinary data binding; you don't need a language-specific enforcement; you just need competent developers. — Robert Harvey, Jul 31 '18 at 21:51
@RobertHarvey So the plan is to 1. add features that can only cause problems and then 2. not use them. And you think that's a good way to do things? Seems pretty dumb to me. — JimmyJames, Jul 31 '18 at 22:01
I just don't think reasonably competent software developers need a policeman looking over their shoulder all the time, that's all. Nor is that actually the case, even in bondage and discipline languages. — Robert Harvey, Jul 31 '18 at 22:06
Consider that languages like C# and Java actually *allow* you to use setters on public collection properties, even though it's never a good idea in your world. You can't legislate everything. — Robert Harvey, Jul 31 '18 at 22:08
@RobertHarvey No one is looking over anyone's shoulder. My point is about competency. Adding broken methods to a class is a prime example of *incompetency*. If you set boobytraps on the field, the kids aren't going to run as fast or play as well. See [poka-yoke](https://en.wikipedia.org/wiki/Poka-yoke). Instead of blaming other people for using your broken methods just write them correctly in the first place. — JimmyJames, Aug 01 '18 at 13:36
Just to shed a little light on this, it is an MVVM application. Making the bindable collections readonly means that I would have to instantiate them *in their entirety* in the constructor scope, which means no helper methods for the queries. Rather than make the collection properties readonly on the off-chance that someone is incompetent enough to replace the collections later, we simply trusted our people (it's a small group), and left the helper methods in. I can assure you unequivocally that *it is not broken;* our code works just fine. — Robert Harvey, Aug 01 '18 at 15:14

If I modify a private field using another setter, is it a bad practice?

2 Answers2