How do I eliminate the retry code during a loop?
I have implemented two member functions in one 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 these two member functions two more member functions (so I cannot combine them into one function):
private static void A()
{
getRequiredTag()
}
private static void B()
{
getRequiredTag()
addTagToTraceId()
}
tagSet
- java.util.Set
, a traceListMap
- java.util.Map
.
I know DRY principle
and I really want to eliminate the retry code, so I ended up with 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);
}
}
I seem to have a new repetition ... I have no idea to eliminate the repetition in this case, can anyone give me some advice?
update 2015-5-13 21:15:12:
Some guys give a boolean argument for eliminating replay, but I know
Robert C. Martin Clean Code Tip #12: Eliminate Boolean Arguments.
(you can point it for more details).
Could you tell us about this?
source to share
The details that change require values String tagId
and String traceId
, so we'll start by extracting the interface that takes these 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 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 function ( BiConsumer
also a functional interface defined in Java 8), but it is perfectly possible to do the same in Java 7 or less, it just requires more verbose code.
Some other problems with your code:
- Too many things
static
- The class is
Vector
deprecated, it is recommended to use itArrayList
(if you need synchronization, wrap it inCollections.synchronizedList
) - Always use braces, even for one-line buttons
source to share
tagId seems to always be null on the second try.
However, one approach would be to extract the code that collects tagIds (it seems to be the same in both methods) into its own method. Then, in each of the two methods, simply iterate over the set of returned tags and perform different operations on them.
for (String tagId : getTagIds(context)) {
// do method specific logic
}
EDIT
Now I noticed that you are also using traceId in the second method. The principle remains the same, just collect the records in a separate method and iterate over them using two methods (taking tagId and traceId from records).
The lambdas solution is the most elegant, but without them it involves creating a separate interface and two anonymous classes, which are too verbose for this use case (to be honest, here I would rather go with a boolean argument than a strategy without a lambda).
source to share
You can use a stream (not 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 one could only do:
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);
});
}
source to share
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 another parameter boolean true if you want to add otherwise false to get it.
source to share