What??? CheckStyle is to guarantee code quality, or not?
No, it can not, but can make huge damage in quality if used in a wrong way. I try to make this clear. The only purpose of Code quality could be to make the developers life easier. The compiler doesn’t care if you made comments for all your getter-setter. The consumer of the code quality is the developer only, nothing and nobody else.
Code quality = Readable code for developers
So if we make developers job harder just in the name of “Code Quality”, then the concept of “Quality” lose it’s real meaning, lose it’s goal, and the goal becomes achieving something that is probably measurable by management point of view, meanwhile does not reduce development and maintenance time since it’s misses the point. This seems to be a very common management mistake in current practice. Typical scenario that before there were no rules, then manager decides and introduce overruled styling in the name of “Code Quality”. From management point of view, this is simple logic:
- more rules = more quality !
- and rules are measurable!
- so we can measure code quality by automated tools (how wonderful!)
This can be a comfortable management point of view, but the truth is that
real code quality is not measurable by automated tools
Code quality is code readability, that is only measurable by the developers who working on it, by how hard they found to work with it. This article tries to make this clear and to show the bad side of overruling.
1) Auto-formatting by mandatory
Using mandatory auto-formatting policy at a company means that developers lose the control of formatting their own code. Why is this bad? Because auto-formatter is not smart enough to handle more sophisticated formatting cases. For example auto-formatter destroys fluent code. Which is more readable?
Fluent code before formatter:
Configuration config = ConfigurationBuilder
.begin()
.addRule()
.when(JAASRoles.required("admin", "privacy-monitor")
.and(Direction.isInbound())
.and(Path.matches("/admin/{tail}")))
.perform(Forward.to("/internal/resource.jsp"))
.where("tail").matches(".*");
return config;
Fluent code after formatter:
Configuration config = ConfigurationBuilder.begin()
.addRule().when(JAASRoles.required("admin", "privacy-monitor").and(Direction.isInbound()).and(Path.matches("/admin/{tail}")))
.perform(Forward.to("/internal/resource.jsp")).where("tail").matches(".*");
return config;
There are several other formatting cases also, when auto-formatter destroys readability, because auto-formatter is not smart enough to handle every readability cases so enforcing it cause decrease of readability.
2) Enforcing obsolete styling rules like:
- “this.” prefix for member variables,
- “Class.this” for inner classes,
- “final” qualifier for local vars, (inroduced effective final in java 8 to get rid of that)
- prohibit using “return” from inside block,
- alphabetic ordering of methods, variables
- and so on…
Which is more readable?
enforcing “this.”, and “InnerClass.this”
this.tableComponent = AutoStartWorkflowTableFactory.this.appCtx.getBean(PagingTableComponent.class);
this.tableComponent.initTableContent(AutoStartWorkflowTableFactory.this.factory, AutoStartWorkflowTableFactory.this.factory, 5,
this.createDefaultFilter());
Item selectedItem = AutoStartWorkflowTable.this.tableComponent.getTable().getItem(selectedValue);
workflowName = AutoStartWorkflowTable.this.getValue(selectedItem, AutoStartWorkflowTable.WW_NAME);
fromDate = AutoStartWorkflowTable.this.getValue(selectedItem, AutoStartWorkflowQueryFactory.WW_FROM_DATE);
without that:
tableComponent = appCtx.getBean(PagingTableComponent.class);
tableComponent.initTableContent(factory, factory, 5, createDefaultFilter());
Item selectedItem = tableComponent.getTable().getItem(selectedValue);
workflowName = getValue(selectedItem, AutoStartWorkflowQueryFactory.WW_NAME);
fromDate = getValue(selectedItem, AutoStartWorkflowQueryFactory.WW_FROM_DATE);
These rules are made for messy code, or old IDE-s but become obsolete because for example modern IDE-s using colors to qualify elements. For really bad code ruling like this can be step forward where 300 line methods exists, to figure out what’s happening, but if code converges to clean code, these rules are making more harm than effort by creating noise, so decreasing readability.
3) Enforce those comments everywhere
Ok, this cause results like this:
/** the property name for seoUrl. */
public static final String PROPERTY_SEO_URL = "seoUrl";
/** the property name for seoPath. */
public static final String PROPERTY_SEO_PATH = "seoPath";
/** the property name for itemAcl. */
public static final String PROPERTY_ITEM_ACL = "itemAcl";
/** The type name*/
public static final String TYPENAME = "seoHtml";
/** Host authentication state */
private AuthState hostAuthState = new AuthState();
/** Proxy authentication state */
private AuthState proxyAuthState = new AuthState();
/** @return the value of "Seo Url" property. */
public String getSeoUrl();
/** @return the value of "Seo Path" property.*/
public String getSeoPath();
/** @return the value of "Item Acl" property.*/
public String getItemAcl();
Are these comments really helpful?
/**
* Default constructor.
*
* @param pCommand the command
* @param pAcknowledgments the acknowledgments
* @param pSwitchingStatus the switching status
*/
public SwitchingCommandContext(Command pCommand, SwitchingStatus pSwitchingStatus, Acknowledgments pAcknowledgments) {
...
}
OK, we are now “well documented”, so our code has 100% quality rigth ?
Unfortunately not.
These enforced useless default comments
- would take 95% of typical codebase.
- makes harder to refactor code,
- opens door to mistake of not updating it,
- leads to misleading obsolete information.
These problems can be avoided by
- choosing the rigth names of our variables and methods,
- using comments rarely only if it has reasonable additional information
- cleaning code instead of commenting bad code.
NOTE: There are cases when comments are reasonable, for example the public API of a library.
An overruled code:
This example presents the typical code style policy mismanagement. Useless information is present, but code actually not well readable. Probably checkstyle percentage is perfect, but developers still could not maintain it easily.
Which is more understandable and maintainable?
public class DecoratorConverterHelper {
/**
* Decorate a repository item. //don't you say...
*
* @param <T> type //thank you!
* @param item repository item //thanks for this also
* @param clazz class //i would never figure this out, thanks!
* @return decorated repository item //thanks to make this clear!
*/
public static <T> T decorate(RepositoryItem item, Class<? extends T> clazz) {
T t = null; // <- ugly workaround for styling rule: "should not return from block"
if (null != item) {
CollectionConverter<RepositoryItem, T> converter = new CreateRepositoryFacadeByReflection<RepositoryItem, T>(clazz);
t = converter.applyConversion(item);
}
return t;
}
}
Without useless comments, and rules:
public class DecoratorConverterHelper {
public static <T> T decorate(RepositoryItem item, Class<? extends T> clazz) {
return item == null ? null :
new CreateRepositoryFacadeByReflection<RepositoryItem, T>(clazz).applyConversion(item);
}
}
Conclusion:
These kind of rules can cause small quality gain in case of very bad codebases, and negligent developers, but applying on a good code base and enthusiastic developers ends up in a decrease of the code quality and increased frustration for the professionals who feels paralyzed by the non reasonable rules. The tool is not the devil, but the negligent usage of the tool. If using such tool like CheckStlye, a very careful selection of rules should be done involving the developers, since the goal of the “code quality” is the developers themselves at the end. Developers are professionals, not children, so consult with them, educate them, trust them instead of enforcing them, and don’t dream of auto-measuring real quality by checking style.
