This will be an exercise in refactoring, for better design and readibility, the Floats class that can be found in the Google Guava library.
I've chosen this class more or less arbitrarily from a widely used, open source, Java third party library called Guava. Guava is a library containing lots of different utility classes that complement well the JDK's libraries.
I find it very useful and with a well designed interface. I also use it in virtually every project, the main reason being that it renders common expressions and snippets of code that I otherwise find myself writing all the time, more meaningful and concise.
This class is a container for static methods that operate with floating point numbers. Therefore the one thing we cannot do is to assess its design as an object-oriented class.
Class purpose
Right off the bat, the class' javadoc description seems sketchy:
Static utility methods pertaining to {@code float} primitives, that are not already found in either {@link Float} or {@link Arrays}.
As much as this is a container for utility methods and not the abstraction that a class is supposed to be, nothing prevents us from critiquing its public interface. It seems that putting a method in the class because it doesn't fit so well in a couple of other classes (and viceversa) is a pretty weak design.
Javadoc formatting
In the same javadoc there's another questionable choice: the use of html tags in javadoc comments. It's supposed to be a comment, not an html page (even if it will end up as html, too).
Arrays
Throughout the class arrays are being used. There's no reason to use arrays in Java except for a few exceptional cases. Use the List interface instead.
Clutter javadoc
The javadoc for the function "int hashCode(float value)" is not really necessary. If it doesn't add anything at all it's better not to include it, for the sole reason that code is a live thing and comments have to be maintained, creating an unnecessary burden. Also it decreases readibility.
The same can be said of the javadoc of other methods' in this class.
Y.A.G.N.I.
// TODO(kevinb): consider making this public
private static int indexOf(float[] array, float target, int start, int end) {
...
It's more sensible to create a method when one makes up their mind that it needs to be part of the public interface. Same goes for other methods with the same or equivalent comments.
Null
Float tryParse(String string). This method can return "null", which is a bad idea and the developer thought it was better to fail gracefully for whimsical reasons. So I disagree.
In any case returning Float.NaN would be better than returning null.
Bugs and defects
return NEGATIVE_INFINITY < value & value < POSITIVE_INFINITY;
Someone didn't read the code (simply going through the code is a good method to detect defects).
It's a small defect, but it is obvious that the "&" should be "&&". If I'm wrong about this, I think it would be one of the rare occasions where an explanatory comment is in order.
It's a small defect, but it is obvious that the "&" should be "&&". If I'm wrong about this, I think it would be one of the rare occasions where an explanatory comment is in order.
Routines
There are several problems with the implementation of "int indexOf(float[] array, float[] target)". One is that it could be made more readable. Another is that the Guava library contains similar classes with identical methods for different primitive data types where the algorithm is repeated in each case. Finally Java's libraries already provide this kind of functionality, only not quite as concise as this method does.
The implementation can be done with a call to "java.util.Collections.indexOfSubList(List, List): int". By the way, with that in mind, isn't this ironic?
The implementation can be done with a call to "java.util.Collections.indexOfSubList(List, List): int". By the way, with that in mind, isn't this ironic?
Same can be said of the implementation of method "float min(float... array)" and some others.
Organization
There are classes amongst the methods. It would be better to put them together in one block or simply in their own files.
Code design
Class "FloatArrayAsList" can be initialized with an array and a "start" and "end" indeces. Instead of keeping the original array (for who knows what) and work with the relevant subarray, start and end indeces are used all over the implementation.
I'm not showing here the refactored class here since most changes would be pretty straightforward and uninteresting.
It's a little appalling that a widely popular library developed by Google programmers has substandard code and design.
Comments
Post a Comment