4
\$\begingroup\$

I have two JSON and each JSON I am loading in a Map.

First JSON

{
    "abc": "2",
    "plmtq": "hello+world",
    "lndp": "def+ooo",
    "yyt": "opi"
}

I am loading this JSON in a map as key:value pair. Here abc is the key and 2 is the value. Let's call this mapA. This is a String to String map.

Second JSON

{
  "abc": {
      "renameTo": "count"
  },
  "plmtq": {
      "renameTo": "title",
      "decode": "true"
  },
  "lndp": {
      "renameTo": "cross",
      "copyTo": "browse",
      "decode": "true"
  }
}

I am also loading this JSON in a map. Let's call this as mapB. And this is Map<String, Transform>. For example: Here key will be abc and value will be Transform class which has another map and it will have renameTo as the key and count as the value.

Now I need to use these two maps and make a new immutable map by doing some transformations which will look like this and it will be of String and String as well and I am doing all the transformations in the apply method of my Transform class (as shown below).

This will be the content of the new map after all transformations:

count=2
title=hello world // as you can see this got URL decoded with UTF-8 format.
cross=def ooo  // as you can see this got URL decoded with UTF-8 format.
browse=def ooo  // as you can see this got URL decoded with UTF-8 format.
yyt=opi

For example:

  • Here renameTo means value of abc will go into count variable.
  • Similarly value of plmtq goes into title variable but with URL decoded as decode field is true.
  • Similarly, the value of lndp will go into cross and browse field but with the URL decoded as that field is true as well. Here copyTo means that we will copy value of lndp into browse field. (I guess instead of using copyTo here, I can use same renameTo field as well and then it becomes like this "renameTo": ["cross", "browse"], but for now I will go with what I have.)

My Transform class looks like this and tasks maps for example contains renameTo as the key and count as the value.

public class Transform {
  public static final Transform NO_OP = new Transform(ImmutableMap.<String, String>of());
  private final Map<String, String> tasks;

  public Transform(Map<String, String> tasks) {
    // could add some sanity checks here that tasks.keySet() doesn't
    // contain any unexpected values
    this.tasks = tasks;
  }

  public Map<String, String> apply(Entry<String, String> entry) {
    Map<String, String> transformedMap = Maps.newHashMap();
    List<String> keys = Lists.newArrayList();
    String value = entry.getValue();
    if (MapUtils.isNotEmpty(tasks)) {
      for (Entry<String, String> task : tasks.entrySet()) {
        switch (task.getKey()) {
          case "renameTo":
          case "copyTo":
            keys.add(task.getValue());
            break;
          case "decodeValue":
            if (Boolean.valueOf(task.getValue())) {
              value = URLDecoder.decode(value, "UTF-8");
            }
            break;
          // and so on
          default:
            throw new IllegalArgumentException("Unknown task " + task.getKey());
        }
      }
    } else {
      transformedMap.put(entry.getKey(), value);
    }

    for (String key : keys)
      transformedMap.put(key, value);
    return transformedMap;
  }
}

So each tag in switch statement has special meanings and I need to perform certain operations accordingly. Right now it has two but in general it has 7-8 exactly similar to these.

Here is how I call it:

  private static Map<String, String> generateNewMap(Map<String, String> mapA,
      Map<String, Transform> mapB) {
    Map<String, String> newMap = Maps.newHashMap();
    for (Entry<String, String> entry : mapA.entrySet()) {
      Transform t = mapB.get(entry.getKey());
      if (t == null) {
        t = Transform.NO_OP; // just passes the entry through
      }
      Map<String, String> decoded = t.apply(entry);
      newMap.putAll(decoded);
    }

    return newMap;
  }

I'd like to know if there is any better and efficient way to achieve this thing. I am also open for any other format for my second JSON which can make this problem simpler to solve if it helps. For now, I wanted to see how I can improve my code based on the structure I have.

I am still on Java 7.

\$\endgroup\$
  • \$\begingroup\$ Just out of curiousity, what is the reason behind this JSON-format? Where does it come from? What is the use-case? \$\endgroup\$ – Simon Forsberg Dec 17 '16 at 19:57
  • \$\begingroup\$ I have a Kafka consumer which is listening to Kafka cluster, so first JSON comes from that and second JSON is in our database which we poll every 30 minutes so basis on second JSON we transform what we get in first JSON and make a new map which we then avro serialize it. \$\endgroup\$ – david Dec 17 '16 at 19:58
4
\$\begingroup\$

I like the way you solved this problem. I do have some qualms about the code and the mechanism though:

Used Data-Structures

The whole code is stringly typed. This may be useful now, but I doubt it is for maintenance purposes. Instead you should consider encapsulating transformation tasks into a separate class. This allows you to use a structure that's more object-focused, and not quite that imperative.

Currently the code has the problem of being overly reliant on magic strings. I'd instead expect a possibility to invoke tasks independently of each other and an aggregate result. The problem with that is that your operations suddenly are depending on the order of appearance. Consider the following transformation request:

"lndp": {
    "copyTo": "crossEncoded",
    "decode": "true",
    "renameTo": "cross"
}

What I'd hope to do with this is generating the following final result:

crossEncoded="def+ooo"
cross="def ooo"

note that the copied value was not decoded. This would be helpful for certain use-cases. I'm not sure in how far these are relevant, but currently the order of tasks is irrelevant for your code. This is IMO undesirable.

Consider the following:

public interface Task {
    void preApply(String value);
    String apply(String value); // alternatively pass the whole Entry
    void postApply(String value);
    Optional<Map.Entry<String,String>> collectResult();
}

This is a very extensible idea to encapsulating your transformations. It also allows you to drastically simplify your code:

public Map<String, String> transform(Map.Entry<String,String> original) {
    Map<String, String> result = new HashMap<>();
    String value = original.getValue();
    for (Task task : tasks) {
        task.preApply(value);
        value = task.apply(value);
        task.postApply(value);
        Optional<Map.Entry<String,String>> res = task.collectResult();
        if (res.isPresent()) {
            result.put(res.getKey(), res.getValue());
        }
    }
    return result;
}

This allows you to pass NO_OP a task that changes nothing, which eliminates the if-else branch in the current code.

If you don't see the simplification yet, then you don't have enough Tasks in your switch-statement. This will never get longer as Tasks change. You encapsulate Task behaviour into separate Objects. Some people call this a "chain of responsibility", some call it "actor pattern". Whatever you want to call it, I think it significantly improves the readability of the code, because it stops the intermingling of abstraction levels.
In case you want to keep the current behaviour, it should be obvious where you need to adjust this pattern. I do strongly recommend against it, though, because Transformations are usually not independent of order.

Using a List for the result keys is a mediocre choice. Instead I'd expect a Set. This makes it abundantly clear that you're not expecting these to have duplicates, not that it'd make any difference unless Transforms were containing the same key multiple times. In that case you even get a tangible benefit.

One last word on Data Structures: "decode": "true" is an ugly artifact of the fact that you assumed JSON is properly encodable as a Map<String,String>. It isn't. Javascript primitive literals are not encoded as Strings. Your abstraction is leaking. As soon as the supplier of your JSON actually starts sending valid JSON, your code will start failing :(

Arrow Code

A small thing I additionally noticed is the tendency to have deep nesting in the code, as well as a missing out on "cheaper" or "cleaner" library methods.

Instead of:

if (MapUtils.isNotEmpty(tasks)) {
    // lots of code
} else {
  transformedMap.put(entry.getKey(), value);
}

consider the following:

if (tasks.isEmpty()) {
    transformedMap.put(entry.getKey(), entry.getValue());
    return transformedMap;
}
// lots of code

And instead of tediously checking for null yourself here:

  Transform t = mapB.get(entry.getKey());
  if (t == null) {
    t = Transform.NO_OP; // just passes the entry through
  }

use the shorter and cleaner method available since java 8:

Transform t = mapB.getOrDefault(entry.getKey(), Transform.NO_OP);

On Constructors And Local Variables

You seem to have an aversion to Constructors. Whereever I'd have expected a new Something<>() there is a Somethings.newSomething(). This works because of type-erasure, but it should produce compiler warnings, because you can't guarantee that the factory method returns a Generics-Matching instance of your Object. Java is a little crazy like that, but in at least 70% of the cases, using the constructor is the correct solution over using Somethings.*. There is some exceptions, but these are not relevant here. The only thing you "gain" from using these methods is the overhead of a method call in addition to a constructor call :/

Additionally there's no need to extract method call results into a variable.

Java 8 features

Finally your calling code could be rewritten to use java 8 streams. This does similarly apply to your current code, but is not that easy for the code I proposed there. Well in either case, here's how I'd write your calling code:

return mapA.entrySet().stream().map(entry -> {
    Transform t = mapB.getOrDefault(entry.getKey(), Transform.NO_OP);
    return t.apply(entry);
}).flatMap(result -> result.entrySet().stream())
.collect(Collectors.toMap(Entry::getKey, Entry::getValue));

alternatively (and a little closer to the original code):

Map<String,String> newMap = new HashMap<>();
mapA.entrySet().stream().map(entry -> {
    Transform t = mapB.getOrDefault(entry.getKey(), Transform.NO_OP);
    return t.apply(entry);
}).forEach(newMap::putAll);
return newMap;
\$\endgroup\$
  • \$\begingroup\$ I am still reading your suggestions but one thing I would like to mention. I am still on Java 7 and cannot move to Java 8 yet. That's one problem for me. If you can provide suggestions around Java 7 as well, then that will help me a lot here. \$\endgroup\$ – david Dec 17 '16 at 20:45
  • \$\begingroup\$ yeesh ... java 7 has been EOL since over one and a half years now :( Well there's some other places I used java 8. Most notably in my proposed code: Optional should be removed and isPresent() replaced with a simple null-check. every other usage of java 8 is explicitly marked as such :) \$\endgroup\$ – Vogel612 Dec 17 '16 at 21:06
  • \$\begingroup\$ Optional I can still use through guava as I am using it in lot of other places. I basically meant for your last two code samples. And looking closely I think what I have should be ok. \$\endgroup\$ – david Dec 17 '16 at 21:08
  • \$\begingroup\$ There are lot of things which are a great suggestions. Thanks for writing extensive feedback on my code. I saw you suggested to have an interface and then I should have all my tasks class implementing that interface and then applying transformation, correct? Instead of having separate classes for each Task, can I embed your same logic in the current code I have? I think you mentioned that it is obvious but somehow I am not able to grasp that. If you can provide an example how would that work, it will help me to understand how to use that. Btw, I have around 10-12 tasks mostly all similar. \$\endgroup\$ – david Dec 17 '16 at 21:17
  • \$\begingroup\$ The problem with the approach I propose is the fact that transformation tasks now have an order to them. For your current code that's not the case. The code I propose is basically working already, the problem that remains in applying it to your current code is that it changes the output. The changes necessary to keep your current behaviour mean that you will have to adapt the code I propose to basically look more like yours. These changes mean that you will have to pass values to Tasks by Reference and only collect the results with a second pass over the Tasks. \$\endgroup\$ – Vogel612 Dec 17 '16 at 21:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.