Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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
 */

This is my solution

public boolean add(E o)  {

    if (o == null)
        throw new NullPointerException();

    ListNode newNode = new ListNode(o, null);
    if (contains(o))
    {
     return false;
    }
    else {

    head.next=newNode;
    head=newNode;
    return true;
   }     
}

Can please anybody check it and correct if there are mistakes, or give hints to correct it?

share|improve this question
1  
I feel like we are missing the big picture here. –  Marc-Andre May 1 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 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 Harkness May 1 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 at 17:39
3  
@DavidHarkness: This is no longer accepted practice. –  Jamal May 1 at 17:51
show 3 more comments

5 Answers 5

Let's say that you are starting with a list containing five elements:

$$ \newcommand{ptr}[1]{\overset{\mathtt{#1}}{\longrightarrow}} \mathtt{head} \ptr{} \fbox{first} \ptr{next} \fbox{second} \ptr{next} \fbox{rest} \ptr{next} \fbox{of} \ptr{next} \fbox{list} \ptr{next} \mathtt{null} $$

Then, you add a non-duplicate item called "another". After executing head.next=newNode:

$$ % "align" allows us to do vertical alignment % "\\" separates lines % "&" is an alignment marker. The markers in different rows % are positioned above each other, and horizontally centered. \newcommand{ptr}[1]{\overset{\mathtt{#1}}{\longrightarrow}} \begin{align*} &\mathtt{newNode} \\ &\quad\downarrow \\ \mathtt{head} \ptr{} \fbox{first} \ptr{next} &\fbox{another} \ptr{next} \mathtt{null} \\ &\fbox{second} \ptr{next} \fbox{rest} \ptr{next} \fbox{of} \ptr{next} \fbox{list} \ptr{next} \mathtt{null} \end{align*} $$

You've just orphaned and discarded everything starting from the second element. Then it gets worse when you set head=newNode.

share|improve this answer
5  
+1 for MathJax awesomeness (and bookmarking this one for reference!) –  Mat's Mug May 1 at 17:08
    
This make me wonder if the OP has tested the code before posting. Just by using his implementation a bit he would have seen that his implementation is missing elements. –  Marc-Andre May 1 at 17:12
1  
@Mat'sMug Shared on Meta for everyone's benefit. –  200_success May 1 at 17:15
add comment

The first big problem I see is readability. Your code is missing good indentation and the style does not conform with what I'm used to seeing. Your code would read better like this:

public boolean add(E o)  {
    if (o == null) {
        throw new NullPointerException();
    }

    ListNode newNode = new ListNode(o, null);

    if (contains(o)) {
        return false;
    } else {
        head.next=newNode;
        head=newNode;
        return true;
   }     
}

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:

public boolean add(E o) {
    if (o == null) {
        throw new NullPointerException();
    }

    if (contains(o)) {
        return false;
    }

    ListNode newNode = new ListNode(o, null);
    head.next = newNode;
    head = newNode;
    return true;
}
share|improve this answer
    
There are many commonly-used styles for code, so the one you're used to isn't the only option. –  Ypnypn May 1 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-Andre May 1 at 21:55
add comment

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.

share|improve this answer
add comment

throw new NullPointerException();

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:

if (o == null && !isNullSupported()) {
    throw new NullPointerException();
}

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.

share|improve this answer
add comment

The "else" part of the if-else is redundant, since the if contains a return statement.

Also what happens when you add the first element? If head is null, the head.next=newNode line will give a NullPointerException.

share|improve this answer
2  
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. –  skiwi May 1 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. –  Geobits May 2 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. –  skiwi May 2 at 9:37
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.