|
Re: The Code C.R.A.P. Metric Hits the Fan - Introducing the crap4j Plug-i
|
Posted: Oct 5, 2007 7:34 AM
|
|
> > I have this method which has a CRAP score of 30 and a > CRAP > > load of 5: > > > > public boolean matches(Method method) { > > return > > method.getName().equals(methodName) && > > Arrays.equals(method.getParameterTypes(), params) && > > ( > > !declaringClass || > > method.getDeclaringClass().equals(clazz) > > ); > > } > > > > Could crap4j give me any clues on what kind of tests I > > should add? > > Hi Steven > > First of all, thank you for trying crap4j and reporting > back. > > > Could crap4j give me any clues on what kind of tests I > > should add? > > The method you mention has the form: > > return A && B && (C || D) > > Where: > A = method.getName().equals(methodName) > B = Arrays.equals(method.getParameterTypes(), params) > C = !declaringClass > D = method.getDeclaringClass().equals(clazz) > > In theory, to make sure you have covered all possible > combinations of true/false for the 4 components of your > boolean-valued expression (i.e. A, B, C, and D). That > would mean writing 16 (i.e. 2^4) test cases. In practice, > however, you can probably get away with fewer combinations > and still keep crap4j happy and protect yourself quite > well. > > You have probably noticed that crap4j shows code coverage > information (green/red bars on the side of the source code > view). You can use those to guide you. In the case of > conditional expressions like the one you have, if you > hover the mouse pointer over the coverage bars you should > see detailed information on which of the conditional > clauses it has not covered. > > You should definitely have test cases like the following: > > A,B,C,D: true, false, true, false > > (i.e. same method name but different parameter types and > the declaring class is not equal to clazz but > !declaringClass is true). This one should return false. > > A,B,C,D: true, true, true, false > > (i.e. same method name and same parameter types. The > declaring class is not equal to clazz but !declaringClass > is true). This one should return true. > > And so on. > > You might also consider doing some refactoring to simplify > the code and the testing (and lower the CRAP score). For > example: > > method.getName().equals(methodName) && > Arrays.equals(method.getParameterTypes() > > Could be extracted as a method called > "sameMethodSignature(..., ...)". > > I hope this helps. Thanks again for experimenting with > crap4j.
Hey Alberto,
Thanks for your reply!
I've been thinking about the metier of unit testing for years now and I tend to oscillate between "everything has to be tested" and "most things have to be tested". It depends on my mood and I guess on the type of code and projects I'm working on.
In this case the match() method is part of a utility class that is called regularly, according to crap4j this method is currently being called 54 times when I run all my tests.
I actually had to add the "declaring class" part to the boolean expression because in the case of some static methods I didn't get the overwritten methods I expected but the ones declared in parent classes.
So my (rethoric) question is: do I actually explicitly have to test this method and the utility method that calls it? It's obviously been tested by my other tests and I would quickly find bugs and already have in the past.
Thanks for any thoughts on this. I like crap4j.
Steven
|
|