0

I have a Circle class type of object for which I have a member 'r' to store the radius. Now because I use these objects a lot, I don't want to make the radius private because other classes would to have to use a getR() method and it would make the code hard to read. For instance it's much easier to read code like this: double alpha = b1.r + b2.r -2*b1.r*b2.r; than to read this: double alpha = b1.getR() + b2.getR() - 2*b1.getR()*b2.getR();

Yet, because this is a radius and its square is also used a lot and I want to compute it once, I woud like to have a setter for the radius, so I can update both r and its square at the same time. So I want r to be private to force the use of setR() but I want it publc so it can be referenced for reading. Does anyone know of a way to do this in Java ?

jdbertron
  • 733
  • 7
  • 11
  • I would suggest go with the getter/setters and include a comment on top which gives the intended calculation i.e. b1.r + b2.r -2*b1.r*b2.r So you will not sacrifice on the readability. It is a good practice to keep your attributes private and use accessors for reading and writing – Rohit Mar 10 '13 at 05:29
  • You could switch to Scala – Antimony Mar 10 '13 at 05:36

4 Answers4

4

Java doesn't have the notion of read-only properties.

If you allow r to be readable, someone can and will change it randomly. Besides, it's a bad habit to get into. Just don't. The longshot example would be: what if you needed this to function in some odd non-euclidean space? Now you have to go unfurl the mess anyhow to fix it.

Realistic options:

You could deviate from the norm and have your getter be named r(). Almost as short.

Or, you could push such math into the circle class, where it can refer to its members directly. You get the better part of encapsulation with the shorter names.

class Circle() {
   static double alpha(Circle c1, Circle c2) {
      return c1.r + c2.r -2*c1.r*c2.r
   }
   ...
}
James
  • 8,512
  • 1
  • 26
  • 28
  • YAGNI. In the rare case that you do need a Non Euclidean space, you'll probably have to make much larger changes to the code base anyway. – Antimony Mar 10 '13 at 05:37
  • 2
    Agreed, it's horrible example. The point is that it's a bad habit to break encapsulation because you're feeling lazy and want to save a few characters. Especially given the explicit requirement of not allowing arbitrary changes to r. I don't know enough of what jdberton is doing to offer a real example of where this can come to bite them in the ass later. Can you get away with doing it on trivial things where you're never really going to add on to it? Probably. Is it going to come back to bite you later when you bring that bad habit on to a bigger project? Probably as well. – James Mar 10 '13 at 05:42
  • 1
    It's only really an issue if it's part of a Public API. Otherwise, you can easily refactor as necessary. – Antimony Mar 10 '13 at 05:46
  • 1
    As soon as you make r public, and let anyone else touch it, you've effectively made it part of your API. If you have control of all code touching it, sure, you can change it as necessary - but the farther you let r leak, the more code you'll have to touch to refactor. – James Mar 10 '13 at 06:07
  • I think James is correct. It's definitely a bad choice, but it's not really to save characters. It's to make several other lines of math much easier to read and understand, now and 5 years from now when I'll look at it again. For the same reason, I use greek variables (you can do that with eclipse) so it's clearer and matches the research paper. That, I think does much more to protect my project from insidious bugs. So, for now I'll go with the r() getter, and make r private. – jdbertron Mar 20 '13 at 03:35
1

a). Don't skip using getters and setters to provide encapsulation to your code.

b). If readability is your issue, then I suggest get the radius using getR() method and store in local variables. such as - int firstCircleRadius = first.getR() and similarly for other circles and make the formula easily readable using your local variables.

c). Don't use r as a property name. Use radius instead which by itself increases readability.

John Jai
  • 3,463
  • 6
  • 25
  • 32
0

I personally would use getters and setters. However you could have the real radius stored in a private member and then use you setR() method to update the dummy radius r. This doesn't fix the problem of being able to change r manually.

private int radius;
public int r;
public void setR(int r) {
    radius = r;
    square(); //do you squaring and other math
    this.r = r;
}
Breavyn
  • 2,232
  • 16
  • 29
0

I have been burnt by this issue recently, and at the risk of getting a bunch of negative votes and wrath of the Java community, I would suggest NOT to rely on Java access - there is nothing private about a class variable or a method (or what I used to think "private" to be for encapsulation in an Object Oriented Programming language. Just use public for everything and sleep well at night. Here is an explanation, Consider the following class:

package hiddenscope;

public class Privacy {
    private int privateInt = 12;
    private double oneBy2pi = 0.5 / Math.PI;
    private String myClassName;

    public Privacy () { 
        privateInt = 12;
        myClassName = this.getClass().getName();
        print ("Constructor");
    }

    // Given the circumference of a circle, get its radius
    public double c2r (double c) {
        return c * oneBy2pi;
    }

    // Do not let any other class call this method
    private void print (String caller) {
        System.out.println ("["+caller+"] Current value of privateInt in class " + 
                                                myClassName + " is " + privateInt);
    }
}

Here is a class that changes the private variable privateInt of the above class and calls it private "print" method, even from outside the package (protected, anyone?):

package userpackage;
import hiddenscope.Privacy;

import java.lang.reflect.*;


public class EditPrivate {

    private int i2set;
    private String fieldNameToModify, myClassName;
    private Object p;
    private Method printer;

    public EditPrivate (Object object, String fName, int value) {
        fieldNameToModify = fName;
        i2set = value;
        p = object;
        myClassName = this.getClass().getName();
        Method[] methods = p.getClass().getDeclaredMethods();
        for (Method m : methods) {
            if (m.getName().equals("print")) {
                printer = m;
                printer.setAccessible(true);
                break;
        }}

        if (printer == null)
            return;

        try {
            printer.invoke (p, myClassName);
        } catch (IllegalAccessException ex1)        {   ex1.printStackTrace();
        } catch (InvocationTargetException ex2)     {   ex2.printStackTrace();
    }}

    public void invadePrivacy () throws Exception {
        Field[] fields = p.getClass().getDeclaredFields();
        for (Field field : fields) {
            String fieldName = field.getName();
            if (fieldName.equals(fieldNameToModify)) {
                field.setAccessible(true);
                if ("int".equals(field.getType().toString())) {
                    try {
                        field.setInt(p, i2set);
                        if (printer != null)
                            printer.invoke (p, myClassName);
                        return;
                    } catch (IllegalAccessException ex) {
                        ex.printStackTrace();
        }}}}
        throw new Exception ("No such int field: " + fieldNameToModify);
    }

    public static void main(String[] args) {
        try {
            Privacy p = new Privacy();
            new EditPrivate(p, "privateInt", 15).invadePrivacy();
        } catch (Exception ex) {
            ex.printStackTrace();
}}}

To add insult to this injury, this is what happens where multiple threads mess up the internal and private variables of a class (oneBy2pi is supposed to be private!)

package userpackage;
import java.lang.reflect.Field;
import java.util.concurrent.*;
import java.util.*;

import hiddenscope.*;

public class ThreadedPi implements Runnable {

    Privacy p = new Privacy();
    private int nSample = 2000, bins[] = new int[4]; // 94814117 999065 0003379, 2028859
    private Field c2dField;
    private boolean mutate;

    public ThreadedPi () {
        Field[] fields = p.getClass().getDeclaredFields();
        for (Field field : fields) {
            String fieldName = field.getName();
            if (fieldName.equals("oneBy2pi")) {
                field.setAccessible(true);
                c2dField = field;
    }}}

    synchronized private boolean setMutationOfField (boolean master, boolean value) {
        if (master)
            mutate = value;
        return mutate;
    }

    public void run () {
        Random rng = new Random();
        for ( ; ; ) {
            if (setMutationOfField (false, true)) {
                int nDigit = 2 + rng.nextInt(4);
                double doublePi = 4.0 * Math.atan(1.0),
                        decimal = Math.pow(10, nDigit),
                        apprxPi = Math.round(decimal * doublePi) / decimal;
                try {
                    c2dField.setDouble (p, 0.5/apprxPi);
                } catch (IllegalAccessException ex) {
                    ex.printStackTrace();
            }} else {
                return;
    }}}

    public void execute () {
        setMutationOfField (true, true);
        new Thread (this).start();
        Executed[] class2x = new Executed[nSample];
        ExecutorService executor = Executors.newCachedThreadPool();
        for (int i = 0 ; i < 4 ; i++)
            bins[i] = 0;
        for (int i = 0 ; i < nSample ; i++) {
            class2x[i] = new Executed();
            executor.execute(class2x[i]);
            double d = class2x[i].getDiameter(-1);
            if (d < 399.99)         bins[0]++;
            else if (d < 400)       bins[1]++;
            else if (d < 400.01)    bins[2]++;
            else                    bins[3]++;
            //System.out.println ("d: " + d);
        }
        setMutationOfField (true, false);
        for (int i = 0 ; i < 4 ; i++)
            System.out.print("\t\t[" + (i+1) + "] " + bins[i]);
        System.out.println ();
    }

    public static void main(String[] args) {
        ThreadedPi tp = new ThreadedPi();
        for (int k = 0 ; k < 5 ; k++)
            tp.execute();
    }

    private class Executed implements Runnable {
        private double d=-1, c = 2513.274123;

        synchronized double getDiameter (double setter) {
            if (setter < 0) {
                while (d < 0) {
                    try {
                        wait();
                    } catch (InterruptedException ex) {
            }}} else {
                d = setter;
                notify();
            }
            return d;
        }

        public void run () {
            getDiameter (p.c2d(c));
}}}

All the above classes compile and run perfectly, which explains why "private" may not have any meaning in java. Shields up, Scotty!

Manidip Sengupta
  • 3,573
  • 5
  • 25
  • 27
  • Well of course you can do anything with SetAccesible. You could do the same with Native code. You shouldn't expect language invariants to be enforced on mechanisms specifically designed to get around them. If you're worried about security, you need to use SecurityManagers. – Antimony Mar 10 '13 at 13:06
  • It is not about my expectations, and I have not used anything low level in the code above - these features are either part of Java or they are not. Unfortunately, it looks like they are, and frameworks are around that actively use it. You must agree that this is a clear violation of the basic OO paradigm of encapsulation, and we have to live with it. – Manidip Sengupta Mar 10 '13 at 23:54
  • You're calling Java functions, but those functions ultimately call native code. Therefore, they are free to do whatever they want (in this case break encapsulation). – Antimony Mar 11 '13 at 00:47