3
\$\begingroup\$

This is for an instrumentation tool using the ASM tree api. This method assigns a handler to a field, which is used in instrumented methods. As a result of my instrumentation, the instrumented class implements an interface and is a handler itself, so a value of this behaves like a NullObject concerning handled methods - it doesn't do anything different but never throws an Exception (when the handled method in the instrumented class is invoked, it will cause a NullPointerException if the handler is null because methods are called on it).

Now, there's 3 different ways to avoid NullPointerExceptions (my enum):

  • checkBeforeCall: on every call of the handled method, this is used as the handler if it is not set to anything else but null. That's a "slow" approach if the handled method is called very often, as an if is required per call. Pretty please avoid telling me that that's ok and I shouldn't use the other two enum values - that's beside the point :-)
  • assignAfterSuper: after the super constructor is called, the handler field is initialized. This causes little to no overhead at runtime, but it causes problems if the handled method was overridden and is called in the super constructor, because it will throw the NPE.
  • assignBeforeSuper: The handler is set to this before the super constructor is called. No Java compiler will ever produce this bytecode, but it's valid and the scala guys do it as well. The handler field is then fully initialized after the super constructor was called.

I provide all three methods as the first one is fail-safe, the second is valid and you should be ok in most scenarios and the third is best but could potentially cause trouble in tools.

Now, for each of these initializations, there are two ways to set the handler. One - if you have the instrumented instance - is to call a "setHandler" method. That's ok if you have full access to the object, but you're helpless if you don't. The other is to provide a static method, which is called to create a handler. The instrumented instance is passed (so it can be returned itself if no handler is needed) and the result is then used to initialize the field.

Passing this to a static method before the super constructor was called is invalid bytecode. So I use this "spawner" method (if it was specified) after super(...) was called.

So the gory situation: I have an instruction list which I modify. In the relevant two enum-cases I need to consider for constructor instrumentation, I also execute the code for assignAfterSuper on assignBeforeSuper, but only if a static initializer is used (otherwise, it's already set to this). I'm modifiying an instruction list and use a little more state information and factoring any methods out doesn't really make sense, as the parameter list would be longer than the method itself and readability would be diminished. Factoring it out into different classes would also be problematic, as it hurts readability and I have to switch open files when I want to understand the methods behavior or I have to duplicate all code further up or I have to factor it out but can't see what else happens to my InsnList (which is dangerous, because it could mess up the result. I'd like to keep the InsnList for one MethodNode inside of the method so I can see at first glance which changes occur).

private void assignHandler(MethodNode method,
                String handleeInternalName, String handlerField) {
    if (this.guard == NullPointerGuard.checkBeforeCall) {
        return;
    }
    InsnList instructions = method.instructions;
    final boolean usesLabels = instructions.getFirst() instanceof LabelNode;
    // search for call of constructor super(...) or this(...)
    AbstractInsnNode constructorCall = instructions.getFirst();
    while (!(constructorCall.getOpcode() == INVOKESPECIAL
            && constructorCall instanceof MethodInsnNode
            && "<init>".equals(((MethodInsnNode) constructorCall).name))) {
        constructorCall = constructorCall.getNext();
    }
    // skip modification on delegation to this(...)
    if (handleeInternalName.equals(((MethodInsnNode) constructorCall).owner)) {
        return;
    }
    // not delegating to this(...), inject initialization of Handler field
    AbstractInsnNode node = instructions.getFirst();
    InsnList storeHandler = null;
    switch (this.guard) {
    case assignBeforeSuper:
        storeHandler = storeHandlerInField(handleeInternalName, handlerField, false);
        if (usesLabels) {
            storeHandler.add(new LabelNode());
            instructions.insert(node, storeHandler);
        } else {
            instructions.insertBefore(node, storeHandler);
        }
        if (!usesSpawner()) {
            break;
        } // else fallthrough
    case assignAfterSuper:
        storeHandler = storeHandlerInField(handleeInternalName, handlerField, true);
        if (usesLabels) {
            storeHandler.insertBefore(storeHandler.getFirst(), new LabelNode());
        }
        instructions.insert(constructorCall, storeHandler);
        break;
    default:
        throw new InstrumentationException(this.guard + " is not implemented yet");
    } 
    method.maxStack += usesSpawner() ? 2 : 1;
}

I am concerned about the fall-through inside the switch, but before I restructured it this way, I had either duplicated code for the actions or duplicated checks for the conditions. Unfortunately, each case cannot easily be extracted into a method.

Now, the structure is transparent, can easily be read and understood (as long as one knows how switch works) and there is no more redundancy. Still, this feels weird. Though I'm really proud I have found this solution, I wonder if there's a better way.

\$\endgroup\$
  • 9
    \$\begingroup\$ Without really seeing more of the code a common suggestion is to use inheritance and polymorphism. A first step would be to more common code to code to methods then once that is done decide if a seperate class will encapsulate the functionality you are seeing and the variables used. \$\endgroup\$ – dreza Aug 24 '12 at 19:42
  • \$\begingroup\$ is it possible that only code for e2 should be executed? \$\endgroup\$ – Denis Tulskiy Aug 25 '12 at 13:21
  • 2
    \$\begingroup\$ Should be a good starting point if you decide to go the route dreza stated (which I believe is the correct route): youtube.com/watch?v=4F72VULWFvc \$\endgroup\$ – Corbin Aug 25 '12 at 17:57
  • 2
    \$\begingroup\$ « The // do stuff part uses too many variables to extract it into a method. » is the first code smell. Try to refactor it in several independant methods, or group the parameters into a dedicated class. After that you can have e1 and e2 each in its own method, and decide to call one, the other, or both. \$\endgroup\$ – Joan Charmant Aug 26 '12 at 19:43
  • 1
    \$\begingroup\$ @Joan Charmant I do agree with you, but there's some kinds of problems where it's not that easy. For me, there's a threshold for how long a class and a method may get before I am willing to split them. And there's another one for when I'm willing to split a method into sub methods: Each one has to be easily understood and has to do something a human deems sensible. So a matrix multiply is fine, but when will I need some of LU factorization stuff with the relevant state information in another context? And how should I name that? \$\endgroup\$ – Arne Aug 27 '12 at 6:35
1
\$\begingroup\$

You use of switch is unconventional and in consequence hard to follow. I'd avoid it for that reason. What I see is that that both case are very similar, and so I'd try to refactor them into a function.

Here's my approach:

private void assignHandler(MethodNode method,
                String handleeInternalName, String handlerField) {
    if (this.guard == NullPointerGuard.checkBeforeCall) {
        return;
    }
    MethodInsNode constructorCall = findConstructorCall(method);

    // skip modification on delegation to this(...)
    if (handleeInternalName.equals(constructorCall.owner)) {
        return;
    }
    switch (this.guard) {
    case assignBeforeSuper:
        insertHandler(method, handleeInternalName, handlerField, null);
        if( usesSpawner() )
        {
            insertHandler(method, handleeInternalName, handlerField, constructorCall);
        }
        break;
    case assignAfterSuper:
        insertHandler(method, handleeInternalName, handlerField, constructorCall);
         break;
    default:
        throw new InstrumentationException(this.guard + " is not implemented yet");
    } 
}

void insertHandler(MethodNode method, String handleeInternalName, String handlerField, AbstractInsnNode node) // node specified node
 // to insert before, or null for beginning of method
{
    InsnList instructions = method.instructions;
    final boolean usesLabels = instructions.getFirst() instanceof LabelNode;
    InsnList storeHandler = storeHandlerInField(handleeInternalName, handlerField, node != null);


    // the rest of this function probably belongs in a InstructionInserter class 
    if(useLabels)
    {
        if(node == null)
        {
            storeHandler.add( new LabelNode() );
            node = instructions.getFirst(); // insert after existing label node
        }
        else
        {
            storeHandler.insert( new LabelNode() );
        }
    }


    if(node == null)
    {
        instructions.insert(storeHandler);
    }
    else
    {
        instructions.insert(node, storeHandler);
    }

    method.maxStack++;
}
\$\endgroup\$
  • \$\begingroup\$ I think you could be right, but I'm not sure yet. And that's exactly why I posted this. If I use this style, I'd probably also change the switch to an if ... else if ... else .... But I have strong aversions to pass control of something changing the state of the MethodNode outside of the function, I think it may hurt maintainability because the control flow is easier and better understood if the corresponding code is in close proximity (and the method length doesn't exceed my tolerance threshold yet). And I still think the switch hurts less than having to skip through the file. \$\endgroup\$ – Arne Aug 27 '12 at 16:00
  • \$\begingroup\$ ... continued: What I love is findConstructorCall (but I think I'll pass InsnList instead of MethodNode, so it's a little more generic). I thought of it myself but discarded it, but then I reorganized my method to its current version and now it would probably fit. Thanks! \$\endgroup\$ – Arne Aug 27 '12 at 16:04
  • \$\begingroup\$ @Arne, I think the control flow is easier to follow if you break it up into functions. That way its easier to grasp the high level logic of function. I think its easier to see in my version the logic about where the extra instructions get inserted because its not tangled with the low level details regarding label nodes and such. \$\endgroup\$ – Winston Ewert Aug 27 '12 at 17:18
  • \$\begingroup\$ @Arne, as for findConstructorCall, does it actually make sense for lists of instructions that aren't methods? It seems to me that the method only makes sense in that context so MethodNode is the appropriate parameter. \$\endgroup\$ – Winston Ewert Aug 27 '12 at 17:19
  • \$\begingroup\$ it doesn't even really make sense for MethodNode - as it's not necessarily a constructor, could also be a regular method. Still, if I want to use it to construct something else, I can use fragments of InsnList to construct a new MethodNode. \$\endgroup\$ – Arne Aug 27 '12 at 17:43

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.