2

We have a service in Java that deals with a lot of user-uploaded files, and tasks that prepare these files for different models to execute with.

One such phase of preparation included overriding values in json files. 4 out of 10 tasks were supposed to do this.

I created a static utility class that contained a function to override a value in a json file, something like

public static class ParameterUtils {
   public static String overrideParameter(String originalContent, String key, String newValue) {
     //4 lines of code that overrides this value and return modified content.
   }
}

However, my senior developers said that this utility class hardly offers any value, while I was trying to preserve the DRY principle. At the end I did get rid of this utility class, and now the 4 lines of code to override a value in a json file reside in 4 different files.

Do you think that this class and method are not good from OOP's view? Why and why not?

user2684198
  • 812
  • 1
  • 10
  • 18
  • This is an opinion based question and will probably get close but I partially agree with you senior dev. I don't think that it doesn't have any value but too much repeated code decreases readability of code and thus should be avoided especially when there are good alternative available. – Yoshikage Kira Sep 25 '19 at 03:42
  • This is subjective. But I would go with a static utility. Maybe, name it very specific to your application. We have such things in our code base. – Thiyagu Sep 25 '19 at 03:44
  • 1
    Might want to go with a more complete example and post to https://codereview.stackexchange.com/. There's really no rule that doesn't have exceptions and so the devil is in the details. – Mark Peters Sep 25 '19 at 03:44
  • Hardly offers any value - up until it needs to be changed. No I don’t like repeating code, but there might be better approaches then a utility function. Maybe creating a chain of modifiers instead – MadProgrammer Sep 25 '19 at 03:45
  • It certainly has value, to stuff a bunch of primitive `String` manipulation methods into a class (or use some base class)... compared to four instances of the same code, distributed all over the place. Ask them for their position and their salary, because they might not be worth it. – Martin Zeitler Sep 25 '19 at 03:45
  • The big code smell to me is working with JSON and manipulating it as a string. Unless you have a good reason, this is not something to do normally. – vikarjramun Sep 25 '19 at 03:47
  • If this piece of code was being used in multiple places, I would prefer using this utility. Reasons: 1. Readability: You don't want to keep adding this in multiple places somewhere between your application logic, which kills the flow of code altogether, util named properly (as in your case) would make it very clear for the reader. 2. Maintenance: you know which is the one place you have to make edits in code in case of a change in structure or logic to override parameter (which is the purpose of DRY). – skott Sep 25 '19 at 03:50

1 Answers1

1

No, it is definitely bad practice to duplicate (slightly-complex) code in multiple places.

The DRY principle says "Don't Repeat Yourself". This is something to keep in mind, you don't want to repeat yourself by instructing the computer to override this parameter multiple times.

However, there may be better places to put this than in a static utility class which has only a single method.

If the 4 files you use it in all extend a certain class, maybe make it a protected static method there. Or perhaps place it in the class which returns JSON strings.

However, passing around JSON as a string seems like a code smell, especially if you are manipulating it through string manipulations. It might be a good idea to use a JSON library such as Jackson or org.json. If you use Jackson's databind feature (the most popular way to use Jackson), you create a POJO with actual getters and setters, and Jackson takes care of converting it to JSON. In the case of org.json, you work with a JSONObject class, which is very similar to a Map<String, Object>, and you mutate it like you would a map, and can convert it to and from a String with a single method.

vikarjramun
  • 1,042
  • 12
  • 30
  • Yes, we are converting json file to a Map using a JSON library, changing our values, and converting it back to string to upload to S3. Thanks for your answer, it helps. – user2684198 Sep 25 '19 at 06:30