0

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");
} }
  • 3
    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
  • 2
    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
3

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.

| improve this answer | |
0

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); 
| improve this answer | |
  • [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
  • 1
    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

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.