51 lines
1.8 KiB
Plaintext
51 lines
1.8 KiB
Plaintext
=== on 29 May 2015, 08:46:00 Massimo PALADIN wrote:
|
|
\[~ann.campbell.2] could you please review this java sub-task?
|
|
|
|
=== on 29 May 2015, 14:52:26 Ann Campbell wrote:
|
|
looks good [~massimo.paladin]
|
|
|
|
=== on 4 Jun 2015, 13:10:24 Massimo PALADIN wrote:
|
|
@Ann I added an exception to this rule, could you validate the change?
|
|
|
|
=== on 4 Jun 2015, 14:24:58 Ann Campbell wrote:
|
|
looks good [~massimo.paladin]
|
|
|
|
=== on 16 Aug 2018, 00:22:34 Michal Domagala wrote:
|
|
Hello,
|
|
|
|
I am interested in justification and history of this issue. Basically, this rule looks very strange and I would like to know how this concept was invented.
|
|
|
|
What I found:
|
|
|
|
1. Initially, someone figured that methods, which do not access instance fields should be static, but didn't explained why.
|
|
|
|
2. Next other someone realized that the rule is wrong for non-private methods. So the rule is implemented only for private methods, but without explanation why
|
|
|
|
3. I also found that initially someone expected that the rule has (among others) performance meaning, but someone later realized that it is not true. I think it was a good moment to review sense of the rule, but the review was not performed.
|
|
|
|
|
|
I applied the rule to joda time library and 19 code smells was detected.
|
|
|
|
|
|
Do you think that code
|
|
|
|
----
|
|
/**
|
|
* Checks whether the period is non-null.
|
|
*
|
|
* @throws IllegalArgumentException if the period is null
|
|
*/
|
|
private void checkPeriod(ReadablePeriod period) {
|
|
if (period == null) {
|
|
throw new IllegalArgumentException("Period must not be null");
|
|
}
|
|
}
|
|
----
|
|
|
|
smells bad and ``++private static void checkPeriod(ReadablePeriod period)++`` smells good?
|
|
|
|
|
|
I think that the rule was based on wrong assumption, was heavy modified without review the sense and encourages to bad style coding. Effectively, it is anti-rule and should be deleted.
|
|
|
|
include::../comments-and-links.adoc[]
|