3
\$\begingroup\$

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

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

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.