redhatemea / massivedata Goto Github PK

@utherp0 There are factories all over the place with signatures & bodies like this;

public static <T extends IHash> T getHash(String className) throws ClassCastException, ClassNotFoundException, InstantiationException, IllegalAccessException {
    return (T) Class.forName(className).newInstance();
}

The thing is, this class is really rather pointless. It's a type unsafe way of creating a new class!

While it can be a good thing to stub in code (create "empty classes" where new code will go later), there is lots of lots of code in the project that is dead - HashFactory for example has 0 usages at the moment.

My mantra is to write code as you need it, not as you predict you'll need it. For now, I recommend simply using the "new" keyword! Here is a quote on a website against this directly as a (bogus) "design pattern":

Passing Class Names
Sometimes you see APIs like public SomeInterface SomeFactory.createInstance(String className) in itself this API is fine. You are loading and instanciating classes by name. Its absolutely sensible to hide that in a neat little method, maybe you are doing some memoization inside. Nothing wrong with that.

But then a developer comes around and tells you this exists in order to decouple the client using that API from the implementing classes. This is when your bull shit detectors should go of. The client has to put the class name in a String. And a string with a class name is basically the same kind of dependency as a direct reference to that class. Actually it is worse, since no static code analysis will find it. So this is hiding a dependency not removing a dependency. Don’t ever mix that up.

This method really scares me, UUID's are a very well defined format, we should not be aiming to "improve" upon them or anything.

    public static String generateUUID(boolean addUTC) {
        UUID uniqueID = UUID.randomUUID();

        return (addUTC ? uniqueID.toString() + "_" + System.currentTimeMillis() : uniqueID.toString());
    }

Could you explain the problem you are trying to solve, @utherp0 ? I think it should be solved in a more standard way.

@utherp0 So, allow me to explain my thoughts on thrown exceptions, and when it is good to wrap exceptions. Just to be clear, I'll cover the terminology;

_Thrown Exception_

public void testGeneration() throws IOException {
    throw new IOException("This is an exception");
}

_Wrapped Exception_

public void testGeneration() throws GenerationException {
    throw new GenerationException(new IOException("I am the root cause of a generation exception!");
}

Both examples should be used throughout the codebase. In my code, I follow these very simple rule;

_Rule: When to use explicitly thrown exceptions_

Declare thrown exceptions when it is undesirable to catch an exception. Prefer specificity over generalization (ie, if a method throws FileNotFoundException AND SocketException, then prefer to throw both, instead of the higher level IOException (which these both extend from).

By throwing both exceptions here, you can act in two separate ways. eg: log message for FileNotFoundException and system.exit for a SocketException. The calling method cannot handle both cases if you only throw IOException.

_Rule: When to use wrapped exceptions_

Finally, wrapped exceptions should be thrown on interface signatures.

The reason is, the caller of interface methods should have to know nothing about the implementation details (and the exceptions that those implementations might throw).

Also, you cannot prodict all exceptions that an implementation might throw. Lets say you have a PdfGenerator and you want to throw a PdfMalformedException. Because you don't declare this on the interface signature, you'll never be able to catch it. Therefore, you wrap this exception using the constructor GenerationException(Exception cause).

Hope this all makes sense. Let me know what you think.

JUnit for the win. 👍 Custom test structure for the loose.