2

I'm still currently learning OOP. I need a method that takes in a List<JsonNode>. Now there are multiple components that need some resources from the List. Component1 needs all of the keys. Component2 only needs the keys that have a numerical value. As we understand, these two both need keys, thus requiring a function to iterate through the given List and parse the keys from the given List.

What we are planning is that all of it happens in a single function/class (or rather as a single responsibility). This responsible function would take in a List instance, iterate through the List and one by one get the keys and create a resource for Component1; while it does so, it also looks for the keys with numerical values and creates a resource for Component2. The name of the responsibility would be Extractor.

Would this be in accordance with OOP principles? Should we just give it a better definition(name)? Or should we define them as two different responsibilities?

Edit:

So JsonNode has keys and they're all different from each other. I may have

[{"name":"john", "age": 10, "status":"single"}]

So I need to get

Component1 -> [name, age, status]
Component2 -> [age]
  • The part I'm getting a little stuck on is why you have to parse keys in the map (not saying you don't have a genuine reason, I'm just having a hard time trying to imagine one). – user204677 Feb 2 '18 at 1:21
  • 1
    I simply need to get all the keys in the map and the map may also have Maps as values meaning there will be nested keys. Let's just say that we need those keys as a resource - like options. I'll be editing above as i forgot to include something. – Rei Brown Feb 2 '18 at 2:00
  • 1
    I was planning to migrate to Map but then realize it may be harder to implement. Sorry but what we're currently using is List<JsonNode>. So I changed the content above. – Rei Brown Feb 2 '18 at 2:05
  • What you name things has little to do with OOP principles, though a vague or overly long name could be a potential sign of OOP trouble. – Robert Harvey Feb 2 '18 at 2:18
  • What I was thinking about the name was being specific with it. I can choose to name the function for Component1 as AllKeysParser and function for Component2 as NumericalValueKeyParser. But if I choose to combine them then it could be just KeyExtractor – Rei Brown Feb 2 '18 at 2:26
2

What we are planning is that all of it happens in a single function/class (or rather as a single responsibility).

I can't say I've ever planned a responsibility. I find that I plan behavior and the responsibilities emerge as that behavior is assigned or implemented.

In your case, the planned behavior is that the two components will receive keys where the values meet certain conditions from a given set of JSON objects. The initial objects we have are the components, the JSON values, and some new behavior that will cover the rest. The simplest implementation (assuming some details of how it would work):

void extractKeysForComponents(List<JsonValue> jsonValues, Component component1, Component component2) {
    jsonValues.stream()
        .map((jsonValue) -> jsonValue.key())
        .forEach((key) -> component1.add(key));

    jsonValues.stream()
        .filter((jsonValue) -> jsonValue.value() instanceof Number)
        .map((jsonValue) -> jsonValue.key())
        .forEach((key) -> component2.add(key));
}

Our simplest implementation implements the behavior we want. Of course we also wrote tests for this (which I will just assume are there). This method is very smelly though. The method takes two Component parameters that do not interact. This is the first indication that we are doing more than one thing. The implementation has two clearly distinct parts. This is the second indication. Those two parts are also near-duplicates. This is our third indication. At this point, we can see that there might be more than one responsibility hiding in there, but what they are has not yet emerged. We know duplication is the worst of the three indicators we have, but we only have near-duplication. Let's make them as similar as possible:

void extractKeysForComponents(List<JsonValue> jsonValues, Component component1, Component component2) {
    jsonValues.stream()
        .filter((jsonValue) -> true)
        .map((jsonValue) -> jsonValue.key())
        .forEach((key) -> component1.add(key));

    jsonValues.stream()
        .filter((jsonValue) -> jsonValue.value() instanceof Number)
        .map((jsonValue) -> jsonValue.key())
        .forEach((key) -> component2.add(key));
}

Now both parts are as similar as we can make them while still preserving behavior. Fixing the near-duplication also points us to a potential responsibility: filtering. Before, we had a hidden default filter of no filter. Now, that's become explicit. Let's pull out what varies so we can begin to encapsulate it:

void extractKeysForComponents(List<JsonValue> jsonValues, Component component1, Component component2) {
    Predicate<JsonValue> component1Filter = (jsonValue) -> true;
    Predicate<JsonValue> component2Filter = (jsonValue) -> jsonValue.value() instanceof Number;

    jsonValues.stream()
        .filter(component1Filter)
        .map((jsonValue) -> jsonValue.key())
        .forEach((key) -> component1.add(key));

    jsonValues.stream()
        .filter(component2Filter)
        .map((jsonValue) -> jsonValue.key())
        .forEach((key) -> component2.add(key));
}

Now we see that the only difference between the duplicated parts is which component and associated filter are used. Maybe we can use a data structure to associate those two components to their filters in a generic way:

void extractKeysForComponents(List<JsonValue> jsonValues, Component component1, Component component2) {
    Map<Component, Predicate<JsonValue> componentsAndPredicates = new LinkedHashMap<>();
    componentsAndPredicates.put(component1, (jsonValue) -> true);
    componentsAndPredicates.put(component2, (jsonValue) -> jsonValue.value() instanceof Number);

    for (Map.Entry<Component, Predicate<JsonValue>> entry : componentsAndPredicates) {
        jsonValues.stream()
            .filter(entry.getValue())
            .map((jsonValue) -> jsonValue.key())
            .forEach((key) -> entry.getKey().add(key));
    }
}

Now separate responsibilities are more clearly starting to emerge. Components are associated with specific filters, so perhaps we should move those filters over to the Component classes? Each component can then provide its filter in a generic way. (This is one way to approach this problem, and there are others. I am simply choosing this one since I don't have enough details about your specific code.) The code would look like:

void extractKeysForComponents(List<JsonValue> jsonValues, Component component1, Component component2) {
    List<Component> components = Arrays.asList(component1, component2);

    for (Component component : components) {
        jsonValues.stream()
            .filter(component.getJsonValueFilter())
            .map((jsonValue) -> jsonValue.key())
            .forEach((key) -> component.add(key));
    }
}

What is now most strange is that we are taking just two Component parameters to convert them to a list and handle them generically. In programming, there are only three numbers: zero, one, and many. We can take an arbitrary number of Components and handle them all equally well.

void extractKeysForComponents(List<JsonValue> jsonValues, Component... components) {
    for (Component component : components) {
        jsonValues.stream()
            .filter(component.getJsonValueFilter())
            .map((jsonValue) -> jsonValue.key())
            .forEach((key) -> component.add(key));
    }
}

So we've gone from an implementation that supported very specific behavior to one that can handle arbitrary behavior in a more generic way, while progressively exposing responsibilities. We didn't plan the responsibilities, though. We just followed where the code and refactorings led us. There's still a lot to do with this code. I still don't think the responsibilities are properly distributed. There are a number of bad names. There were alternate refactorings that could have been better. But we've started on a path to revealing those responsibilities without needing to identify or design them all up front.

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.