Discussions

J2EE patterns: Replace Iteration With Indexing

  1. Replace Iteration With Indexing (7 messages)

    You are iterating over a collection or array using the enhanced for loop, and you need to have access to the iterator or index variable in order to make some operation with it.

    Replace the for-each construct with a classic for loop.

    Replace this:

     int locate(int[] values, int value) {
         int index = 0;
         for(int v : values) {
             if (v == value) {
                 return index;
             }
             index++;
         }
         return -1;
     }


    With this:

     int locate(int[] values, int value) {
         for(int index = 0; index < values.length; index++) {
             if (values[index] == value) {
                 return index;
             }
         }
         return -1;
     }

    Motivation

    In some situations, the use of the enhanced for loop, also called for-each, is not suitable for iterating over collections and arrays. In the example above, the code shows a method that finds a value into an array and returns its position. The for-each construct hides the index variable, so to use it, we need to create a temporary variable and increment it on each iteration to later return the value position. This process is automatically carried out by a traditional for loop, so it's better for this kind of problem.

    Another situation when we should decline to use iteration is when you need to replace elements in a collection or array as you traverse it. Also, the for-each construct is not usable for loops that must iterate over multiple collections in parallel.

    Using the wrong loop may cause behavioral problems or, in the best case, decrease code clarity.

    Mechanics

    Check that you really needs to have access to the iterator or index variable.
    If not is so, you shouldn't trunc the for-each construct.
    Replace the for-each loop construct with a for loop construct.
    Compile and test.

    Example

    Lets look at this example where we want to expunge all empty strings from a linked list:

     void expunge(LinkedList<String> list) {
         int index = 0;
         for(String value : list) {
             if (value.equals("")) {
                 list.remove(index);
             }
             index++;
         }
     }


    The code above seems to be right, but it isn't. Run it and you'll get a ConcurrentModificationException?. The problem is that the for-each construct uses a fail-fast iterator to move over collections, so using a for-each loop you can't remove elements while iterating over any collection without getting a ConcurrentModificationException?.

    To get the proper behavior, and simplify the method, apply ReplaceIterationWithIndexing:

     void expunge(LinkedList<String> list) {
         for(Iterator<String> iterator = list.iterator(); iterator.hasNext();) {
             if (iterator.next().equals("")) {
                 iterator.remove();
             }
         }
     }


    I compile and test, and no exception is thrown. Looking at the code, we don't need anymore the temporary index variable because we are using an iterator embedded with the for loop construct. The code works fine, and it shows its intention in a clearer manner.

    Acknowledgments:

    Thanks to J. B. Rainsberger (Diaspar Software Services) for the names of these refactorings.
  2. If you want to propose a refactoring, you should first see if it is used. I don't see why anyone would apply this as it's simply modifying code from one syntax to another. Unless you plan on performing an experiment on evaluating code that was versus wasn't refactored, you can't speculate on what is conducive to being fault-prone.

    The examples depend too much on a specific syntax (and a specific version of it). I've for one, never seen an enhanced loop in Simula. At least add a reference to some other examples like Smalltalk (or Squeak). Make sure this refactoring is general as this is for your thesis, no?.

    sv
  3. If I am rewritting a portion of existing code, I might consider using the new syntax. If the code works, rewritting it in order to use a new syntax feature is an extremely risky thing to do especially if the original version is obfuscated. The most likely change to come of it is a new bug.
  4. If the original version is obfuscated then surely you have all the more reason to refactor it, so that you can actually understand it.
  5. hi,

    Nice example. Although it is not good enough to motivate for refractoring, it presents an alternative approach to looping through collection.

    Talking about “cleaner approach” and “I compile and test, and no exception is thrown”, I guess something is missing in above piece of code.

    To make it safe in all conditions I suggest following change.

    if( null != myCollection){
    for (Iterator iter = myCollection.iterator(); iter.hasNext(); ) {
    MyType myElem = (MyType) iter.next();

    }
    }

    Here I have enveloped for() loop in the if() condition that checks for “null”. Although seems trivial such small condition, when missing can put you in jam when your application goes live.

    I have set above piece of code as template in my Eclipse.

    Hope this helps,
    Shrini
  6. Replace Iteration With Indexing[ Go to top ]

    hi,Nice example. Although it is not good enough to motivate for refractoring, it presents an alternative approach to looping through collection.Talking about “cleaner approach” and “I compile and test, and no exception is thrown”, I guess something is missing in above piece of code. To make it safe in all conditions I suggest following change. if( null != myCollection){ for (Iterator iter = myCollection.iterator(); iter.hasNext(); ) { MyType myElem = (MyType) iter.next(); } }Here I have enveloped for() loop in the if() condition that checks for “null”. Although seems trivial such small condition, when missing can put you in jam when your application goes live.I have set above piece of code as template in my Eclipse. Hope this helps,Shrini

    It's not necessarily a good idea to do this in every case. It may be the case that the collection being null is indicative of a really big problem. Instead of failing, the code skips over this and continues on as if everything is OK. The user or system that called it, continues on thinking that the method was successful. This could cause data corruption or wasted hours of work.

    Which do you think is worse for a user, having an application crash, or finding out that an entire days work was not saved or has been screwed up?

    I work with a system written by people who live by the 'it's better not to crash' mentality. Something goes wrong and it just keeps on processing. Because it's 'better' not to crash. When a new piece of code was deployed with a small bug, it didn't crash. It created thousands of bad records requiring hundreds of man hours to resolve. If it had just failed, the error would have been apparent immediately and the new code rolled-back.

    I'm not saying you shouldn't ever check for nulls but if the assumtion of the system is that the list is not null, it's important to fail if that is not the case. Throwing a NPE isn't the most elegant solution but it works. Exceptions are not something to avoid at all costs, they are actually quite helpful. Your template may keep you from finding a bug in development or testing. You'll find out after it's in production and causing real problems.
  7. When you define a word, you must be very careful not to use the word within the definition or you will eventually run into cases where the definition looks like:

    Car: (n) A car.

    In the motivation of this pattern, there is no driving force to make the change. That is because the motiviation looks like:

    Motivation: Sometimes it's better to change it this way.

    In some cases, the author may be correct in making this statement, but it's not a useful statement. A useful motivation would address the reoccuring problem that this pattern mitigates, and would describe the forces which act on the motivation, leading the reader to know when to apply the pattern.

    As it stands, I would say that a circular motivation ("Do it, because sometimes you have to do it") robs the pattern of its potential usefulness, and provides no justification for deciding whether to use one technique or the other, and no hint as to the detrimental trade-offs between the two techniques.

    Without justification of why one reason solves certain problems or protects against certain issues, there is no ability to defend the choice of iterators over indexing. It becomes a matter of preference, much like the positioning of curly braces, or the number of spaces to indent code.

    It seems that this is not a pattern at all, but a refactoring technique.
  8. No a pattern[ Go to top ]

    +1 to "Circular resoning in motivation makes this pattern suspect"

    Firstly, the motivation statement is not that good, not that precise.

    Secondly, It's not a pattern at all:

    1) It's all about syntax, Java syntax. When you'r talking about O-O pattern, it should at least apply to all O-O languages. And when you'r talking about basic pattern, it should apply to most language. Never stick to syntax.

    2) Not useful and not helpful when you come to make the final decision - you don't know use it or not to use it for a particular situation. Both ways are alternative - not that suggestive to make a right choice.