How to eliminate repeat code in a for-loop?

Question

I have implemented two member functions in the same class:

  private static void getRequiredTag(Context context) throws IOException
  {
    //repeated begin
    for (Record record : context.getContext().readCacheTable("subscribe")) {
      String traceId = record.get("trace_id").toString();
      if (traceSet.contains(traceId) == false)
        continue;
      String tagId = record.get("tag_id").toString();
      try {
        Integer.parseInt(tagId);
      } catch (NumberFormatException e) {
        context.getCounter("Error", "tag_id not a number").increment(1);
        continue;
      }
      //repeated end
      tagSet.add(tagId);
    }
  }

  private static void addTagToTraceId(Context context) throws IOException
  {
    //repeated begin
    for (Record record : context.getContext().readCacheTable("subscribe")) {
      String traceId = record.get("trace_id").toString();
      if (traceSet.contains(traceId) == false)
        continue;
      String tagId = record.get("tag_id").toString();
      try {
        Integer.parseInt(tagId);
      } catch (NumberFormatException e) {
        context.getCounter("Error", "tag_id not a number").increment(1);
        continue;
      }
      //repeated end
      Vector<String> ret = traceListMap.get(tagId);
      if (ret == null) {
        ret = new Vector<String>();
      }
      ret.add(traceId);
      traceListMap.put(tagId, ret);
    }    
  }

I will call that two member functions in another two member functions(so I can't merge them into one function):

private static void A()
{
  getRequiredTag()
}
private static void B()
{
  getRequiredTag()
  addTagToTraceId()
}

tagSet is java.util.Set and traceListMap is java.util.Map.

I know DRY principle and I really want to eliminate the repeat code, so I come to this code:

  private static void getTraceIdAndTagIdFromRecord(Record record, String traceId, String tagId) throws IOException
  {
    traceId = record.get("trace_id").toString();
    tagId = record.get("tag_id").toString();
  }

  private static boolean checkTagIdIsNumber(String tagId)
  {
    try {
      Integer.parseInt(tagId);
    } catch (NumberFormatException e) {
      return false;
    }
    return true;
  }

  private static void getRequiredTag(Context context) throws IOException
  {
    String traceId = null, tagId = null;
    for (Record record : context.getContext().readCacheTable("subscribe")) {
      getTraceIdAndTagIdFromRecord(record, traceId, tagId);
      if (traceSet.contains(traceId) == false)
        continue;
      if (!checkTagIdIsNumber(tagId))
      {
        context.getCounter("Error", "tag_id not a number").increment(1);
        continue;
      }
      tagSet.add(tagId);
    }
  }

  private static void addTagToTraceId(Context context) throws IOException
  {
    String traceId = null, tagId = null;
    for (Record record : context.getContext().readCacheTable("subscribe")) {
      getTraceIdAndTagIdFromRecord(record, traceId, tagId);
      if (traceSet.contains(traceId) == false)
        continue;
      if (!checkTagIdIsNumber(tagId))
      {
        context.getCounter("Error", "tag_id not a number").increment(1);
        continue;
      }
      Vector<String> ret = traceListMap.get(tagId);
      if (ret == null) {
        ret = new Vector<String>();
      }
      ret.add(traceId);
      traceListMap.put(tagId, ret);
    }    
  }

It seems I got an new repeat... I have no idea to eliminate repeat in that case, could anybody give me some advice?

update 2015-5-13 21:15:12:

Some guys gives a boolean argument to eliminate repeat, but I know Robert C. Martin's Clean Code Tip #12: Eliminate Boolean Arguments.(you can google it for more details).

Could you gives some comment about that?

Answer 1

The parts that changes requires the values of String tagId and String traceId so we will start by extracting an interface that takes those parameters:

public static class PerformingInterface {
     void accept(String tagId, String traceId);
}

Then extract the common parts into this method:

  private static void doSomething(Context context, PerformingInterface perform) throws IOException
  {
    String traceId = null, tagId = null;
    for (Record record : context.getContext().readCacheTable("subscribe")) {
      getTraceIdAndTagIdFromRecord(record, traceId, tagId);
      if (traceSet.contains(traceId) == false)
        continue;
      if (!checkTagIdIsNumber(tagId))
      {
        context.getCounter("Error", "tag_id not a number").increment(1);
        continue;
      }
      perform.accept(tagId, traceId);
    }    
  }

Then call this method in two different ways:

private static void getRequiredTag(Context context) throws IOException {
    doSomething(context, new PerformingInterface() {
         @Override public void accept(String tagId, String traceId) {
              tagSet.add(tagId);
         }
    });
}

private static void addTagToTraceId(Context context) throws IOException {
    doSomething(context, new PerformingInterface() {
         @Override public void accept(String tagId, String traceId) {
             Vector<String> ret = traceListMap.get(tagId);
             if (ret == null) {
                 ret = new Vector<String>();
             }
             ret.add(traceId);
             traceListMap.put(tagId, ret);
         }
    });
}

Note that I am using lambdas here, which is a Java 8 feature (BiConsumer is also a functional interface defined in Java 8), but it is entirely possible to accomplish the same thing in Java 7 and less, it just requires some more verbose code.

Some other issues with your code:

• Way too many things is static
• The Vector class is old, it is more recommended to use ArrayList (if you need synchronization, wrap it in Collections.synchronizedList)
• Always use braces, even for one-liners

Answer 2

You could use a stream (haven't tested):

private static Stream<Record> validRecords(Context context) throws IOException {
    return context.getContext().readCacheTable("subscribe").stream()
        .filter(r -> {
            if (!traceSet.contains(traceId(r))) {
                return false;
            }
            try {
                Integer.parseInt(tagId(r));
                return true;
            } catch (NumberFormatException e) {
                context.getCounter("Error", "tag_id not a number").increment(1);
                return false;
            }
        });
}

private static String traceId(Record record) {
    return record.get("trace_id").toString();
}

private static String tagId(Record record) {
    return record.get("tag_id").toString();
}

Then could do just:

private static void getRequiredTag(Context context) throws IOException {
    validRecords(context).map(r -> tagId(r)).forEach(tagSet::add);
}

private static void addTagToTraceId(Context context) throws IOException {
    validRecords(context).forEach(r -> {
        String tagId = tagId(r);
        Vector<String> ret = traceListMap.get(tagId);
        if (ret == null) {
            ret = new Vector<String>();
        }
        ret.add(traceId(r));
        traceListMap.put(tagId, ret);
    });
}