5
\$\begingroup\$

In my data I have few hierarchy values - example address

Name -> Street -> City -> County -> State -> Country

The validation I need is: if a value in the above hierarchy is present, then all the previous/higher order values should be present.

Below is a valid case

Name -> Street -> City -> State
This is valid even though there is no country - as it is last in the order

and below is an invalid case

Name -> Street -> State -> Country
This is invalid as there is no City Which is of higher order than state and country.

The below code helped me in doing the checks. Can this be simplified further? I want to get rid of the last dummy return statement. (To the best of my knowledge, it is difficult to get rid of it. But there are more people far more knowledgeable than me.)

 package com.example.demo;

public class SimplyfyHierarchyCheck {

    public static void main(String[] args) {

        String a="a", b="b", c="c", d="d"; 

        SimplyfyHierarchyCheck shc = new SimplyfyHierarchyCheck();
        System.out.println(shc.isGapLessHierarchy("a", b, "", "") );
    }

    //can this method be made even more concise.
    private boolean isGapLessHierarchy(String... args) { //Hierarchy 
        boolean isGapLess=true;

        for(int i=args.length-1; i>=0; i--) {
            if(args[i].isBlank() )  //nothing to check 
            {
                continue;
            }
            for(int j=i-1; j>=0; j--) {
                if(args[j].isBlank() )  //gap found i.e false 
                {
                    return false;
                }
            }
        }
        return isGapLess; //dummy return // am required to return true or false // can i get rid of this
    }

}
\$\endgroup\$
4
  • 1
    \$\begingroup\$ Why was this tagged C/C++? It's java. \$\endgroup\$
    – Reinderien
    Apr 1, 2021 at 2:29
  • \$\begingroup\$ IMO, you should always return isGapLess. In your loop, you can set isGapLess = false; break; and then let the control to reach the last return statement \$\endgroup\$ Apr 1, 2021 at 4:31
  • \$\begingroup\$ Is County excluded as optional node in your hierarchical validation (first valid example without 'country')? \$\endgroup\$
    – hc_dev
    Apr 1, 2021 at 20:15
  • \$\begingroup\$ What is abstraction and what is true domain-context? What purpose has the gap-identification (i.e. to avoid "gaps in hierarchy"): an address should be resolvable or be specific enough to be locatable❔ \$\endgroup\$
    – hc_dev
    Apr 1, 2021 at 20:20

3 Answers 3

5
\$\begingroup\$

Since you are looking for a more concise way, you can use a for-loop with an empty body:

boolean isGapLessHierarchy(String... args) {
    int i = args.length - 1;
    for (; i >= 0 && args[i].isBlank(); i--);
    for (; i >= 0 && !args[i].isBlank(); i--);
    return i == -1;
}

This also removes the need for the variable isGapless.

With Streams you can use dropWhile and noneMatch:

boolean isGapLessHierarchy(String... args) {
    List<String> argsList = Arrays.asList(args);
    Collections.reverse(argsList);
    return argsList.stream()
            .dropWhile(String::isBlank)
            .noneMatch(String::isBlank);
}

Note that this approach creates an additional list.

General recommendation: I believe you have your reasons to try to make the code shorter. As also others pointed out, making the code shorter often affects readability and that is hardly worth it. I have to admit that the solutions I provided are not the most readable, so think twice before using them in a shared project. On the other hand, if you are the only maintainer and doing some experiments it's totally fine. Good luck with your project.

\$\endgroup\$
5
  • \$\begingroup\$ tested and works. +1. from java doc's i could not figure out how dropWhile is doing it for unordered list - returns, if this stream is unordered, a stream consisting of the remaining elements of this stream after dropping a subset of elements that match the given predicate. So if I have after reversing {a, b, " ", d, " ", f }, for this unordered list i am hoping the output will be {a, b, d, f}. But seems i am wrong. Any hints. \$\endgroup\$
    – joven
    Apr 1, 2021 at 8:46
  • \$\begingroup\$ plz don't ask for a SOF post - that will loose the context and can get lengthy for me. \$\endgroup\$
    – joven
    Apr 1, 2021 at 8:58
  • \$\begingroup\$ got it - Certain stream sources (such as List or arrays) are intrinsically ordered, whereas others (such as HashSet) are not. \$\endgroup\$
    – joven
    Apr 1, 2021 at 9:08
  • 2
    \$\begingroup\$ I've had to look at some of my code after 5 or even 10 years... Even if you're the sole maintainer clarity matters! ;-) \$\endgroup\$ Apr 1, 2021 at 15:47
  • \$\begingroup\$ When reviewing this answer could try to improve names: (a) isGapLessHierarchy(String... args) to hasConsequentHierarchy(Address address) to communicate domain-knowledge and specific purpose (instead of over-generic vagueness)🤔 \$\endgroup\$
    – hc_dev
    Apr 1, 2021 at 20:12
8
\$\begingroup\$

As a long-term Java programmer, I know that most of my time will be spent in maintenance.

On that basis I prefer simple code that expresses its purpose clearly to clever, concise, unclear code.

With that context in place here's some basic comments.

 String a="a", b="b", c="c", d="d"; 
  • It's not the Java way to do multiple declarations together - I'd split these.

  • They are constants not variables, so they should be "static final" (and UPPERCASE).

  • Three of them are not used, so delete them.

    System.out.println(shc.isGapLessHierarchy("a", b, "", "") );

  • Why do you mix use of a constant and a literal? As a maintenance programmer looking at your code I'm now wondering whether this has some significance.

Now to your "isGapLessHierarchy()" method:

  • The method has no dependency on an object, so it should be "static"
  • Never call anything "args" (other than perhaps the arguments to a main method). Give your method parameters meaningful names. How about "hierarchyElements" in this case?
  • (Almost) Never use 'i' and 'j' as indexes. Use something meaningful.
  • Going forwards through a list is (IMHO) clearer than going backwards. Your condition can be expressed as "a gap should never be followed by a non-gap", which allows us to go forwards.

Here's my code using your pattern, with some cleanup and adding a more extensive set of tests (I couldn't be bothered with Junit in this case, but you probably should).

  public class HierarchyCheck {

  public static void main(String[] args) {
    String[][] testCases = new String[][]{
      new String[]{"Name", "Street", "City", "County", "State", "Country"},
      new String[]{"Name", "Street", "City", "County", "State", ""},
      new String[]{"Name", "Street", "City", "County", "", "Country"},
      new String[]{"Name", "Street", "", "County", "State", "Country"},
      new String[]{"Name", "", "City", "", "State", "Country"},
      new String[]{"Name", "", "", "", "", ""}
    };

    for (String[] testCase : testCases) {
      System.out.format("%s %s gapLess%n", 
                        Arrays.toString(testCase),
                        isGapLessHierarchy(testCase) ? "is" : "is not");
    }
  }

  private static boolean isGapLessHierarchy(String... hierarchyElements) {

    // Find the first gap
    int firstGap = 0;
    for (firstGap = 0; firstGap < hierarchyElements.length; firstGap++) {
      if (isBlank(hierarchyElements[firstGap])) {
        break;
      }
    }

    // Is there a non-gap after the first gap?
    for (int nonGap = firstGap + 1; nonGap < hierarchyElements.length; nonGap++) {
      if (!isBlank(hierarchyElements[nonGap])) {
        // There is, so it's not gapless
        return false;
      }
    }

    // Definitely gapless
    return true;
  }

  // We don't have Java 11 for the String.isBlank() method, so fake it!
  private static boolean isBlank(String string) {
    return (string == null) || (string.trim().length() == 0);
  }
}

However, I'd probably have gone for a simpler, arguably cruder approach.

  private static boolean isGapLessHierarchy(String... hierarchyElements) {

    boolean seenGap = false;

    for (String hierarchyElement : hierarchyElements) {
      if (isBlank(hierarchyElement)) {
        seenGap = true;
      }
      else {
        if (seenGap) { // gap followed by non-gap
          return false;
        }
      }
    }

    // Definitely gapless
    return true;
  }
\$\endgroup\$
3
  • \$\begingroup\$ good advice. +1. Thanks \$\endgroup\$
    – joven
    Apr 1, 2021 at 9:04
  • 2
    \$\begingroup\$ I would even try to avoid negation whenever possible: it's not gapless means "has gaps" and gap-less is probably continuous. Athough I am neither domain-expert nor native-speaker 😉 \$\endgroup\$
    – hc_dev
    Apr 1, 2021 at 20:05
  • 1
    \$\begingroup\$ Fair points @hc_dev but there were limits to how much I could fit in, given that I basically did my review and rewrites in a coffee break. \$\endgroup\$ Apr 1, 2021 at 20:38
2
\$\begingroup\$

What I'd recommend:
If Gapless, only a single complete iteration of the sequence is required. Otherwise, checks till the first gap found.

    private boolean isGapLessHierarchy(String... args) { //Hierarchy 
        int i=args.length-1;
        while(args[i].isBlank() && i>=0)  //till data is found
            i--;
        for(--i; i>=0; i--)
            if(args[i].isBlank())  //gap found i.e false 
                return false;
        return true; //is necessary
    }

What you did:
Nested the j-controlled loop in the i-controlled loop. For every Gapless sequence you pass on, parts of the sequence are checked multiple times.
The last return statement is necessary to get true as return from the function.

\$\endgroup\$
6
  • \$\begingroup\$ Why i>=0; above the while loop? \$\endgroup\$
    – joven
    Apr 1, 2021 at 5:34
  • \$\begingroup\$ A mistake while pasting. Sorry. I don't have Java 11 where String.isBlank() was introduced docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/… and hence could not run the program before presenting. \$\endgroup\$
    – Kitswas
    Apr 1, 2021 at 5:42
  • \$\begingroup\$ i got the core logic. Thanks for it. That's wonderful. Tiny things matter for programmer. +1. \$\endgroup\$
    – joven
    Apr 1, 2021 at 6:43
  • \$\begingroup\$ Happy to help. You can accept my answer if it solved your problem. Marc's code is more concise, though. \$\endgroup\$
    – Kitswas
    Apr 1, 2021 at 6:50
  • 2
    \$\begingroup\$ Small thing: return isGapLess can be reduced to return true. isGapLess is never modified, which makes it always equal to true and is thus not needed. \$\endgroup\$
    – JensV
    Apr 1, 2021 at 12:22

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.