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

Create possibility to validate method or constructor parameters for Not-nullness easily. #549

Closed
lombokissues opened this Issue Jul 14, 2015 · 6 comments

Comments

Projects
None yet
2 participants
@lombokissues
Collaborator

lombokissues commented Jul 14, 2015

Migrated from Google Code (issue 514)

@lombokissues

This comment has been minimized.

Show comment
Hide comment
@lombokissues

lombokissues Jul 14, 2015

Collaborator

👤 vyacheslav.sahno   🕗 May 07, 2013 at 10:44 UTC

What steps will reproduce the problem? Code like that:
public EmailableException(Object source, String errorMessage) {
if (source == null) {
throw new IllegalStateException(" (source == null) ");
}
if (errorMessage == null) {
throw new IllegalStateException(" (errorMessage == null) ");
}
this.source = source;
this.errorMessage = errorMessage;
this.exception = null;
}
What is the expected output? What do you see instead? Code like that:
public EmailableException(@ NotNull Object source, @ NotNull(customErrorMessage=" (source == null) ") String errorMessage) {
this.source = source;
this.errorMessage = errorMessage;
this.exception = null;
}

What version of the product are you using? On what operating system?
0.11.8

Please provide any additional information below.

Collaborator

lombokissues commented Jul 14, 2015

👤 vyacheslav.sahno   🕗 May 07, 2013 at 10:44 UTC

What steps will reproduce the problem? Code like that:
public EmailableException(Object source, String errorMessage) {
if (source == null) {
throw new IllegalStateException(" (source == null) ");
}
if (errorMessage == null) {
throw new IllegalStateException(" (errorMessage == null) ");
}
this.source = source;
this.errorMessage = errorMessage;
this.exception = null;
}
What is the expected output? What do you see instead? Code like that:
public EmailableException(@ NotNull Object source, @ NotNull(customErrorMessage=" (source == null) ") String errorMessage) {
this.source = source;
this.errorMessage = errorMessage;
this.exception = null;
}

What version of the product are you using? On what operating system?
0.11.8

Please provide any additional information below.

@lombokissues

This comment has been minimized.

Show comment
Hide comment
@lombokissues

lombokissues Jul 14, 2015

Collaborator

👤 askoning   🕗 May 07, 2013 at 13:54 UTC

I'd say that Guava already provides for quite readable code here and removes the use for the boilerplate if-throw-block:
Preconditions.checkNotNull(source);

Personally I prefer that over an annotation on a constructor parameter.
Do a static import and your code can become as short as

public EmailableException(Object source, String errorMessage) {
this.source = checkNotNull(source);
this.errorMessage = checkNotNull(errorMessage);
this.exception = null;
}

Collaborator

lombokissues commented Jul 14, 2015

👤 askoning   🕗 May 07, 2013 at 13:54 UTC

I'd say that Guava already provides for quite readable code here and removes the use for the boilerplate if-throw-block:
Preconditions.checkNotNull(source);

Personally I prefer that over an annotation on a constructor parameter.
Do a static import and your code can become as short as

public EmailableException(Object source, String errorMessage) {
this.source = checkNotNull(source);
this.errorMessage = checkNotNull(errorMessage);
this.exception = null;
}

@lombokissues

This comment has been minimized.

Show comment
Hide comment
@lombokissues

lombokissues Jul 14, 2015

Collaborator

👤 vyacheslav.sahno   🕗 May 08, 2013 at 07:34 UTC

I think in so frequent situation to make programming easy every keyboard hit counts. That is why i think static import is long to do. Class name is long to write too, "check" method name prefix is redundant and braces too. I'd prefer to do @ NotNull + ctrl+space.

Collaborator

lombokissues commented Jul 14, 2015

👤 vyacheslav.sahno   🕗 May 08, 2013 at 07:34 UTC

I think in so frequent situation to make programming easy every keyboard hit counts. That is why i think static import is long to do. Class name is long to write too, "check" method name prefix is redundant and braces too. I'd prefer to do @ NotNull + ctrl+space.

@lombokissues

This comment has been minimized.

Show comment
Hide comment
@lombokissues

lombokissues Jul 14, 2015

Collaborator

👤 reinierz   🕗 May 16, 2013 at 17:49 UTC

I actually wanted something like this myself, so, I think this might be happening. However, two notes:

  • No configurable error message, that's CRAZY - parameter lists are already a convoluted mess, and throwing long strings in there is, in my opinion, bonkers. The error message will be the name of the parameter, which fortunately happens to match conventions.
  • The exception thrown will obviously be NullPointerException. IllegalStateException means something entirely different and is not at all appropriate. I can see an argument for IllegalArgumentException, but not a good one. NPE it is. This will not be configurable.

We'll probably put lombok.NonNull to good use for this; we currently have no defined semantics for sticking that on a method/constructor parameter.

So, in summary:

public void whateveryouwant(@ NonNull String x) {
// do stuff
}

becomes:

public void whateveryouwant(@ NonNull String x) {
if (x == null) throw new NullPointerException("x");
// do stuff
}

Collaborator

lombokissues commented Jul 14, 2015

👤 reinierz   🕗 May 16, 2013 at 17:49 UTC

I actually wanted something like this myself, so, I think this might be happening. However, two notes:

  • No configurable error message, that's CRAZY - parameter lists are already a convoluted mess, and throwing long strings in there is, in my opinion, bonkers. The error message will be the name of the parameter, which fortunately happens to match conventions.
  • The exception thrown will obviously be NullPointerException. IllegalStateException means something entirely different and is not at all appropriate. I can see an argument for IllegalArgumentException, but not a good one. NPE it is. This will not be configurable.

We'll probably put lombok.NonNull to good use for this; we currently have no defined semantics for sticking that on a method/constructor parameter.

So, in summary:

public void whateveryouwant(@ NonNull String x) {
// do stuff
}

becomes:

public void whateveryouwant(@ NonNull String x) {
if (x == null) throw new NullPointerException("x");
// do stuff
}

@lombokissues

This comment has been minimized.

Show comment
Hide comment
@lombokissues

lombokissues Jul 14, 2015

Collaborator

👤 reinierz   🕗 May 30, 2013 at 23:09 UTC

... aaaand, done!

will be in the next official release, and we just pushed an edge release if you want to get your hands dirty:

https://projectlombok.org/download-edge.html

Collaborator

lombokissues commented Jul 14, 2015

👤 reinierz   🕗 May 30, 2013 at 23:09 UTC

... aaaand, done!

will be in the next official release, and we just pushed an edge release if you want to get your hands dirty:

https://projectlombok.org/download-edge.html

@lombokissues lombokissues removed the accepted label Jul 14, 2015

@lombokissues lombokissues added this to the 0.11.10 milestone Jul 14, 2015

@lombokissues

This comment has been minimized.

Show comment
Hide comment
@lombokissues

lombokissues Jul 14, 2015

Collaborator

End of migration

Collaborator

lombokissues commented Jul 14, 2015

End of migration

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment