2

I'd like to know if this is the preferred way to design a class; if not what's wrong with it and what's a better option?

class Calculator {

   public Calculator(Input input) {...};
   CalculatorOutput output;

   class CalculatorOutput {
   private CalculatorOutput() {} //private constructor - so no one other than Calculator can instantiate this class
   Double someCalcStuff;
   List<Double> someOtherCalcStuff;
}

    public void calculate() {
    CalculatorOutput op = new CalculatorOutput();
    //..after this function populates the someCalcStuff & someOtherCalcStuff; it just returns an instance of CalculatorOutput
}

...
return op;

}

//usage

Calculator.CalculatorOutput result = new Calculator(input).calculate();
for (Double d: result.someOtherCalcStuff) {...}

The reason I have CalculatorOutput as an inner class of Calculator is b/c it's got no other purpose or context than returning the calculator's output. I have CalculatorOutput's members as public b/c I don't really see a point in making them private , and then accessible via a getter/setter...Since I don't expect this inner class to be inherited, or do any other functionality mentioned in this post: Why use getters and setters?

The user cannot instantiate the inner class (private constructor), so I don't see a real drawback in creating public accessible members of the inner class (CalculatorOutput)

Specific questions:

  • Is this approach bad? why?
  • What's a better approach to achieve this?
Community
  • 1
  • 1
labheshr
  • 2,858
  • 5
  • 23
  • 34
  • I don't see the need for an inner class at all – OneCricketeer Apr 10 '16 at 14:18
  • To avoid having CalculatorOutput inside the Calculator class, you could declare CalculatorOutput with no access (for example, class CalculatorOutput {}) and you would have Calculator and CalculatorOutput on the same package. Then only classes inside that package can access CalculatorOutput . I am talking about the access levels in Java. You can read more here: https://docs.oracle.com/javase/tutorial/java/javaOO/accesscontrol.html – Erick Apr 10 '16 at 14:25
  • 1
    I think this question is a better fit for codereview.stackexchange.com. Take a look at the [codereview on-topic help page](http://codereview.stackexchange.com/help/on-topic). You'd probably get more useful feedback over there. – DaoWen Apr 10 '16 at 14:25
  • @Erick: I understand package-protected is another option..thanks...@DaoWen: is there a way i can "cross-post" this to stackexchange codereview (and possibly close the post from here?) thanks – labheshr Apr 10 '16 at 14:29

1 Answers1

0

To make the code cleaner, what I would do is create a Calculator package and add the following classes into that package:

  1. Calculator
  2. CalculatorOutput

And I would make the following changes: (make members private and provide getters and setters where needed)

class Calculator {
    private CalculatorOutput output;

    public Calculator(Input input) {/* logic here */} 

     // getters and setters
}

class CalculatorOutput {

    private Double someCalcStuff;

    private List<Double> someOtherCalcStuff;

    // getters and setters

    CalculatorOutput() {} 
}

public class Test{

    public static void main(String [] args)
    {
        Calculator calc = new Calculator();

        CalculatorOutput output = new CalculatorOutput();

        // use methods as needed
    }
}
Erick
  • 823
  • 16
  • 37