23

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?

eliocs
  • 18,511
  • 7
  • 40
  • 52
  • I also dont like this approach because in Kotlin we use property access syntax and kotlin is interoperable with Java. So people might also do mistakes if in future code is migrated to kotlin from java – firstpostcommenter Jul 28 '22 at 15:37

7 Answers7

16

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.

Nicola Musatti
  • 17,834
  • 2
  • 46
  • 55
  • +1; this is the whole truth! It's important to distinguish between real object and dumb data transfer objects. – home Oct 31 '11 at 15:24
  • I agree except for the last sentence. I think Correlation can be a computational object since it (at least) can calculate correlation when a equals b. – jalopaba Oct 31 '11 at 15:55
  • @Jala: Correlation isn't something that's 1 when a and b are equal and undefined otherwise: one thing is to check arguments and another is to calculate a function of said arguments. – Nicola Musatti Oct 31 '11 at 16:04
  • Even if the logic was defined on another class would you add the value constraint on the setter method? – eliocs Oct 31 '11 at 16:18
  • On second thoughts, probably no. That logic belongs where correlation is actually calculated and this test is both very partial and likely redundant. On the other hand, if you have to have it, that's the best place to put it ;-) – Nicola Musatti Oct 31 '11 at 18:25
7

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.

cjstehno
  • 13,468
  • 4
  • 44
  • 56
  • 1
    I don't see a side effect in his second getter. One exception, the first getter might give a rounding error. But that would make the second getter preferable. – S.L. Barth is on codidact.com Oct 31 '11 at 15:05
  • Agreed. In general, the best practice is to have a method do one thing only if possible and avoid side effects at all costs. Think of it as the Single Responsibility Principle applied to the method level. – Sam T. Oct 31 '11 at 15:07
  • i agree that putting logic in getters/ setters is typically not a great idea, but you may need some side effects at times - e.g. a user object may encrypt/ decrypt password in a setter/ getter – aishwarya Oct 31 '11 at 15:10
  • @aishwarya agreed good point, to clarify my answer, side-effects or operations performed in setters/getters should be paired, to use your example... if you encrypt the value on set, you would decrypt on get. – cjstehno Oct 31 '11 at 16:06
1

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.

Dilum Ranatunga
  • 13,254
  • 3
  • 41
  • 52
1

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.

jalopaba
  • 8,039
  • 2
  • 44
  • 57
0

I would say use something like getValue()

AHungerArtist
  • 9,332
  • 17
  • 73
  • 109
0

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.

K2xL
  • 9,730
  • 18
  • 64
  • 101
0

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.

user949300
  • 15,364
  • 7
  • 35
  • 66