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

Reply via email to