3

I have an interface which looks like the following

interface Evaluator {
    boolean requiresP2();
    EvalResult evaluate(Param1 p1, Param2 p2, Param3 p3);
    // some more methods
}

This interface is implemented by several classes. The parameter p2 of the evaluate method is used by some and not used by others. The method requiresP2 basically returns a boolean telling whether the evaluate method uses p2 or not.

Now, this questions may appear a little weird out of context but believe me, it makes sense in our use case. Plus, it would require a lot of time to refactor all the code to eliminate the need for the requiresP2 method so I would appreciate if we discuss solutions other than a top-to-bottom refactoring of the codebase.

The problem is that the return value of method requiresP2 is based on how the evaluate method is implemented. Therefore everyone must ensure that they update the requiresP2 method when they change the evaluate method.

I am looking for ways so that this can be enforced by the compiler/unit-tests/linters rather than leaving it to the developer's memory.

EDIT: I am still exploring the applicability of mocking frameworks to this problem.

I thought that I could reflection in unit tests to inspect evaluate's body in the unit test to check if it refers to p2 or not and then making sure it matches with the value returned by requiresP2 method but it seems that it is not possible to inspect method body using reflection.

I am looking for suggestions on how to do this. Any input is appreciated.

lakshayg
  • 2,053
  • 2
  • 20
  • 34

2 Answers2

5

There is another option you did not mention: a Static Code Analysis tool.

You can use the SonarQube + SonarLint combination in order to get your desired enforcement:

Use the SonarQube server in order to create a new static code analysis rule, which will be based on the interface you are using and your unique use case.

Then install SonarLint on your IDE/IDEs (Eclipse and IntelliJ are both supported), and connect it to the SonarQube server.

This way the static code analysis scan will detect improper usage of your interface and indicate this with a visual marking in the IDE, on the relevant code lines (which is actually linting your code).

Rann Lifshitz
  • 4,040
  • 4
  • 22
  • 42
  • This looks like a good option. I will explore the possibility of using this in our code. I will leave the question open for a while to see what other options are available. – lakshayg May 17 '18 at 23:49
4

You can use ASM to check whether the parameter is used.

To add it to your project using e.g. Apache Ivy, you would add this to ivy.xml:

<dependency org="org.ow2.asm" name="asm" rev="6.1.1" />

Or do the equivalent for Maven, Gradle, etc. Then you can check on the parameter by:

import java.io.IOException;
import java.io.InputStream;
import java.io.UncheckedIOException;
import java.util.concurrent.atomic.AtomicBoolean;

import org.objectweb.asm.ClassReader;
import org.objectweb.asm.ClassVisitor;
import org.objectweb.asm.MethodVisitor;
import org.objectweb.asm.Opcodes;

// . . .

public static boolean usesP2(Evaluator evaluator) {
    AtomicBoolean usesP2 = new AtomicBoolean(false);
    String internalName = evaluator.getClass().getName().replace('.', '/');
    String classFileResource = "/" + internalName + ".class";

    ClassVisitor visitor = new ClassVisitor(Opcodes.ASM6) {
        @Override
        public MethodVisitor visitMethod(int access, String name,
                String desc, String signature, String[] exceptions) {
            if ("evaluate".equals(name)) {
                return new MethodVisitor(Opcodes.ASM6) {
                    @Override
                    public void visitVarInsn(final int insn, final int slot) {
                        if (slot == 2) usesP2.set(true);
                    }
                };
            }
            return super.visitMethod(access, name, desc, signature, exceptions);
        }
    };
    try (InputStream is = Evaluator.class.getResourceAsStream(classFileResource)) {
        ClassReader reader = new ClassReader(is);
        reader.accept(visitor, 0);
    } catch (IOException e) {
        throw new UncheckedIOException(e);
    }
    return usesP2.get();
}

public static void assertCorrectlyDocumentsP2(Evaluator evaluator) {
    boolean usesP2 = usesP2(evaluator);
    if (usesP2 && !evaluator.requiresP2()) {
        throw new AssertionError(evaluator.getClass().getName() +
                " uses P2 without documenting it");
    }
    if (!usesP2 && evaluator.requiresP2()) {
        throw new AssertionError(evaluator.getClass().getName() +
                " says it uses P2 but does not");
    }
}

Unit tests:

@Test
public void testFalsePositive() {
    assertCorrectlyDocumentsP2(new FalsePositive());
}

@Test
public static void testFalseNegative() {
    assertCorrectlyDocumentsP2(new FalseNegative());
}

(This supposes there are two bad Evaluators, FalsePositive and FalseNegative, one of which documents that it uses P2 but doesn't, and the other which doesn't document that it uses P2 even though it does, respectively.)

Note: In usesP2 we check for a variable instruction (an instruction which accesses a local variable) in slot 2 of the stack frame. The slots are numbered from 0, and the first one is this. P2 is in slots 2 only because Evaluator::evaluate is an instance method. If it were a static method, we would have to check if slot 1 were used in order to detect if parameter P2 were used. Caveat lector.

David Conrad
  • 15,432
  • 2
  • 42
  • 54