My task is to add the specified element to the linked list if it is not already present

/**
* Adds the specified element to this set if it is not already present.
* @param o element to be added to this set.
* @return true if this set did not already contain the specified element.
* @throws NullPointerException - if the specified element is null and this set does
not support null elements
*/

I feel like we are missing the big picture here.
–
Marc-AndreMay 1 '14 at 15:39

In typical terminology, a "set" is a list that can contain no duplicate items. I've interpreted and retitled your question accordingly.
–
200_success♦May 1 '14 at 16:43

2

"Check it and correct if there are mistakes" doesn't inspire a lot of confidence in the correctness of this code, the first requirement of a good question on this site.
–
David HarknessMay 1 '14 at 17:33

2

Please do not edit the code in the question in a way that invalidates existing answers. I've rolled back Rev 4 → 3.
–
200_success♦May 1 '14 at 17:39

I've removed some of the unneeded white space lines, since it was creating some weird space in the code. This can be considered style, but you should try to come up with something that feels natural.

Brackets {} have a standard in Java; you should follow it.

I've added brackets to the if with the throw exception. Note that this is optional and a matter of personal style.

Another thing that you could do is reorganize the method a bit. At the moment, you first verify if o is null, then you create newNode and after you check if o is already in the list. If o is in the list, you return false from the method and the new node will be discarded. So why would you create newNode if you're not sure if you will use it later on?

One option could be to move ListNode newNode = new ListNode(o, null); to the else block. You could also remove the else part and move the if (contains(o)) just before the initialization, leaving something like this:

There are many commonly-used styles for code, so the one you're used to isn't the only option.
–
YpnypnMay 1 '14 at 20:57

1

@Ypnypn perfectly right, but there were no organization in the original code. I presented one of the most common style for Java, but was still mentioning that there was alternative to what I am presenting.
–
Marc-AndreMay 1 '14 at 21:55

There are a few inconsistencies in your JavaDoc comments (bravo! for using them):

Parameter and exception descriptions don't need to be complete sentences and should not end with a period. This helps keep them short, too.

Omit the - after the exception type. These lines should look just like the @param lines.

It's typical to indent the * one space extra so the asterisks form a corner, and make sure every line has an asterisk.

I like to place a blank line between the method's description and the first attribute (@param in this case), but that's a personal style chosen to aid my poor vision.

Taking these all together produces this:

/**
* Adds the specified element to this set if it is not already present.
*
* @param o element to be added to this set.
* @return true if this set did not already contain the specified element
* @throws NullPointerException if the specified element is null
* and this set does not support null elements
*/

Since this set doesn't support null elements, you can drop the "and this set does not support null elements" at the end.

The other reviewers cover other bits sufficiently well, so I just want to point out how odd this looks.

A null pointer exception typically occurs when you try to access a null Object using the dot operator. Ie:

Object obj = null;
obj.toString(); // NullPointerException!!!

In your case, however, you're not accessing an object. You're simply checking if it's null or not.

Also, the rule of thumb is that exceptions should only be thrown when something exceptionally bad happens. Is receiving a null object in this function exceptionally bad? (That's for you to decide). If you regularly expect attempts to insert null objects, then I would say that throwing an exception is extreme, and you should just return false instead.

Also, the javadoc:

@throws NullPointerException - if the specified element is null and this set does not support null elements

David Harkness pointed this out, but I'll reiterate since I'm on the exception anyway. You say "and this set does not support null elements", which either suggests some sets can have a null element, or you're saying that this set class doesn't allow null elements. If some sets are allowed a null element, you need to change your if statement to this:

Regardless, this bit of documentation would be more fitting in the class' javadoc instead of in this exception's javadoc. Knowing whether a collection takes null values or not is one of the first things people will want to know when they use your code.

This answer is incorrect. The contents of the if-branch have nothing to do with the else-branch, hence one cannot be rendered redundant over what another branch does.
–
skiwiMay 1 '14 at 17:10

@skiwi Since the only thing the if branch contains is return false, there is no need for the rest of the code to be in an else block. See Marc-Andre's answer, for example. This may not be explained very well here, but I don't see how it's incorrect.
–
GeobitsMay 2 '14 at 1:32

@Geobits That doesn't make the else part redundant, that makes the else { ... } around the statement be redundant. This answer here is very ambigious as it does not even provide a code sample to demonstrate it.
–
skiwiMay 2 '14 at 9:37