4
\$\begingroup\$

I'm curious whether checking for null is redundant in this concrete example.

 public LoginJob(@NotNull String login, @NotNull String password) {
        super(new Params(Priority.NORMAL).requireNetwork().persist());
        Preconditions.checkNotNull(login, "login is null or empty");
        Preconditions.checkNotNull(password, "password is null or empty");

        this.login = login;
        this.password = password;
    }

Please notice I'm using Contract Annotator from JetBrains, so the User of a Class warned the LoginJob constructor can't take null values, it can't prevent him/her of doing so though.

Improve this question
\$\endgroup\$
1
  • 1
    \$\begingroup\$ The only time null checks are unnecessary is if you ban null from your entire code base. Then you only need to check for null where your code interacts with third party code. \$\endgroup\$
    – Doval
    Jun 19 '14 at 16:02
5
\$\begingroup\$

You said it yourself in your question :

it can't prevent him/her of doing so though.

That settle it for me. You still have the responsibility to prevent that null value are pass to your classes. The annotation seems to be about preventing NullPointerException. So you should be checking for null values even if you use @NotNull. 

    Preconditions.checkNotNull(login, "login is null or empty");
    Preconditions.checkNotNull(password, "password is null or empty");

Your message or the method you use is lying. The method is called checkNotNull but you warn about empty String, this is two very different things. Either you should modify you message to inform that you're only checking null values or you could change/rename the method checkNotNull to better represent the empty check.

Improve this answer
\$\endgroup\$
3
  • \$\begingroup\$ Isn't that the user's fault if he/she is ignorant about using the method? You tell him not to provide null values (, or unexpected behavior might occur). If he does, he's in trouble, and it's his own fault. However, a clear note that the project uses this approach might be necessary. \$\endgroup\$
    – nhaarman
    Jun 19 '14 at 19:03
  • 1
    \$\begingroup\$ @NiekHaarman You could do that kind of approach, I think it's indeed a valid way of doing things (You could add this point as an answer). I would prefer to use something that fails early and gracefully over one that could break somewhere I didn't expect. Preconditions.checkNotNull throw a NullPointerException so it will behave "as if" he would not had made the check. I"m not sure but I believe that the @NotNull annotation the OP use is "valid" only in Intellij, so user with Eclipse could potentially not see those warnings (I'm very not sure about this point). \$\endgroup\$
    – Marc-Andre
    Jun 19 '14 at 19:12
  • 1
    \$\begingroup\$ You have a very good point there: I would prefer to use something that fails early and gracefully. I'm not very happy about cluttering the code with these precondition checks though (Although, I must say, using Preconditions.checkNotNull solves a bit of the cluttering over if statements). \$\endgroup\$
    – nhaarman
    Jun 19 '14 at 19:22

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.