On Sun, 26 Jun 2022 at 20:52, Sumanth Rajkumar <rajkumar.suma...@gmail.com> wrote:
> Hi, > I have raised PR #113 after rebasing to the master branch with Alex's > checkstyle changes > > As per feedback, I have made the following changes > a) Added javadoc comments > b) Ensured test coverage > c) Renamed accessors on the interface > Thanks for the changes. Note that a new PR is not required. You can simply force push changes to the previous PR. It is covering the same subject. I've not yet fully read the PR. However the level of abstraction on some of the simple functions seems excessive. Many of the scalar operations using applyScalarFunction are one liners that have been abstracted to multiple layers of function references. Simple operations such as add, subtract, conjugate, negate, arg (defined as Math.atan2) may be better left alone. They can be duplicated into the complex functions class if the API is for public consumption but performance may be impacted by the abstraction. The code is definitely made less readable. Also note that you have some double empty lines which should be a single empty line and then some functions ending with } and no empty line after. These can be simply fixed using a regular expression to search for them. I note that some javadoc is missing for private methods. I have not set checkstyle to enforce this and the default scope is public. It should at least be protected, but my preference would be package. I will see if the rest of the project is OK for this and then update the rule. > > > > Gilles Sadowski <gillese...@gmail.com> wrote: > > In "DComplex", I propose that the accessors be named "real()" and > > "imag()" (or just > > "re()" and "im()"). ["DComplex" is not a very satisfying name either...] > > For the interface name, shall I change it to Complex64 from DComplex? > In c the 'complex' keyword is a suffix: double complex c1; float complex c2; long double complex c3; In c++ the type is generic (and read as a suffix): complex<double> c1; complex<float> c2; Either of these would be my preference over DComplex or Complex64. > > Are we sure that all this code needs to be part of the public API? > > If not, I'd suggest limiting accessibility to "package-private". > > Are you referring to the static methods in ComplexFunctions and > ComplexBiFunctions classes? > I think they would need to be public for developers to be able to compose > multiple operations... > The static helper functions have been extracted to support all the ISO c99 operations on the list structure of complex numbers. A list will ideally implement a generic foreach operation. So to apply a single function only requires making the static functions public. The alternative is to make the list expose all the ISO c99 operations in its public API. To create a composite function that eventually writes back to the list can be implemented by writing intermediate values to a result which is then passed to the next operation. This can be satisfied by using the Complex class. This already exposes all the ISO c99 functions. So perhaps it is not required to make all the helper functions public for the purpose of composing multiple operations. But it would be helpful for all the single operations. > > Thanks, > Sumanth > > PS: Noticed master branch unit test failures in numbers-fraction module > This has been fixed. Sorry for the mistake. Alex
