Skip to content

[Core] Optimize MethodScanner #1238

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 11, 2017
Merged

[Core] Optimize MethodScanner #1238

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 16 additions & 20 deletions java/src/main/java/cucumber/runtime/java/MethodScanner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,25 +2,22 @@

import cucumber.api.java.After;
import cucumber.api.java.Before;
import cucumber.runtime.ClassFinder;
import cucumber.runtime.CucumberException;
import cucumber.runtime.Utils;
import cucumber.runtime.ClassFinder;

import java.lang.annotation.Annotation;
import java.lang.reflect.Method;
import java.util.Collection;
import java.util.List;

import static cucumber.runtime.io.MultiLoader.packageName;

class MethodScanner {
private final Collection<Class<? extends Annotation>> cucumberAnnotationClasses;

private final ClassFinder classFinder;

public MethodScanner(ClassFinder classFinder) {
this.classFinder = classFinder;
cucumberAnnotationClasses = findCucumberAnnotationClasses();
}

/**
Expand Down Expand Up @@ -56,26 +53,25 @@ public void scan(JavaBackend javaBackend, List<String> gluePaths) {
* @param glueCodeClass the class where the method is declared.
*/
public void scan(JavaBackend javaBackend, Method method, Class<?> glueCodeClass) {
for (Class<? extends Annotation> cucumberAnnotationClass : cucumberAnnotationClasses) {
Annotation annotation = method.getAnnotation(cucumberAnnotationClass);
if (annotation != null) {
if (!method.getDeclaringClass().isAssignableFrom(glueCodeClass)) {
throw new CucumberException(String.format("%s isn't assignable from %s", method.getDeclaringClass(), glueCodeClass));
}
if (!glueCodeClass.equals(method.getDeclaringClass())) {
throw new CucumberException(String.format("You're not allowed to extend classes that define Step Definitions or hooks. %s extends %s", glueCodeClass, method.getDeclaringClass()));
}
if (isHookAnnotation(annotation)) {
javaBackend.addHook(annotation, method);
} else if (isStepdefAnnotation(annotation)) {
javaBackend.addStepDefinition(annotation, method);
}
Annotation[] methodAnnotations = method.getAnnotations();
for (Annotation annotation : methodAnnotations) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be even more efficient to use Method.getAnnotation(Class annotationClass) rather then checking all annotations. Then we can replace this for loop with two null checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about it. Could you clarify? Based on previous logic one method can be annotated with Before/After and any number of step annotations - we can't achieve it without loop. Second thing is that we are not able to get step annotation e.g. @Given using Method.getAnnotation(Class annotationClass) because we don't know what annotation is used (actually cucumber does not care if it is @Given or @Then it just should be annotation that itself is annotated with @StepDefAnnotation ), eventually this code won't work: method.getAnnotation(StepDefAnnotation.class)

Copy link
Contributor

@mpkorstanje mpkorstanje Oct 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Derp. You are correct. I didn't have the hierarchy of annotations properly in my mind.

if (isHookAnnotation(annotation)) {
validateMethod(method, glueCodeClass);
javaBackend.addHook(annotation, method);
} else if (isStepdefAnnotation(annotation)) {
validateMethod(method, glueCodeClass);
javaBackend.addStepDefinition(annotation, method);
}
}
}

private Collection<Class<? extends Annotation>> findCucumberAnnotationClasses() {
return classFinder.getDescendants(Annotation.class, "cucumber.api");
private void validateMethod(Method method, Class<?> glueCodeClass) {
if (!method.getDeclaringClass().isAssignableFrom(glueCodeClass)) {
throw new CucumberException(String.format("%s isn't assignable from %s", method.getDeclaringClass(), glueCodeClass));
}
if (!glueCodeClass.equals(method.getDeclaringClass())) {
throw new CucumberException(String.format("You're not allowed to extend classes that define Step Definitions or hooks. %s extends %s", glueCodeClass, method.getDeclaringClass()));
}
}

private boolean isHookAnnotation(Annotation annotation) {
Expand Down