16f6c0aecf
Inline adoc files when they are included exactly once. Also fix language tags because this inlining gives us better information on what language the code is written in.
113 lines
3.2 KiB
Plaintext
113 lines
3.2 KiB
Plaintext
== Why is this an issue?
|
|
|
|
Non-overridable methods (``++private++`` or ``++final++``) that don't access instance data can be ``++static++`` to prevent any misunderstanding about the contract of the method.
|
|
|
|
=== Noncompliant code example
|
|
|
|
[source,java]
|
|
----
|
|
class Utilities {
|
|
private static String magicWord = "magic";
|
|
|
|
private String getMagicWord() { // Noncompliant
|
|
return magicWord;
|
|
}
|
|
|
|
private void setMagicWord(String value) { // Noncompliant
|
|
magicWord = value;
|
|
}
|
|
|
|
}
|
|
----
|
|
|
|
=== Compliant solution
|
|
|
|
[source,java]
|
|
----
|
|
class Utilities {
|
|
private static String magicWord = "magic";
|
|
|
|
private static String getMagicWord() {
|
|
return magicWord;
|
|
}
|
|
|
|
private static void setMagicWord(String value) {
|
|
magicWord = value;
|
|
}
|
|
|
|
}
|
|
----
|
|
|
|
=== Exceptions
|
|
|
|
When ``++java.io.Serializable++`` is implemented the following three methods are excluded by the rule:
|
|
|
|
* ``++private void writeObject(java.io.ObjectOutputStream out) throws IOException;++``
|
|
* ``++private void readObject(java.io.ObjectInputStream in) throws IOException, ClassNotFoundException;++``
|
|
* ``++private void readObjectNoData() throws ObjectStreamException;++``
|
|
|
|
ifdef::env-github,rspecator-view[]
|
|
|
|
'''
|
|
== Implementation Specification
|
|
(visible only on this page)
|
|
|
|
include::../message.adoc[]
|
|
|
|
'''
|
|
== Comments And Links
|
|
(visible only on this page)
|
|
|
|
=== 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[]
|
|
|
|
endif::env-github,rspecator-view[]
|