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)
{
if(isEmpty())
{
}
// customer to delete is first element in the list
{
}
// customer is not the first element in the list
else
{
// search until either the end of list is reached or a match is found
{
}
// if end of list is reached
{
// check if there is a match with last element in list
{
}
else
{
return null;
}
}
// match is found somewhere in the middle of list
else
{
}
}
numEntries--; // number of entries decrements each time a customer is deleted
return currentLink; // return the entry that was deleted
}

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

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))


...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)
• I noticed my own mistake and edited ;) – Ross Drew Jan 3 '14 at 11:14
• Uncle Bob would want to break this up into several more methods also. And maybe move stuff to the Link class. – froderik Jan 3 '14 at 11:20