[math] Checkstyle too stringent?

[Sébastien Brisard <se...@m4x.org>]

Hi,
I'm trying to clean up the SymmLQ class (see MATH-761 [1]).
The problem is the heart of the algorithm is very long (it would
probably amount to more thatn 600 lines). When I first implemented it,
I figured that I could split this algorithm in several methods:
init(), update(), updateNorms(), refine(), etc... This makes the code
somewhat more legible [2], with the difficulty that there is a lot of
data to be shared between these methods. That's the reason why I
created a simple nested, container class (namely, State) which holds
all the variables which are updated at each iteration. In order to
make checkstyle happy, all the fields of this nested class are
private, which means that a lot of accessors should be created with a
null benefit, since this class is nested anyway. I have to admit I
forgot to create these accessors, so synthetic accessors are
automatically generated at the present time. I'm not sure this is good
practice, but I'm also reluctant to implement all those (useless in my
view) accessors. So my question is two fold

1. Would it be acceptable to have public fields in a nested class?  If
yes, the checkstyle rules should be modified.
2. Is it good practice otherwise to let the JVM generate synthetic
accessors (which would probably get inlined anyway by modern JVMs?).

I'm currently puzzled by the SymmLQ class. I feel the need for some
cleaning up, but I do not really know where to start. So if anyone
feels like reviewing the code, I would be grateful for some help. The
good news is that unit tests are very extensive, so we can try drastic
refactoring.

Thanks for your help!
Sébastien

[1] Which is probably ill-named. Should be named "cleaning-up SymmLQ"
[2] I have to say I have mixed feelings now: I'm wondering whether a
600-hundred lines method would not be better in that case, but I would
probably not get a wide support...


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org

Re: [math] Checkstyle too stringent?

[Sébastien Brisard <se...@m4x.org>]

Le 19 mars 2012 07:11, Sébastien Brisard <se...@m4x.org> a écrit :
> 2012/3/18 sebb <se...@gmail.com>:
>> 2012/3/18 Sébastien Brisard <se...@m4x.org>:
>>> Hi,
>>>
>>> 2012/3/18 sebb <se...@gmail.com>:
>>>> 2012/3/18 Sébastien Brisard <se...@m4x.org>:
>>>>> Hi,
>>>>> I'm trying to clean up the SymmLQ class (see MATH-761 [1]).
>>>>> The problem is the heart of the algorithm is very long (it would
>>>>> probably amount to more thatn 600 lines). When I first implemented it,
>>>>> I figured that I could split this algorithm in several methods:
>>>>> init(), update(), updateNorms(), refine(), etc... This makes the code
>>>>> somewhat more legible [2], with the difficulty that there is a lot of
>>>>> data to be shared between these methods. That's the reason why I
>>>>> created a simple nested, container class (namely, State) which holds
>>>>> all the variables which are updated at each iteration. In order to
>>>>> make checkstyle happy, all the fields of this nested class are
>>>>> private, which means that a lot of accessors should be created with a
>>>>> null benefit, since this class is nested anyway. I have to admit I
>>>>> forgot to create these accessors, so synthetic accessors are
>>>>> automatically generated at the present time. I'm not sure this is good
>>>>> practice, but I'm also reluctant to implement all those (useless in my
>>>>> view) accessors. So my question is two fold
>>>>>
>>>>> 1. Would it be acceptable to have public fields in a nested class?  If
>>>>> yes, the checkstyle rules should be modified.
>>>>> 2. Is it good practice otherwise to let the JVM generate synthetic
>>>>> accessors (which would probably get inlined anyway by modern JVMs?).
>>>>
>>>> Why not put the methods and the data in the same class?
>>>>
>>>> That would eliminate the need for accessors.
>>>>
>>> That's actually what I've done: init(), update(), refine() and the
>>> likes all belong to the class State. This indeed reduces the problem,
>>> but does not remove it completely.
>>
>> As far as I can tell, you can make the State class static with very
>> little effort.
>> Just pass in the (final) fields check and delta when creating the
>> class (and store in State final fields).
>>
> Thanks for your review! I'll try something along these lines. To tell
> the truth, I've tried that already, but was not too pleased with the
> result. I can't remember why, though. You're right that making State a
> static class would be an good first step.
>
Oh! You've done that already! I'll just modify the parameter order in
the constructor of State, to be consistent with the constructor of
SymmLQ.

>> The field delta is not otherwise used by the main class so can perhaps
>> be dropped as an instance field there.
>>
> delta is inherent to the solver itself (required accuracy), not to the
> linear system to be solved. I know people might disagree with that,
> but I think that it should be kept as an instance field.
>
> Sébastien


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org

Re: [math] Checkstyle too stringent?

[sebb <se...@gmail.com>]

2012/3/19 Sébastien Brisard <se...@m4x.org>:
> 2012/3/18 sebb <se...@gmail.com>:
>> 2012/3/18 Sébastien Brisard <se...@m4x.org>:
>>> Hi,
>>>
>>> 2012/3/18 sebb <se...@gmail.com>:
>>>> 2012/3/18 Sébastien Brisard <se...@m4x.org>:
>>>>> Hi,
>>>>> I'm trying to clean up the SymmLQ class (see MATH-761 [1]).
>>>>> The problem is the heart of the algorithm is very long (it would
>>>>> probably amount to more thatn 600 lines). When I first implemented it,
>>>>> I figured that I could split this algorithm in several methods:
>>>>> init(), update(), updateNorms(), refine(), etc... This makes the code
>>>>> somewhat more legible [2], with the difficulty that there is a lot of
>>>>> data to be shared between these methods. That's the reason why I
>>>>> created a simple nested, container class (namely, State) which holds
>>>>> all the variables which are updated at each iteration. In order to
>>>>> make checkstyle happy, all the fields of this nested class are
>>>>> private, which means that a lot of accessors should be created with a
>>>>> null benefit, since this class is nested anyway. I have to admit I
>>>>> forgot to create these accessors, so synthetic accessors are
>>>>> automatically generated at the present time. I'm not sure this is good
>>>>> practice, but I'm also reluctant to implement all those (useless in my
>>>>> view) accessors. So my question is two fold
>>>>>
>>>>> 1. Would it be acceptable to have public fields in a nested class?  If
>>>>> yes, the checkstyle rules should be modified.
>>>>> 2. Is it good practice otherwise to let the JVM generate synthetic
>>>>> accessors (which would probably get inlined anyway by modern JVMs?).
>>>>
>>>> Why not put the methods and the data in the same class?
>>>>
>>>> That would eliminate the need for accessors.
>>>>
>>> That's actually what I've done: init(), update(), refine() and the
>>> likes all belong to the class State. This indeed reduces the problem,
>>> but does not remove it completely.
>>
>> As far as I can tell, you can make the State class static with very
>> little effort.
>> Just pass in the (final) fields check and delta when creating the
>> class (and store in State final fields).
>>
> Thanks for your review! I'll try something along these lines. To tell
> the truth, I've tried that already, but was not too pleased with the
> result. I can't remember why, though. You're right that making State a
> static class would be an good first step.
>
>> The field delta is not otherwise used by the main class so can perhaps
>> be dropped as an instance field there.
>>
> delta is inherent to the solver itself (required accuracy), not to the
> linear system to be solved. I know people might disagree with that,
> but I think that it should be kept as an instance field.

For some reason Eclipse showed it as unused by the parent class at one
point, but that is not the case.

> Sébastien
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org

Re: [math] Checkstyle too stringent?

[Sébastien Brisard <se...@m4x.org>]

2012/3/18 sebb <se...@gmail.com>:
> 2012/3/18 Sébastien Brisard <se...@m4x.org>:
>> Hi,
>>
>> 2012/3/18 sebb <se...@gmail.com>:
>>> 2012/3/18 Sébastien Brisard <se...@m4x.org>:
>>>> Hi,
>>>> I'm trying to clean up the SymmLQ class (see MATH-761 [1]).
>>>> The problem is the heart of the algorithm is very long (it would
>>>> probably amount to more thatn 600 lines). When I first implemented it,
>>>> I figured that I could split this algorithm in several methods:
>>>> init(), update(), updateNorms(), refine(), etc... This makes the code
>>>> somewhat more legible [2], with the difficulty that there is a lot of
>>>> data to be shared between these methods. That's the reason why I
>>>> created a simple nested, container class (namely, State) which holds
>>>> all the variables which are updated at each iteration. In order to
>>>> make checkstyle happy, all the fields of this nested class are
>>>> private, which means that a lot of accessors should be created with a
>>>> null benefit, since this class is nested anyway. I have to admit I
>>>> forgot to create these accessors, so synthetic accessors are
>>>> automatically generated at the present time. I'm not sure this is good
>>>> practice, but I'm also reluctant to implement all those (useless in my
>>>> view) accessors. So my question is two fold
>>>>
>>>> 1. Would it be acceptable to have public fields in a nested class?  If
>>>> yes, the checkstyle rules should be modified.
>>>> 2. Is it good practice otherwise to let the JVM generate synthetic
>>>> accessors (which would probably get inlined anyway by modern JVMs?).
>>>
>>> Why not put the methods and the data in the same class?
>>>
>>> That would eliminate the need for accessors.
>>>
>> That's actually what I've done: init(), update(), refine() and the
>> likes all belong to the class State. This indeed reduces the problem,
>> but does not remove it completely.
>
> As far as I can tell, you can make the State class static with very
> little effort.
> Just pass in the (final) fields check and delta when creating the
> class (and store in State final fields).
>
Thanks for your review! I'll try something along these lines. To tell
the truth, I've tried that already, but was not too pleased with the
result. I can't remember why, though. You're right that making State a
static class would be an good first step.

> The field delta is not otherwise used by the main class so can perhaps
> be dropped as an instance field there.
>
delta is inherent to the solver itself (required accuracy), not to the
linear system to be solved. I know people might disagree with that,
but I think that it should be kept as an instance field.

Sébastien


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org

Re: [math] Checkstyle too stringent?

[sebb <se...@gmail.com>]

2012/3/18 Sébastien Brisard <se...@m4x.org>:
> Hi,
>
> 2012/3/18 sebb <se...@gmail.com>:
>> 2012/3/18 Sébastien Brisard <se...@m4x.org>:
>>> Hi,
>>> I'm trying to clean up the SymmLQ class (see MATH-761 [1]).
>>> The problem is the heart of the algorithm is very long (it would
>>> probably amount to more thatn 600 lines). When I first implemented it,
>>> I figured that I could split this algorithm in several methods:
>>> init(), update(), updateNorms(), refine(), etc... This makes the code
>>> somewhat more legible [2], with the difficulty that there is a lot of
>>> data to be shared between these methods. That's the reason why I
>>> created a simple nested, container class (namely, State) which holds
>>> all the variables which are updated at each iteration. In order to
>>> make checkstyle happy, all the fields of this nested class are
>>> private, which means that a lot of accessors should be created with a
>>> null benefit, since this class is nested anyway. I have to admit I
>>> forgot to create these accessors, so synthetic accessors are
>>> automatically generated at the present time. I'm not sure this is good
>>> practice, but I'm also reluctant to implement all those (useless in my
>>> view) accessors. So my question is two fold
>>>
>>> 1. Would it be acceptable to have public fields in a nested class?  If
>>> yes, the checkstyle rules should be modified.
>>> 2. Is it good practice otherwise to let the JVM generate synthetic
>>> accessors (which would probably get inlined anyway by modern JVMs?).
>>
>> Why not put the methods and the data in the same class?
>>
>> That would eliminate the need for accessors.
>>
> That's actually what I've done: init(), update(), refine() and the
> likes all belong to the class State. This indeed reduces the problem,
> but does not remove it completely.

As far as I can tell, you can make the State class static with very
little effort.
Just pass in the (final) fields check and delta when creating the
class (and store in State final fields).

The field delta is not otherwise used by the main class so can perhaps
be dropped as an instance field there.

> Oh well, I'll try and find a way.
> Thanks anyway!

> Sébastien
>
>>> I'm currently puzzled by the SymmLQ class. I feel the need for some
>>> cleaning up, but I do not really know where to start. So if anyone
>>> feels like reviewing the code, I would be grateful for some help. The
>>> good news is that unit tests are very extensive, so we can try drastic
>>> refactoring.
>>>
>>> Thanks for your help!
>>> Sébastien
>>>
>>> [1] Which is probably ill-named. Should be named "cleaning-up SymmLQ"
>>> [2] I have to say I have mixed feelings now: I'm wondering whether a
>>> 600-hundred lines method would not be better in that case, but I would
>>> probably not get a wide support...
>>>
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org

Re: [math] Checkstyle too stringent?

[Sébastien Brisard <se...@m4x.org>]

Hi,

2012/3/18 sebb <se...@gmail.com>:
> 2012/3/18 Sébastien Brisard <se...@m4x.org>:
>> Hi,
>> I'm trying to clean up the SymmLQ class (see MATH-761 [1]).
>> The problem is the heart of the algorithm is very long (it would
>> probably amount to more thatn 600 lines). When I first implemented it,
>> I figured that I could split this algorithm in several methods:
>> init(), update(), updateNorms(), refine(), etc... This makes the code
>> somewhat more legible [2], with the difficulty that there is a lot of
>> data to be shared between these methods. That's the reason why I
>> created a simple nested, container class (namely, State) which holds
>> all the variables which are updated at each iteration. In order to
>> make checkstyle happy, all the fields of this nested class are
>> private, which means that a lot of accessors should be created with a
>> null benefit, since this class is nested anyway. I have to admit I
>> forgot to create these accessors, so synthetic accessors are
>> automatically generated at the present time. I'm not sure this is good
>> practice, but I'm also reluctant to implement all those (useless in my
>> view) accessors. So my question is two fold
>>
>> 1. Would it be acceptable to have public fields in a nested class?  If
>> yes, the checkstyle rules should be modified.
>> 2. Is it good practice otherwise to let the JVM generate synthetic
>> accessors (which would probably get inlined anyway by modern JVMs?).
>
> Why not put the methods and the data in the same class?
>
> That would eliminate the need for accessors.
>
That's actually what I've done: init(), update(), refine() and the
likes all belong to the class State. This indeed reduces the problem,
but does not remove it completely. Oh well, I'll try and find a way.
Thanks anyway!
Sébastien

>> I'm currently puzzled by the SymmLQ class. I feel the need for some
>> cleaning up, but I do not really know where to start. So if anyone
>> feels like reviewing the code, I would be grateful for some help. The
>> good news is that unit tests are very extensive, so we can try drastic
>> refactoring.
>>
>> Thanks for your help!
>> Sébastien
>>
>> [1] Which is probably ill-named. Should be named "cleaning-up SymmLQ"
>> [2] I have to say I have mixed feelings now: I'm wondering whether a
>> 600-hundred lines method would not be better in that case, but I would
>> probably not get a wide support...
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org

Re: [math] Checkstyle too stringent?

[sebb <se...@gmail.com>]

2012/3/18 Sébastien Brisard <se...@m4x.org>:
> Hi,
> I'm trying to clean up the SymmLQ class (see MATH-761 [1]).
> The problem is the heart of the algorithm is very long (it would
> probably amount to more thatn 600 lines). When I first implemented it,
> I figured that I could split this algorithm in several methods:
> init(), update(), updateNorms(), refine(), etc... This makes the code
> somewhat more legible [2], with the difficulty that there is a lot of
> data to be shared between these methods. That's the reason why I
> created a simple nested, container class (namely, State) which holds
> all the variables which are updated at each iteration. In order to
> make checkstyle happy, all the fields of this nested class are
> private, which means that a lot of accessors should be created with a
> null benefit, since this class is nested anyway. I have to admit I
> forgot to create these accessors, so synthetic accessors are
> automatically generated at the present time. I'm not sure this is good
> practice, but I'm also reluctant to implement all those (useless in my
> view) accessors. So my question is two fold
>
> 1. Would it be acceptable to have public fields in a nested class?  If
> yes, the checkstyle rules should be modified.
> 2. Is it good practice otherwise to let the JVM generate synthetic
> accessors (which would probably get inlined anyway by modern JVMs?).

Why not put the methods and the data in the same class?

That would eliminate the need for accessors.

> I'm currently puzzled by the SymmLQ class. I feel the need for some
> cleaning up, but I do not really know where to start. So if anyone
> feels like reviewing the code, I would be grateful for some help. The
> good news is that unit tests are very extensive, so we can try drastic
> refactoring.
>
> Thanks for your help!
> Sébastien
>
> [1] Which is probably ill-named. Should be named "cleaning-up SymmLQ"
> [2] I have to say I have mixed feelings now: I'm wondering whether a
> 600-hundred lines method would not be better in that case, but I would
> probably not get a wide support...
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org