Skip to content
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.

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

Replaced explicit null-checks by a declarative approach #951

Closed
wants to merge 1 commit into from
Closed

Replaced explicit null-checks by a declarative approach #951

wants to merge 1 commit into from

Conversation

denis-zhdanov
Copy link

The idea is to do the following:

  1. Remove explicit null-checks
  2. Mark corresponding method parameters by Nonnull annotation
  3. Configure the project to use Traute javac plugin to enhance resulting bytecode by null-checks for the target method parameters

Example: consider ClassPathResource:

Before

public ClassPathResource(String path, ClassLoader classLoader) {
    Assert.notNull(path, "Path must not be null");
    String pathToUse = StringUtils.cleanPath(path);
    ....

After

public ClassPathResource(@Nonnull String path, ClassLoader classLoader) {
    String pathToUse = StringUtils.cleanPath(path);
    ....

Resulting bytecode:

javap -c ./target/classes/spark/resource/ClassPathResource.class
...
  public spark.resource.ClassPathResource(java.lang.String, java.lang.ClassLoader);
    Code:
       0: aload_0
       1: invokespecial #2                  // Method spark/resource/AbstractFileResolvingResource."<init>":()V
       4: aload_1
       5: ifnonnull     18
       8: new           #3                  // class java/lang/IllegalArgumentException
      11: dup
      12: ldc           #4                  // String 'path' must not be null
      14: invokespecial #5                  // Method java/lang/IllegalArgumentException."<init>":(Ljava/lang/String;)V
      17: athrow

Benefits:

  • the code is cleaner because explicit null-checks are removed
  • the code is better documented because it clearly indicates which parameters can't be null
  • IDEs highlight possible NPE problems for method parameters marked by Nonnull

TESTED: mvn clean package & ensured
that resulting bytecode contains null-checks

a declarative approach

TESTED: mvn clean package & ensured
        that resulting bytecode contains
        null-checks
@tipsy
Copy link
Contributor

tipsy commented Dec 13, 2017

We meet again @denis-zhdanov. I'm not sure we want to introduce this in 2.X. @perwendel can decide.

@denis-zhdanov
Copy link
Author

Hi again :)

Any particular concern about that?

@tipsy
Copy link
Contributor

tipsy commented Dec 13, 2017

Not really any concerns, but the current version is supposed to be the last 2.X, with the exception of bugfixes.

@denis-zhdanov
Copy link
Author

Ah, fair enough.
I don't think it's necessary to make a dedicated release for that, i.e. it's enough to apply the feature and then release when there is a significant feature/bugfix to deliver to end-users.

@denis-zhdanov
Copy link
Author

Hey @perwendel

Could you please share your thoughts on the idea of adding Traute to your project?

Regards, Denis

@perwendel
Copy link
Owner

Thanks! Currently there are much higher prioritized bugs/features that needs attention. Adding another lib for null checks does not feel important, even though I like how it seems to work.

@perwendel perwendel closed this Mar 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants