Removing a link from a linked list

Question

I have written a method for removing a link from a linked list for a phonebook project I'm currently working on and it works perfectly. However, I want to know whether or not my code is acceptable from a programmer's perspective. I tried to systematically structure it in a way such that it's understandable, but by doing so, this results in multiple if statements being nested inside one another.

Is this considered bad coding? Also, should I have used a recursive implementation instead? What do you think? Are there any ways to improve this code? Any constructive criticism is certainly welcome.

public Link removeLink(String surname, String firstName)
{
 Link currentLink = firstLink;
 Link previousLink = firstLink;
 if(isEmpty())
 {
 return null; // Name was not found
 }
 // customer to delete is first element in the list
 else if((currentLink.surname).equals(surname) && (currentLink.firstName).equals(firstName))
 {
 firstLink = firstLink.next;
 }
 // customer is not the first element in the list
 else
 {
 // search until either the end of list is reached or a match is found
 while(currentLink.next!=null && !((currentLink.surname).equals(surname) && (currentLink.firstName).equals(firstName)))
 {
 previousLink = currentLink;
 currentLink = currentLink.next;
 }
 // if end of list is reached
 if(currentLink.next == null)
 {
 // check if there is a match with last element in list
 if((currentLink.surname).equals(surname) && (currentLink.firstName).equals(firstName))
 {
 previousLink.next = currentLink.next;
 }
 else
 {
 return null;
 }
 }
 // match is found somewhere in the middle of list
 else
 {
 previousLink.next = currentLink.next;
 }
 }
 numEntries--; // number of entries decrements each time a customer is deleted
 return currentLink; // return the entry that was deleted
}

• Dan: Why do you need to reimplement linked list yourself? There's plenty implementations available, and I'm sure Java runtime has one as well.
• Alan: @Dan Abramov It's part of the learning process
• froderik: me likes curly braces on new line!
• froderik: could be written recursively. slower but probably less code
• Alan: if my phonebook had a million entries, that stack build-up is going to be huge, so I decided not to write it recursively

Answer 1

Code is fine I think, although according to Robert C. Martin, generally, if you need to add a comment then you have failed to write readable code.

For example, this could be made a little easier to read and understand by extracting the block

else //customer is not the first element in the list 

into a separate method. Also the long tests such as

if ((currentLink.surname).equals(surname) && (currentLink.firstName).equals(firstName))
while (currentLink.next!=null && !((currentLink.surname).equals(surname) && ((currentLink.firstName).equals(firstName)))
if (currentLink.surname).equals(surname) && (currentLink.firstName).equals(firstName))

...contain duplicated code, namely

(currentLink.surname).equals(surname) && (currentLink.firstName).equals(firstName)

which would be better extracted into, say testForMatch()

In Summary

• Long methods are frowned upon
• Repeated code is hard to understand and harder to maintain
• Extracting into well named methods beats good comments any day (comments can be left behind during updates and cause more confusion than is necessary with just code)

• Ross Drew: I noticed my own mistake and edited ;)
• froderik: Uncle Bob would want to break this up into several more methods also. And maybe move stuff to the Link class.