1
\$\begingroup\$

I am developing a small compiler like tool. In the process, I wrote a Class<?> wrapper, JavaClass:

public class JavaClass {

    ...

    public List<GenericEntry> getAllGenericEntries() {
        return GenericEntriesFinder.getAllGenericEntries(this);
    }
}

I've at first written the bellow shown code inside JavaClass, but I soon found it too big to mantain in JavaClass, that is already getting fat. I basically will only want to ever use getAllGenericEntries(), so all the other methods are basically implementation details.

public class GenericEntriesFinder {
    public static List<GenericEntry> getAllGenericEntries(JavaClass javaClass) {
        List<GenericEntry> result = new ArrayList<GenericEntry>();
        result.addAll(getClassGenericEntries(javaClass));
        result.addAll(getInterfaceGenericEntries(javaClass));
        return result;
    }

    private static List<GenericEntry> getClassGenericEntries(JavaClass javaClass) {
        IJavaType genericSuperClass = javaClass.getGenericSuperclass();
        JavaClass superClass = javaClass.getSuperClass();

        return getGenericEntries(javaClass, Arrays.asList(genericSuperClass), Arrays.asList(superClass));
    }

    private static List<GenericEntry> getInterfaceGenericEntries(JavaClass javaClass) {
        List<IJavaType> genericInterfaces = javaClass.getGenericInterfaces();
        List<JavaClass> nonGenericInterfaces = javaClass.getInterfaces();

        return getGenericEntries(javaClass, genericInterfaces, nonGenericInterfaces);
    }

    private static List<GenericEntry> getGenericEntries(JavaClass javaClass, List<IJavaType> generic, List<JavaClass> nonGeneric) {
        List<GenericEntry> result = new ArrayList<GenericEntry>();

        for (int i = 0; i < generic.size(); ++i) {
            IJavaType javaType = generic.get(i);
            JavaClass className = nonGeneric.get(i);

            if (javaType.isParameterizedType()) {
                List<JavaTypeVariable> typeVariables = className.getTypeParameters();

                for (int y = 0; y < typeVariables.size(); ++y) {
                    ParameterizedType parameterizedType = javaType.getAsParameterizedType();
                    IJavaType parameter = JavaFactory.createJavaType(parameterizedType.getActualTypeArguments()[y]);

                    result.add(new GenericEntry(typeVariables.get(y), parameter));
                }
            }
        }

        return result;
    }
}

My question is in how to organize this. I see several alternatives:

  1. I could put everything in JavaClass (doesn't seem that great of an idea).
  2. I could put getAllGenericEntries() in JavaClass, that delegates all the work to GenericEntriesFinder.getAllGenericEntries(). This seems to be nice as the client of JavaClass can use the method as long as he has a JavaClass in his hand.
  3. I could put all this logic in a separate class(GenericEntriesFinder, for instance). If I ever wanted to use this logic, I'd use GenericEntriesFinder and pass to getAllGenericEntries() a JavaClass instance. The client has the burden to know there actually exists an GenericEntriesFinder class, which is not that big of an issue, as I'm the only programmer, but still..

If I am not mistaken these are the 3 main possibilities. For possibilities 2 and 3 there is still the question of whether to consider GenericEntriesFinder's methods as static or instance methods. I know in a pure OO scenario one should look and strive for using instance methods, but as I've come to find, when writing a compiler everything tends to turn into a heap of functions(that is, as a sequence of transformations of data), and it's hard a lot of the time to try to model this in a OO way (data plus operations on data).

Plus, on 2., would I decide on using instance methods on GenericEntriesFinder, should I pass its instance in JavaClass's constructor? I don't particularly like this option, as if I start adding more and more methods to JavaClass, I will eventually have to pass it several classes like GenericEntriesFinder as constructor arguments. Passing classes by constructor argument may seem like a unit-testing plus but it actually will make gain nothing, as I'll be unit-testing GenericEntriesFinder.

A big point in here is that this logic is going to be unit-tested.

\$\endgroup\$
1
\$\begingroup\$

The problem with splitting this class out is that it's far too interested in the details of JavaClass. It spends half of its time pulling data out of JavaClass. It spends the other half of its time pulling data of out ParameterizedType. It doesn't really have any interesting data or behavior of its own. In it's heart, its still part of JavaClass. So it's not a great split.

But there are things we can do about that:

        IJavaType javaType = generic.get(i);
        JavaClass className = nonGeneric.get(i);

IJavaType presumably wraps java.lang.reflect.Type. In the case that its a generic type, its the subinterface ParameterizedType. ParameterizedType has a method getRawType() which will return the actual class being parameterized. In this case, that's whatever className refers to. Point being, javaType knows what class it is, so there's no need for the nonGeneric list. I think they code will be significantly streamlined as a result.

public static List<GenericEntry> getAllGenericEntries(JavaClass javaClass) {
    List<GenericEntry> result = new ArrayList<GenericEntry>();
    result.addAll(getClassGenericEntries(javaClass));
    result.addAll(getInterfaceGenericEntries(javaClass));
    return result;
}

private static List<GenericEntry> getClassGenericEntries(JavaClass javaClass) {
    IJavaType genericSuperClass = javaClass.getGenericSuperclass();
    JavaClass superClass = javaClass.getSuperClass();

    return getGenericEntries(javaClass, Arrays.asList(genericSuperClass), Arrays.asList(superClass));
}

private static List<GenericEntry> getInterfaceGenericEntries(JavaClass javaClass) {
    List<IJavaType> genericInterfaces = javaClass.getGenericInterfaces();
    List<JavaClass> nonGenericInterfaces = javaClass.getInterfaces();

    return getGenericEntries(javaClass, genericInterfaces, nonGenericInterfaces);
}

About half of this class is concerned with making sure that we handle both interfaces and superclasses. Instead, how about a function like

List<IJavaType> getBases()
{
    List<IJavaType> bases = new ArrayList<IJavaType>();
    bases.add( getGenericSuperclass() );
    bases.addAll( getGenericInterfaces() );
    return bases;
}

I'm gonna guess you may find other uses for this, and so it probably makes sense to go inside your JavaClass.

        if (javaType.isParameterizedType()) {
            List<JavaTypeVariable> typeVariables = className.getTypeParameters();

            for (int y = 0; y < typeVariables.size(); ++y) {
                ParameterizedType parameterizedType = javaType.getAsParameterizedType();
                IJavaType parameter = JavaFactory.createJavaType(parameterizedType.getActualTypeArguments()[y]);

                result.add(new GenericEntry(typeVariables.get(y), parameter));
            }
        }

This piece of code is far too interested in the internals of the parameterized type. That's a big hint that it should move into a function inside your wrapper of ParameterizedType.

If you follow my advice, I think you'll have a function something like

List<GenericEntry> getBaseGenericEntries()
{
    List<GenericEntry> entries = new ArrayList<GenericEntry>();
    for(IJavaType type: getBases() )
    {
         entries.addAll( type.getGenericEntries() );
    }
    return entries;
}

Of course, this no longer makes sense to put into its own class. It'll do just fine as a member of JavaClass.

I've at first written the bellow shown code inside JavaClass, but I soon found it too big to mantain in JavaClass, that is already getting fat.

One of the trickier parts of OOP is deciding how to break up fat classes. Trying to extract getAllGenericEntries wasn't really a good split. It really fit as part of JavaClass so trying to pull it out doesn't really work that well. So if you want to trim the fat, you need to look elsewhere.

I'd look for implementation details. This is stuff which the user of the class isn't aware of but is somehow supporting the interface. Often that can be extracted into some sort of supporting class. Another place to look is whether there are different subsets of functionality used by different pieces of code. If half of JavaClass is used by ModuleA and the other half is used by ModuleB with little to no overlap, then splitting the class can work really well.

\$\endgroup\$
  • \$\begingroup\$ It really fit as part of JavaClass so trying to pull it out doesn't really work that well. So if you want to trim the fat, you need to look elsewhere. But what if there was nowhere else to trim? What do you do if the implementation really is too large, and adds too much noise to your class definition? \$\endgroup\$ – seand Feb 4 '12 at 5:42
  • \$\begingroup\$ @seand, you'll see that I did in fact suggest pulling specific pieces of the implementation out into the other classes. I'm all for pulling large implementations out of the class. But you've gotta be careful about how you split them. \$\endgroup\$ – Winston Ewert Feb 4 '12 at 5:48
  • \$\begingroup\$ I was referring to the case where you can't pull pieces of the implementation out. For instance, when you aren't manipulating your own classes, but instead some third party library. \$\endgroup\$ – seand Feb 4 '12 at 5:53
  • \$\begingroup\$ @seand, I can always pull pieces of the implementation out. I can always define more classes and put code into them. So I really have no idea what you are getting at. \$\endgroup\$ – Winston Ewert Feb 4 '12 at 6:09
  • \$\begingroup\$ I was trying to make my answer sound less dumb. \$\endgroup\$ – seand Feb 4 '12 at 6:16
0
\$\begingroup\$

I know in a pure OO scenario one should look and strive for using instance methods, but as I've come to find, when writing a compiler everything tends to turn into a heap of functions [...]

Object orientation is appropriate for a large set of projects, but as you've discovered, it's not ideal for everything. You shouldn't try to force things into an OO paradigm if it's going to make things overly difficult or introduce needless complexity.

I think the code you posted is fine. All you're really trying to do is have a getAllGenericEntries() method within JavaClass without having its implementation create extra noise in the class definition, and I think option #2 (with static methods) is the best way to do that.

Really, I think this is something that IDEs should handle. The pattern you used, where a method implementation is split into several methods (which are only used by that one parent method), should be taken into account when displaying a class definition. Any private methods that are only used by one method could probably be hidden from the class definition entirely, and only accessed via that one method.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.