CODEWITHSUNDEEP
javapojo
eliocs
eliocs

Reputation: 18827

getters and setters performing additional logic

I have a Java class which represents the correlation between two elements (typical POJO):

public class Correlation {

    private final String a;
    private final String b;

    private double correlation;

    public Correlation(String a, String b) {
        this.a = a;
        this.b = b;
    }

    public double getCorrelation() {
        return correlation;
    }

    public void setCorrelation(double correlation) {
        this.correlation = correlation;
    }

}

To follow the correct correlation logic if a equals b then the correlation value should be ALWAYS 1. I could add the logic altering the getter method (ignore the fact of the possible null value for a):

public double getCorrelation() {
    if (a.equals(b)) {
        return 1D;
    } else {
        return correlation;
    }
}

What bothers me is adding this logic to a getter method, should I change the method name or documenting it should be considered enough?

Upvotes: 23

Views: 14397

Answers (7)

user949300
user949300

Reputation: 15729

In the cases where a != b, how is correlation calculated?

If correlation is calculated primarily from a and b, setCorrelation() should be removed. Period. The correlation should be calculated in the constructor or in the getCorrelation() method. One principle of OOP is to group related data and logic, so the calculation of correlation should ideally be done in the class you cleverly named Correlation. If the calculation is extremely complicated, it may be elsewhere, (e.g. DIP) but the call should be made from Correlation.

If correlation is not calculated from a and b, I really don't understand the class, so "it depends". If a does equal b, and somebody calls setCorrelation(0.0), is the contract to quietly ignore the call and leave correlation at 1.0, or to throw an exception? And, if I'm writing the calling code, I'm in a quandary, since I don't know what will happen if I try to setCorrelation(0.0) because I have no idea what a and b are, or else everyplace I make the call I'm forced to go if (getA().equals(getB())) etc... or to catch the Exception and do, er, what??? Which violates DRY and is exactly why OOP says that the logic and data should be grouped together in a class.

Upvotes: 0

jalopaba
jalopaba

Reputation: 8119

Given that correlation is a somehow/sometimes "derived" value from a and b (i.e. it is 1 if a equals b, but it might be calculated in some original way depending on (a,b), a good option could be calculate the correlation in the constructor and throw an IllegalArgumentException within setCorrelation if the vaule violates the inner logic of the object:

public class Correlation {

    private final String a;
    private final String b;

    private double correlation;

    public Correlation(String a, String b) {
        this.a = a;
        this.b = b;
        calculateCorrelation();
    }

    protected calculateCorrelation() { 
        // open to more complex correlation calculations depending on the input,
        // overriding by subclasses, etc.
        if (a.equals(b)) {
            this.correlation = 1D;
        } else {
            this.correlation = 0;
        }
    }

    public double getCorrelation() {
        return correlation;
    }

    public void setCorrelation(double correlation) throws IllegalArgumentException {
        if (a.equals(b) && correlation != 1D) {
            throw new IllegalArgumentException("Correlation must be 1 if a equals b");
        }

        this.correlation = correlation;
    }
}

Following this scheme you could also "generify" your Correlation class.

Upvotes: 1

Dilum Ranatunga
Dilum Ranatunga

Reputation: 13374

It make more sense to enforce the value during setCorrelation(...). For example,

public void setCorrelation(double correlation) {
  if (a.equals(b)) {
    if (Math.abs(correlation - 1D) > EPSILON) {
      throw new InconsistentException(...);
    }
    this.correlation = 1D;
  } else {
    this.correlation= correlation;
  }
}

I would also consider making the correlation property a nullable, where a null indicates that a correlation has not been set yet.

Upvotes: 1

Nicola Musatti
Nicola Musatti

Reputation: 18218

Back in the early days of Java getter/setter pairs were used to identify properties of beans exactly for the purpose of making it possible to define conceptual attributes implemented through computation rather than a plain member variable.

Unfortunately with the passing of time programmers have come to rely more and more on getter/setters being just accessors/mutators for underlying attributes, trend that was sort of made official with the introduction of the term POJO to identify objects that only had getters and possibly setters as methods.

On the other hand it is a good thing to distinguish objects that perform computations from objects that just carry data around; I guess you should decide which type of class you wish to implement. In your place I probably would make correlation an additional constructor argument and check it's validity there, rather than in your getter. Your Correlation cannot be a computational object, as it doesn't have enough information to perform any computation.

Upvotes: 16

K2xL
K2xL

Reputation: 10290

If I were using your class I would expect getCorrelation() to return correlation. In fact, I would probably redesign the class to have static method correlateElements(String a, String b) that returns a double.

Upvotes: 0

AHungerArtist
AHungerArtist

Reputation: 9579

I would say use something like getValue()

Upvotes: 0

cjstehno
cjstehno

Reputation: 13984

Side effects in getters and setters is generally not a great idea as it is usually not expected and can lead to tricky bugs. I would suggest creating a "correlate()" method or something else that is not specifically a getter in this case.

Hope this helps.

Upvotes: 7

Related Questions