Friday, September 30, 2011

PMD - Priorities

There are so many rules in PMD. One can get overwhelmed by the rules and can be unsure of which rules to give priority to. PMD fortunately gives priority to each of the rules that it define. But it does not elaborate on why the priority. This post has tried re-prioritize the rules and has given reasons as to why the particular priority for the rule. It has not covered certain sections which which the author is not familiar or considers to be not used much in the industry. Hope it helps. Please do provide your feedback on the new priorities and put in your priorities if you have a different one with reasons.
The goal of setting the priority in this fashion is to make the code simpler, avoid bugs and pay attention to performance.


Priority
Meaning
1
Must follow
2
May indicate problems which can lead to instability or performance issues
3
Impacts Readability
4
Redundant Code



Rule
Priority
Default Priority
Reasoning
1
It is very important to carry out the copy paste checks and eliminate duplication of code. This will lead to cleaner and better maintainable code. This will also eliminate lot of errors when the code is changed for fixing a bug or for enhancing the application.
1
3
Presence of Empty catch blocks indicates poor exception handling and this can lead to unforeseen program crashes
4
3
It will not impact the program in any way and can be safely ignored.
2
3
While it may look similar to the EmptyIfStmt we may end up with infinite loops and so this must be included
3
3
It will not impact the program in any way and can be safely ignored.
2
3
Empty finally blocks may indicate that the resources are not being closed ; and not closing resources will lead to applications not performing well. So this needs to be addressed at least a second priority
3
3
It will not impact the program in any way and can be safely ignored.
2
3
It is better to check for these upfront as this would in most scenarios be a bug. It is very rare scenarios where this situation would be genuine.
3
3
This only reduces the readability. It will not impact the functionality of the program.
3
3
This does not impact the stability of the program, it can cause performance problems if this is present in a frequently executed method.
1
3
This can cause unforeseen problems in the system and this error should not be ignored. This is not something that is understood by many and it makes sense to include this check.
1
1
This can cause problems in highly active systems.  It is better to be prepared for such a situation rather than assuming that it will not happen.
1
3
It is better to check for such scenarios and eliminate them rather than wait for situations to occur where this is exercised and we face a surprise.
3
3
This will not create a problem or instability in the system, but in systems where performance is key we should not ignore this as this can cause sufficient delays in the code execution. It will take a higher priority in applications where latency matters.
4
3
It will not impact the program in any way and can be safely ignored.
4
3
It will not impact the program in any way and can be safely ignored.
3
3
It is difficult to maintain code where this is used as a principle. If there are few occurrences it may be OK, but too many of these can cause a maintainability issue. This was old style of commenting out unwanted code when one was unsure of deleting it.
3
3
Should not normally cause a problem in the execution logic so this need not be given very high priority.
2
2
This should be eliminated as this can cause GC problems especially in applications where the occurrences are high.
4
3
It will not impact the program in any way and can be safely ignored.
3
3
These do not impact the performance or the functioning of the program in any significant way and can be given lower priority
3
3
These do not impact the performance or the functioning of the program in any significant way and can be given lower priority
2
3
It is better to follow this rule to eliminate any class cast exceptions and it also makes a code a little more efficient.
1
3
This should be followed if one is particular about precision. Usage of numbers instead of strings in the constructor can lead to problems normally associated with doubles and floats.
1
3
These can potentially have come about because of the misunderstanding of the developer. It is better to check for these problems and solve them.
1
3
These are bugs and this rule will prevent these bugs.
1
3
This can detect possible NullPointerExceptions. It is better to keep this Rule on
1
3
This check should be on so that one does not end up making the mistake of using ThreadGroups which is not a thread safe object.
1
2
This can detect possible NullPointerExceptions. It is better to keep this Rule on
2
3
One can give this a lower priority to this as it does not impact the stability in a big way. It can cause memory problems so it is better to look at it. It needs to be given a higher priority where BigIntegers are used in a big way.
1
3
Usage of octal values is no longer a normal practice and it is likely that the person has used it erroneously. It is better to check for this upfront rather than delay it.
1
3
Although in the strictest of sense it does not impact the application in any way it is better to address this upfront as one is bound to have problems going forward.
1
3
It is better to do this as one can end up in situations where the application will fail because this check is not done.
1
2
This in theory does not cause a problem in the system if written correctly, but it will be very few developers who can write this code correctly and understand it correctly. So in most cases it would be an error. It is better to eliminate them right at the start.
4
3
It will not impact the program in any way and can be safely ignored.
2
3
This in theory does not cause a problem in the system but it is better to enforce this and get the developers habituated to using braces for all cases.
2
3
This in theory does not cause a problem in the system but it is better to enforce this and get the developers habituated to using braces for all cases.
2
3
This in theory does not cause a problem in the system but it is better to enforce this and get the developers habituated to using braces for all cases.
2
3
This in theory does not cause a problem in the system but it is better to enforce this and get the developers habituated to using braces for all cases.
2
2
Be sure of what and how one should to clone. Otherwise one can end up with surprises. Cloning is not understood by all and improper implementation can lead to situations where values are changed without one’s knowledge. It will also be difficult to trace out the problem as things can change from any one instance of cloned object.
2
3
Must follow convention.
2
3
Must follow convention.
1
3
Usage of this rule will ensure that the programs do not become complex. The default of 200 paths is reasonable.
1
3
Usage of this rule will ensure that the programs do not become complex. The default of 100 lines of code is reasonable.
1
3
Usage of this rule will ensure that the programs do not become complex. The default of 10 parameters is reasonable.
1
3
Usage of this rule will ensure that the programs do not become complex. The default of 1000 lines of code is reasonable.
1
3
Usage of this rule will ensure that the programs do not become complex. The default of 10 decision points is reasonable.
1
3
Usage of this rule will ensure that the programs do not become complex. The default of 45 public may be less if one considers an Object Relational Mapping environment. One could possibly increase this to 100 to get a reasonable assessment.
1
3
Usage of this rule will ensure that the programs do not become complex. The default of 15 fields may be less if one considers an Object Relational Mapping environment. One could possibly increase this to 50 to get a reasonable assessment.
1
3
Usage of this rule will ensure that the programs do not become complex. The default of 100 lines of code is reasonable.
1
3
Usage of this rule will ensure that the programs do not become complex. The default of 1500 lines of code is reasonable.
1
3
Usage of this rule will ensure that the programs do not become complex. The default of 100 lines of code can be further reduced to 50 as number of lines of code in constructors should ideally be as small as possible.
1
3
Usage of this rule will ensure that the programs do not become complex. The default of 10 methods seems to be on the lesser side. One should consider increasing this to about 120 considering the getters and settings that one would end up writing.
3
3
This will not cause any problem to the stability of the applications can be safely given a lower priority
2
3
Although this looks like an innocuous problem, it can indicate lack of knowledge of the developer. It is better to eliminate them.
3
3
This is definitely a controversial rule. One is seeing trends of moving away from this rule which was quite popular when C/C++ ruled roost. It is better to give this a lower priority considering that it does not cause any stability issues.
4
3
This does not cause any problem and sometimes can add to the understanding of the program for a junior programmer. It is better to give this a very low priority.
1
3
The usage of such a syntax can cause confusion and can also be in error unless on is very sure of what is doing. It is better to eliminate these upfront
4
3
This is definitely another controversial rule. We can ignore this rule safely without having any problems.
2
4
It is better to implement this rule and not let the junior programmers cause problems by importing these packages.
1
3
Usage of octal values is no longer a normal practice and it is likely that the person has used it erroneously. It  is better to check for this upfront rather than delay it.
2
3
This is a useful rule to have to avoid making mistakes which can go undetected. It should be given a higher priority in applications which use inheritance in a big way.
3
3
It does not harm the application, but can confuse the reader.
2
3
It is better not to have any classes in the default package.
3
3
It does not impact the functionality of the programs but can cause performance issues. So it can be given a lower priority unless it is used in too many places.
3
5
These can be given lower priority as the ones not detected by compiler are harmless.
3
3
Does not cause any problem, but makes sense to eliminate them with a lower priority
1
1
Better to include this and totally eliminate usage of Short variables.
1
2
Very few people understand the exact working of volatile variables especially since the definition/interpretation has changed over different versions of JDK. It is better to discourage usage of this keyword. When used it should used under the supervision of somebody who understands the implications.
2
2
In some cases usage of native code is unavoidable. Where required it should be used with precaution.
1
3
This definitely must not be attempted. It is better to stay away from this.
2
2
Although one can render this call redundant by passing the right parameters to the JVM at startup, it is better to discourage usage of this.
1
3
Usage of this rule will ensure that the programs do not become tightly coupled. The default of 20 distinct objects is fine.
1
3
Usage of this rule will ensure that the programs do not become tightly coupled. The default of 30 distinct objects is fine.
1
3
Usage of this rule will ensure that the programs do not become tightly coupled. It is very important to follow this rule.
4
3
This is not really necessary. Instead create all methods as static and have a private constructor to avoid the warning. Do not make a singleton. Creation of a singleton object is error prone and the syntax of invocation of these methods because longer as in .getInstance().(); instead of .method() if the method were static.
3
3
This may not may not be a good idea depending on the developer levels.  It can be given lower priority. It will not impact the performance or stability of the code.
2
3
This does not cause any functionality problem, application of this rule will push the users towards naming the methods correctly.
2
3
This is a good to have rule. There can be situations where one may not need a default label for the switch statement.
1
3
It is better to discourage developers from building deeply nested ifs and causing readability problems. It will force them to learn to create functions. The default value of 3 is fine.
3
2
This is usually safe in most scenarios and hence can be given a lower priority.
1
3
Implementation of this rule will improve the readability of the code and will force creation of smaller functions/methods. The default value of 10 is fine.
1
1
This check must be made as it can cause errors which will be difficult for the developers to identify.
2
3
It is better to avoid inner classes unless really essential. This rule can lead to elimination of inner classes.
2
3
In most cases final fields should be static. It is better to have this rule one and save some memory.
1
3
Extremely important and must be included. The opened resources have to be closed.
1
3
Good to avoid. These are not features understood by many and so should not be used by many. J
1
3
Good to avoid. These are not features understood by many and so should not be used by many. J
1
3
Good to avoid. These are not features understood by many and so should not be used by many. J
1
3
This must be used as it will bring some efficiency.
1
3
One should not be in a situation where one has to check for NaN.
1
1
This is again a bug and this rule should be enabled to avoid this problem.
1
3
Although ternary operator works as efficiently as other mechanisms they are not the easiest to understand. It is better to completely avoid usage of this operator.
2
4
This will reduce creation of objects.
2
3
We should use this rule to eliminate bad, redundant code.
1
3
It is good to follow this rule to prevent any surprises in date formatting.
2
3
This does not add much value as many do not understand immutable classes. This can be given a lower priority.
1
3
This is important in applications that are to be internationalized. Most developers do not understand internationalization. It is better to include this rule to avoid surprises.
4
3
This does not cause any problem. It is better to give this a very low priority.
2
3
This can create hidden bugs, especially in a multi-threaded application. It is better to identify these upfront and not let the situation occur
3
3
These classes end up being redundant classes. These will not cause too much problem and can be given a lower priority.
1
3
Synchronizing the method causes major performance issues in the system. It is good to void synchronizing methods. Instead one should synchronize with “this” wherever required.
1
3
In most scenarios a missing break indicates an error. In cases where this is valid it is better to pull out the logic to a function and invoke this function from the case blocks where required.
1
3
These are aspects not understood by many. It is better to catch these problems upfront.
1
3
Usage of this indicates improper exception handling. It is good to force developers to learn good exception handling practices by usage of this rule.
1
3
This situation should not exist. If it does it shows lack of knowledge on part of the developer.
1
3
This is again something not known to everybody and this rule needs to be in place for eliminating such problems.
1
3
This in most cases is a bug. It is good to have this rule turned on.
1
3
This is a good rule to follow to avoid NullPointerExceptions. Enforcing of this will make the developers follow this good practice.
4
3
This does not cause any problem and sometimes can add to the understanding of the program for a junior programmer. It is better to give this a very low priority.
1
3
Extremely important. Not many understand and the rule will go a long way towards eliminating lot of surprises.
4
3
This does not cause any problem. It is better to give this a very low priority.
4
3
This does not cause any problem. It is better to give this a very low priority.
2
3
It is good to define the constants in the right places. Defining an interface for only the constants is not a right approach and should be avoided.
1
3
This is not known to many and it is better to have this check in place to avoid surprises.
1
3
Extremely important as it leads to a good Exception handling framework.
2
3
The performance impact is not known to many. It is good to have this rule in place.
2
1
A good rule to have. Will not be a disaster if we do not give it a very high priority.
1
1
It is important not to have empty implementations of Abstract methods as the sub-classes may not override this method thinking there is an implementation in the super class.
1
3
This rule can prevent creeping of bugs. If the value of the variable needs to be set it should be done through a set method rather than through a generic method.
2
1
This helps preventing Null Pointer Exceptions and it makes sense in having this check given higher priority.
1
1
It does not make sense to have such a class and it is better that such classes are dropped or converted to interfaces if required.
1
1
It is better to use an if - else if statement instead of switch statements in these scenarios.
3
3
Makes no sense in having an empty finalizer. Becomes important in an application dealing with JNI. Otherwise this can be given a lower priority.
3
3
Makes no sense in having an empty finalizer. Becomes important in an application dealing with JNI. Otherwise this can be given a lower priority.
2
3
Not many understand the finalize function and this rule can prevent users from creating this method inadvertently.
1
3
If finalize is being used then it s important to invoke super.finailize() as one needs to ensure that all the native resources are properly cleaned up.
1
3
If finalize exists it is must to have this as protected so that it is not possible to invoke this from outside.
1
3
Even if finalize were visible one should not invoke this.
2
4
It is good to eliminate duplicate imports. This rule should be implemented.
2
4
Objects and packages from Java.lang should never be downloaed.
2
4
Unused imports should be eliminated. The IDEs have the feature to remove these at the stroke of a key. It is better to keep this rule and eliminate duplicates.
2
3
This is unecessary import and should be eliminated.
2
3
Too many static imports can cause confusion. Instead one should look at moving these static fields to this class if this is the class where it is being used the most.

3
This can be important for applications which serialize beans for a long term. As the versions change if this is not set properly this will lead to problems in the application.

3


3


3


3


3


3


3


3


3


3


3

1
3
This is important otherwise tracking the root cause can become difficult.
1
3
This is important otherwise one can end up with unforeseen messages in the logs.
1
2
We should not have more than one type of logger in a system.
1
2
It should be for efficiency as one will need to instantiate this only once per object.
1
2
System.out.println should be discouraged. One should instead use logging in a sensible fashion even for debugging.
1
3
One should not do printStackTrace. One should get it through a logger if required.
1
3
It is important to use List instead of Vectors because we will be decoupling the application from the underlying implementation of the Collection
1
3
It is important to use Map instead of HashTables or HashMaps because we will be decoupling the application from the underlying implementation of the Collection
1
3
It is better to use this as this is the new method of iterating through a list which cannot be accessed via an index.
1
2
Enum has become a keyword and hence it should not be used as a variable name.
1
2
Assert has become a keyword and hence it should not be used as a variable name.
1
2
Use the predefined Integers wherever possible to reduce creating of unnecessary objects.
1
2
This helps improving memory usage and so should be implemented.
1
2
This helps improving memory usage and so should be implemented.
1
2
This helps improving memory usage and so should be implemented.

3


3


3


3


3

1
3
Extremely small variable names should be discouraged. It will lead to unmaintainable code. The default value of less than 3 characters is fine.
1
3
Extremely long variable names can cuase problem. The default value of 17 may be small and we may look at increasing this to 30.
1
3
Very short method names will be a problem and need to considered for reporting.
1
1
These bare minimum rules should be followed by all developers with regards to naming convention.
1
1
These bare minimum rules should be followed by all developers with regards to naming convention.
1
1
These bare minimum rules should be followed by all developers with regards to naming convention.
1
3
These bare minimum rules should be followed by all developers with regards to naming convention. It helps if one is able to identify the Abstract classes by just looking at their name. It may make sense to add a rule to have interfaces prefixed with I so that to identify them just as easily.
2
3
Using the $ symbol in variables names is not a common practice and it is better to discourage the usage.
1
3
Only constructors should have same name as the class. The methods must have different names.
2
3
HashCode method should be implemented if and only if required. Existence of a misnamed Hashcode method can cause confusion.
2
3
It is better to have the constants fields properly name and distinguished from the attributes of the class.
2
2
If a equals method is written it should be written properly. An improper equals method will cause confusion.
1
3
Only constructors should have same name as the class. The fields/attributes must have different names.
1
3
Keep method and attribute names distinct. It can lead to confusion and errors if not followed properly.
1
3
Every class should be in a package.
1
3
The case of the package should be all small cases.
2
3
Normally m_ is used to indicate the attribute of a class. Usage of this convention for other types of variables can cause a problem. It is better to avoid this.
2
4
Instead of get method for returning Booleans one should consider using a isAttribute method.
2
3
This could be taken with slightly lesser priority. Not doing this will normally not cause a problem.
2
3
This could be taken with slightly lesser priority. Not doing this will normally not cause a problem.
3
3
This is a good practice, but many a times this is a requirement and has to be violated.
1
3
Improves performance and so this rule should be enabled and followed.
1
3
Give a performance gain and should be used.
1
3
Absolutely important to have this rule enabled. Consider using StringBuilder in cases where the code is going to be executed only inside a single thread.
1
3
Important for getting good performance.
1
3
Important for getting good performance.
2
3
Important from a good memory management perspective.
1
3
Important to follow this rule as string concatenation is bad in Java.

5

1
3
It covers too many of errors and one should try to handle specific exceptions instead of a generic exception.
1
3
One should throw specific exceptions rather than a generic one so that one is in control rather than at the mercy of the underlying code.
1
3
Definitely a must. Presence of this error indicates improper understanding or implementation of Exception Handling.
1
3
One should not catch NullPointerExceptions. If at all it should be handled only at the topmost level.
1
1
Throw specific Exceptions to the extent possible.
1
1
Null Pointer Exception should never be thrown. It should be left as an unhandled exception at all levels except at the entry points into the system.
3
3
No sense in doing it, but at least it is being rethrown instead of being suppressed which is even worse.
1
3
Improper Exception handling.
1
4
One should not throw an Exception in the finally block as one will not know if a real exception has happened elsewhere. It will become extremely difficult to trace the root cause.
3
3
No sense in doing it, but at least it is being rethrown instead of being suppressed which is even worse.
1
3
Use a constant. The compiler does try to address these issues, but it is better for us to address it rather than depending on the compiler to do it for us.
4
2
Harmless. Does not add to the problems
3
3
Better to avoid to bring some efficiency into the execution.
1
3
Will reduce the improvement brought about by the usage of StringBuffer or StringBuilder.
2
3
Use equalsIgnoreCase instead of converting the strings toUpper or toLower for comparison
1
3
Use the stringBuffer.length instead of StringBuffer().toString().length(). It will unnecessarily create a string before the length is determined.
1
3
Do this to get better performance.
1
3
Will reduce the improvement brought about by the usage of StringBuffer or StringBuilder.
1
3
Use this to get better performance
1
3
Use a utility function to improve performance
1
3
If possible specify an initial buffer length for the StringBuffer and if possible also a growth rate to get better performance.
1
3
Good practice.
1
4
This is extremely dangerous and is actually a bug.
1
3
Not using equals will lead to bugs.
1
3
This can lead to memory leaks and these can be difficult to spot. It is better to have this check in place to avoid this problem.
1
3
It is not good to expose the variable in a class to the external world. Copy and return the array to avoid surprises. One the array goes out of the class one cannot predict on how the values can change in the external world and all these changes will be reflected back in the array in the class and similarly any changes to the array in the class will be reflected in the outside world and one would not know how the rest of the world will react to those changes.
1
3
Do not have reference to external variables. Instead create a copy and keep it to avoid surprises. Like above it is again risky to just keep a reference to an array passed from the external world as it can change outside the class and one cannot predict how the system will react to these changes.
1
3
Need to have this to avoid tight coupling to specific classes
3
3

2
4
Avoid having unused imports.
1
3
Throw specific exceptions instead of generic exception
3
3
It is better to eliminate unused codes as it reduces the footprint of the objects when loaded in memory.
3
3
It is better to eliminate unused codes as it reduces the footprint of the objects when loaded in memory.
3
3
It is better to eliminate unused codes as it reduces the footprint of the objects when loaded in memory.
3
3
It is better to eliminate unused codes as it reduces the footprint of the objects when loaded in memory.
2
3
Avoid using class loader unless you are very sure of what you are doing.
3
4
It is good to follow a standard naming convention so that everybody can understand what is happening.
3
4
It is good to follow a standard naming convention so that everybody can understand what is happening.
3
4
It is good to follow a standard naming convention so that everybody can understand what is happening.
3
4
It is good to follow a standard naming convention so that everybody can understand what is happening.
3
4
It is good to follow a standard naming convention so that everybody can understand what is happening.
1
3
System.exit should not invoked within a J2EE application as this will cause the entire JVM to halt. This should be invoked only by the container.
1
3
Violation of this rule can lead to surprising results.
2
3
This is a good practice, but there can be situations where this needs to be violated. But one should be very careful if one violates this.

3


2


3


3


2


3


2


2


3


3


3


3


3


3

No comments: