Hi. > [...] > >>> ---CUT--- > >>>> BrentSolver mySolver = new BrentSolver(); > >>>> > >>>> // initialize GUI > >>>> solverGui.setRelAccuracy(mySolver.getRelativeAccuracy()); > >>>> solverGui.setAbsAccuracy(mySolver.getAbsoluteAccuracy()); > >>>> > >>>> // fire the GUI > >>>> ... > >>>> > >>>> // in the action callback from the GUI OK button, create the solver > >>>> double rel = solverGui.getRelAccuracy(); > >>>> double abs = solverGui.getAbsAccuracy(); > >>>> > >>>> mySolver = new BrentSolver(rel, abs); > >>> ---CUT--- > >> > >> Creating two solvers to use only one and simply hiding constants seems > >> awkward to me. Just letting the constants visible is simpler. > > > > You don't use only one. And, arguably, for the GUI, the first solver > > instance is the most important because it provides the needed information. > > > > What bothers me most with the variables is: > > 1. The redundancy: > > The value is stored once in the static variable of the class and a > > second time in each instance. > > I don't understand. This is exactly what you have already done. The > current implementation only changed the static variables from public to > private. There are still independent class variables in each class and > independent instant variables in each instance (inherited from the > abstract class).
Well that's true. But a "private" named constant is only used internally (to make the code clearer), and we can assume that the coder of that class will indeed use the right constant in order to initialize that class; that's not really a problem. What I don't like is that the constant is defined in another (parent) class and that it is public; the value can then accessed in two ways: the static field and the setter. That's what I meant by "redundant". > > 2. The possible inconsistency: > > Nothing prevents a (sub)class to *not* use the default. So the > > documentation could be misleading whereas the accessor will always > > provide the actual value. > > Of course a subclass can use something else, this is a main feature of > subclassing: adapting behavior. Also regardless of how the default value > is defined (once publicly in a top level class or several time privately > in each subclass), the user can always choose to build his builder with > non default values. And that will lead to the inconsistency problem below... > The accessors are not here to provide defaults, they > are here to provide the actual values. Yes. That's exactly what I said; the GUI (or whatever use-case) should be sure to get the actual default value of the specific class at hand, not a possible default suggested by a base class. > What we are discussing here is not about the actual values that are > stored in the instances. We are discussing about the default constant > values which *may* be used to initialize the actual ones. They are > simple constants, they are currently all the same and they are > duplicated 8 times. Of course, that's not pretty; but as a matter of principle, these constants might not have the same meaning (although they can have the same value). What I stress is that they should be tied to the specific class, not to the base class. > > > > GUIs _are_ awkward. Having to create two solvers in such a context is > > negligible and a very small price to pay for ensuring consistency. > > > >> I think what Sebb pointed out in this issue was not that the constants > >> were public, but that these constants were duplicated. I agree with him > >> here. However solving the issue by removing the duplication and having > >> constants defined in the top level interface is more straightforward. > > > > There are 2 problems with this: > > 1. The top-level class where such "public" constants should be defined is > > "BaseAbstractUnivariateRealSolver". This is not a "user" class but a > > "developer" class, an implementation detail. Encouraging users to have > > that class appear in their code is not good design. > > Then we can put them in the UnivariateRealSolver interface. Interfaces > can have constants and are a great place to store them in fact. The above problem of principle is the same; I think that it is not appropriate to define a variable without knowing what it actually means. What would be the comment associated of, say, DEFAULT_ABSOLUTE_ACCURACY. Whatever you decide here can be wrong in some implementations. Hence it is wrong at the interface level. > > 2. The "accuracies" might mean different things for different solvers and > > thus the defaults might be set to different values depending on the > > implementation. [That is why I had created a static variable for each > > class.] > > If a specific implementation needs a different value, then it can use > its own default, either by having its own constant (it cannot reuse the > name from the top interface here). However, for now it is clearly not > the case. There are 8 implementations and 8 times the exact same value > (1.0e-6). As said above, the numeric value is not the problem. > > > > Please note also that by using the accessor, users can code through > > interface while with the static constant, their code must reference a > > concrete class. > > No, an interface can define a constant. I didn't know, but that doesn't change my opinion... :-} IMO, if some constructor uses default values for some parameters, they should be documented at that point, ideally with the reason why this a good default or how the user can choose some "good" value. [See e.g. the comment for the constructor of "BrentOptimizer", which I've reproduced from Brent's book.] I fear that anything else can be either misleading or downright wrong. I just advocate for staying on the safe side. Regards, Gilles --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
