CODEWITHSUNDEEP
javadry
Sayalic
Sayalic

Reputation: 7650

How to eliminate repeat code in a for-loop?

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?

Upvotes: 3

Views: 181

Answers (4)

Dragan Bozanovic
Dragan Bozanovic

Reputation: 23552

tagId seems to be always null in your second attempt.

Nevertheless, one approach would be to extract the code that collects tagIds (this seems to be the same in both methods) into its own method. Then, in each of the two methods just iterate over the collection of returned tagIds and do different operations on them.

for (String tagId : getTagIds(context)) {
// do method specific logic
}

EDIT

Now I noticed that you also use traceId in the second method. The principle remains the same, just collect Records in a separate method and iterate over them in the two methods (by taking tagId and traceId from records).

Solution with lambdas is the most elegant one, but without them it involves creation of separate interface and two anonymous classes which is too verbose for this use case (honestly, here I would rather go with a boolean argument than with a strategy without lambdas).

Upvotes: 1

Simon Forsberg
Simon Forsberg

Reputation: 13331

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

Upvotes: 2

Bubletan
Bubletan

Reputation: 3863

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

Upvotes: 1

prashant thakre
prashant thakre

Reputation: 5147

Try this approach

private static void imYourNewMethod(Context context,Boolean isAddTag){
        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;
          }
        if(isAddTag){
            Vector<String> ret = traceListMap.get(tagId);
              if (ret == null) {
                ret = new Vector<String>();
              }
          ret.add(traceId);
          traceListMap.put(tagId, ret);
        }else{
            tagSet.add(tagId);
        }
    }

call this method and pass one more parameter boolean true if you want to add otherwise false to get it.

Upvotes: -1

Related Questions