Reconsider the user of Factories for all types

[jamesread]

@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.

[utherp0]

I don't agree. It provides a fixed base interface/class model that is easily extensible without radical interface changes.

Ian 'Uther' Lawson Senior Solution Architect Open Source/Agile Methods Evangelist

Sent from my Android phone using TouchDown (www.nitrodesk.com)

-----Original Message----- From: James Read [notifications@github.com] Received: Sunday, 03 Mar 2013, 3:38 To: RedHatUKI/massiveData [massiveData@noreply.github.com] CC: utherp0 [ian.lawson@redhat.com] Subject: [massiveData] Reconsider the user of Factories for all types (#1)

@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.

---

Reply to this email directly or view it on GitHub: https://github.com/RedHatUKI/massiveData/issues/1

[utherp0]

Need to check your 'bullshit detector'.....

Say you have 100 generator classes. Which is better, a hundred if-then clauses using separate named instances or a set of Strings/Strings and a single instance creator?

Please keep questions and suggestions constructive, it's hard enough to keep enthusiasm when knackered, terminology like 'bullshit detector' and other 'humourous' statements will get you ignored very quickly.

Ian 'Uther' Lawson Senior Solution Architect Open Source/Agile Methods Evangelist

Sent from my Android phone using TouchDown (www.nitrodesk.com)

-----Original Message----- From: James Read [notifications@github.com] Received: Sunday, 03 Mar 2013, 3:38 To: RedHatUKI/massiveData [massiveData@noreply.github.com] CC: utherp0 [ian.lawson@redhat.com] Subject: [massiveData] Reconsider the user of Factories for all types (#1)

@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.

---

Reply to this email directly or view it on GitHub: https://github.com/RedHatUKI/massiveData/issues/1

[jamesread]

@utherp0 I anticipated that you would be annoyed, so I stopped after raising 4 issues and didn't respond to your comments immediately - wanted to give time for the air to clear and keep this constructive, rather than a rant ;)

So, allow me to clarify - factory classes are not always bad, but the use of factories when we have no implementations to construct is questionable, and passing explicit class names into a factory constructor does not gain you anything.

Here is a code example that hopefully demonstates my point (using the IHash Factory example) with one of the existing method signatures in the codebase today;

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

If you were using this factory, you would have calling code that looks like this;

public class MyTest {
    public void testGetHasher() {
        Sha1Hasher hasher = HashFactory.getHash(Sha1Hasher.class);
    }
}

Now, my concern is that you have a hard dependency to that Sha1Hasher class, which you have to link from your code and ultimatly, the getHash function is just doing a unsafe version of; return new Sha1Hasher(); (because it has no knowledge of construction specifics).

Therefore, factories normally use a technique like the following;

public class MyTest {
    public void testGetHasher() {
        IHash hasher = HashFactory.getHashForItemAspects();;
    }
}

Or, if you would prefer a more useful example for Generators;

public class MyTest {
    public void testGeneratorTextFiles() {
        IGenerator generator = GeneratorFactory.getGeneratorForFile(File f);
    }
}

This above example demonstrates more clearly, _your caller method does not need to know anything about the generators that are available in the system, it asks the factory to do the hard work_. The Generator factory here goes off through all registered generators and then returns the generator that is the most appropriate for the file.

So to summarize, I'm trying to make 2 points here;

1) Don't use lots of factories in the code, just when the new keyword would suffice. 2) When lots of implementations of an interface are available and logically choosing the most relevant is complex (ie, getting the right generator for a file), then use a factory, but don't depend on the explicit class name.

The use of a factory would be correct in the generators example, you'll find that I actually created the Generator Factory precisely for this purpose of scaling to a large number of Generators.

To end: I'm not trying to criticize, I'm trying to bring my experience as a developer to the codebase to improve on what we have. It is an absolutely horrible feeling when your code gets criticized - I know exactly what it feels like. It's not personal, it's asking for thought ;)