current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 5 down vote favorite

How can I refactor my code to remove the label and the need for it? I am aware that a loop with label is "forbidden". But I can not find a way to rewrite this without the label.

private List<myList> sortLinks(SegmentType s, Set<myList> LinkSet) {
        List<myList> LinkList = new LinkedList<myList>();

        String dep = s.getDep().toString();
        mainLoop: for (int index = 0; !LinkSet.isEmpty(); index++) {

            for (Iterator<myList> iterator = LinkSet.iterator(); iterator.hasNext(); ) {
                myList link = iterator.next();
                if (link.getLegDep().toString().equals(dep)) {
                    iterator.remove();
                    link.setLine(s.getLineCode());
                    link.setNb(s.getNb());
                    link.setSuff(s.getSuff());
                    link.setIndex(index);
                    linkList.add(link);

                    dep = link.getDest().toString();
                    continue mainLoop;
                }
            }

            return Collections.emptyList();
        }
        return linkList;
    }
share|improve this question

3 Answers 3

up vote 5 down vote accepted

In general, labelled code is uncommon, but 'forbidden' is a bit harsh. Break, and Continue have better characteristics than GoTo, and should not be 'painted with the same brush'.

The logic in your code is convoluted though.... you are 'searching' the input set, and ensuring that all members of the input set match a condition. As you search, if the member matches, you remove the member. If one of the members does not match, you immediately return an empty result.

Note, that if a 'middle' member fails to match, you have already removed the first members, and yet you return an empty set.

Your logic could be significantly simplified if you extracted part of your method as a 'helper' function:

private myList searchAndRemove(String dep, Set<myList> candidates) {
    for (Iterator<myList> iterator = candidates.iterator(); iterator.hasNext(); ) {
        myList link = iterator.next();
        if (link.getLegDep().toString().equals(dep)) {
            iterator.remove();
            return link;
        }
    }
    return null;
}

private List<myList> sortLinks(SegmentType s, Set<myList> LinkSet) {

    List<myList> matched = new LinkedList<myList>();

    String dep = s.getDep().toString();
    int index = 0;
    while (!LinkSet.isEmpty()) {
        myList link = searchAndRemove(dep, LinkSet);
        if (link == null) {
            return Collections.emptyList();
        }
        matched.add(link);
        link.setLine(s.getLineCode());
        link.setNb(s.getNb());
        link.setSuff(s.getSuff());
        link.setIndex(index++);

        dep = link.getDest().toString();
    }

    return matched;
}

In addition to the above, note that your class names and variable names are really, really bad....

  • myList is a class, and should be CapitalCamelCase naming style, and have a different name.
  • LinkList is a variable name, and should be lowerCamelCase, and have a name like matchedSegments
  • LinkSet is also a variable name, and should be lowerCamelCase.

In general, with the name-pollution of having a class with 'Link' in the name (probably because it's not a horrible name', but it conflicts with LinkedList, so try to avoid Objects with Link as the name.

Additionally, note that your index variable was not an index in to the source data, but an index of the output segment. Including it as part of the original 'for' loop implied that it was used as an index in to that. The reality is that it is independent. I have edited it to make that clear.

share|improve this answer
    
Using a helper method in cases like this is an excellent approach! (At least that's what I've done a couple of times) –  Simon Forsberg♦ Nov 5 '14 at 22:55
    
Great, thank you very much for this answer. –  hacks4life Nov 6 '14 at 9:14
up vote 2 down vote

Adding break; will not be sufficient, because then the return Collections.emptyList(); statement will be executed when it should not. In my opinion it should look like:

(...)
String dep = s.getDep().toString();
boolean found = false;
for (int index = 0; !LinkSet.isEmpty(); index++) { 
(...)
        found = true;
        break; // these two lines instead of continue statement
    }
}
if(!found) 
    return Collections.emptyList();
share|improve this answer
up vote 4 down vote

See the other answers for the label issue. Assuming this is actual code that you use in your programs, I'll continue with other issues.

Consider replacing

for (Iterator<myList> iterator = LinkSet.iterator(); iterator.hasNext(); ) {
      myList link = iterator.next();

with a foreach loop

for (myList link : LinkSet)

I'd also try to properly case names. If myList is a class, it would be properly named as MyList. LinkSet is a parameter, rename it to linkSet. Even the StackExchange highlighter confuses it with a type, which should suggest something about the naming issues :)

I'd also look into why myList is called "my list". You do this:

myList link;
link.setLine(s.getLineCode());
link.setNb(s.getNb());
link.setSuff(s.getSuff());
link.setIndex(index);

Which is a set of operations that are hard to expect on a list. Are you sure myList is actually a list? How is a link, a list?

You should also attempt to give more descriptive names to other things, among which:

String dep = s.getDep().toString(); //dep? department?
link.setNb(s.getNb()); //what's Nb?
link.setSuff(s.getSuff()); //what stuff is that?
share|improve this answer

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.