4
\$\begingroup\$

I was faced with a coding challenge and I'm looking for a more elegant way to achieve the goal.

The challenge

Take the word baseball from the first item in the string array and split it into two comma separated words using only the terms provided in the second item of the string array. If it's impossible to split the word into two separate comma separated words, return the string "not possible".

Example

Given the array new String[] {"baseball","a,all,b,ball,bas,base,cat,code,d,e,quit,z"};

The program should output base,ball

If your program is unable to split baseball using the words from the second item in the string array return "not possible"

I provided the following solution, does anybody have something a little bit more elegant?

public static String ArrayChallenge(String[] strArr) {

    //my code starts here

    String pattern = strArr[0];
    String[] dictionaryWords = strArr[1].split(",");

    Set<String> dictionaryWordSet = new HashSet<>(Arrays.asList(dictionaryWords));

    String[] tempArr = new String[2];

    for (int i = 0; i < pattern.length(); i++) {
        String firstWord = pattern.substring(0, i);

        if(dictionaryWordSet.contains(firstWord)) {
            if(tempArr[0] == null) {
                tempArr[0] = firstWord;
            } else if(tempArr[0].length() < firstWord.length()) {
                tempArr[0] = firstWord;
            }
        }

        String lastWord = pattern.substring(i);

        if(dictionaryWordSet.contains(lastWord)) {
            tempArr[1] = lastWord;
            break;
        }
    }

    if(!pattern.equals(tempArr[0] + tempArr[1])) {
        return "not possible";
    }

    strArr[0] = tempArr[0] + "," + tempArr[1];

    //My code ends here

    return strArr[0];
}

public static void main(String[] args) {
    String[] arr = new String[] {"baseball", "a,all,b,ball,bas,base,cat,code,d,e,quit,z"};

    System.out.println(ArrayChallenge(arr));
}
\$\endgroup\$
8
  • 1
    \$\begingroup\$ The best way to ask a question like this is to put the actual code challenge in a quote block including all restrictions. You can search the questions on this site for programming-challenge for examples. \$\endgroup\$
    – pacmaninbw
    Jun 21 at 13:48
  • 1
    \$\begingroup\$ And put the examples from the question, I can't understand yours. Easiest method is to define all input parameters and all output. \$\endgroup\$
    – lukstru
    Jun 21 at 14:06
  • \$\begingroup\$ @lukstru I tried improving the question, please let me know if that helps. \$\endgroup\$
    – codejunkie
    Jun 21 at 15:04
  • 2
    \$\begingroup\$ Just for clarification: ("aa", "b") would be a "not possible" case, right? What about ("aa", "a")? Can the patterns be repeated? - output would be "a,a" if yes. \$\endgroup\$
    – lukstru
    Jun 21 at 16:20
  • 3
    \$\begingroup\$ This coding challenge isn't worth much. It provides the input as an array of strings of which only the first 2 elements are relevant. There is no point in using an array in such a situation. But even worse. The provided code stores the answer in the input array, in addition to returning it. These are two anti-patterns in a single line of code. If you want to learn how to write good code, choose a different source of challenges. \$\endgroup\$ Jun 21 at 22:46

3 Answers 3

4
\$\begingroup\$

The goal when writing code should almost always be to optimize for readability. Only worry about performance when you have a known performance bottleneck. Readable code can always be rewritten to be performant later. The reverse is much harder.

In Idiomatic java, methods begin with a lowercase letter. I understand this is not your method name, but I’m immediately dubious about whoever has created this challenge.

In idiomatic java, there is whitespace between if and (, to visually distinguish control flow keywords from method names.

pattern is a misleading name, as this is not a pattern. It’s the word to be split.

Streaming might make it cleaner to pull out the words into a set.

dictionaryWordSet can be shortened to dictionaryWords. It is not generally necessary to embed the type of a variable in its name.

tempArr is not a helpful name. Try to avoid abbreviations, as they make the code harder to read, and try to find a descriptive name.

Your algorithm appears to be trying to maximize the length of the first word. This is not in the problem description. The more elegant way to handle this is to reverse the loop so you’re starting from the back, not the front. This will allow you to get rid of tempArr altogether, which is good because it’s confusing. Return early if you find a valid partition.

If you make these changes, your code might look something like:

public static String arrayChallenge(String[] strArr) {

    String wordToSplit = strArr[0];
    Set<String> dictionaryWords = Arrays.stream(strArr[1].split(",")).collect(toSet());

    for (int i = wordToSplit.length() - 1; i > 0; i--) {
        String firstWord = wordToSplit.substring(0, i);
        String lastWord = wordToSplit.substring(i);

        if (dictionaryWords.contains(firstWord) && dictionaryWords.contains(lastWord)) {
            return firstWord + "," + lastWord;
        }
    }
    return "not possible";
}
\$\endgroup\$
5
  • \$\begingroup\$ All very good feedback, I would have never thought to reverse the loop and avoid the extra array. I will work on writing more readable code code with better variable names. \$\endgroup\$
    – codejunkie
    Jun 22 at 12:56
  • \$\begingroup\$ As I look closer at your solution I'm still seeing the ability to continue iterating forward with the same result due to your use of the control flow. Is there any benefit to iterating in reverse? \$\endgroup\$
    – codejunkie
    Jun 22 at 13:18
  • \$\begingroup\$ Your original code went out of its way to find the largest possible first word. Reversing the loop finds the largest first word. If you want the smallest first word, then yes, loop up from 0. \$\endgroup\$
    – Eric Stein
    Jun 22 at 13:37
  • \$\begingroup\$ I agree with you, however with your approach you are moving the split down the string and testing both sides of the split for a set match. I'm just pointing out that it doesn't matter which direction you iterate the loop given both sides of the split are required to be present within the set in order to satisfy the condition. The size appears to be completely irrelevant making your approach much better. \$\endgroup\$
    – codejunkie
    Jun 23 at 14:23
  • \$\begingroup\$ ["baseball", "base,baseb,all,ball"]returns different results if you loop up vs. down. \$\endgroup\$
    – Eric Stein
    Jun 24 at 4:05
5
\$\begingroup\$

I provided the following solution, does anybody have something a little bit more elegant?

Depends on how you define elegant. Shorter? More readable? Extendable to not only include string matching but also regex matching?

I'd argue that shorter is only guaranteed more elegant if it removes repetition. There can be shorter versions but at least some of them are less readable and therefore less elegant in my opinion.

Which transitions to my next topic, readability. I think improving readability always improves elegance.

Your code has some variables that have a pretty wide scope (in the context of your program): take the variable tmpArr, defined on the 4th line of the logic function. It is used through the entire function, and I have no clue what it is there for from the name.

Here are my main rules for naming variables:

  • I usually always write out abbreviations unless they are well known in my context. Sorry C devs.
  • I never name my variables tmp if they live longer than two lines of code.
  • I name my variables after their purpose.
  • Variables are objects, their names are nouns.
\$\endgroup\$
1
  • \$\begingroup\$ I guess I should clarify, I was more so looking for the best performance. My colleague's method was brute force by iterating the dictionary terms until he had a match with a sub query. In my solution, I might be creating a bunch of garbage with the String creation. \$\endgroup\$
    – codejunkie
    Jun 21 at 18:18
3
\$\begingroup\$

Testing for tempArr[0].length() < firstWord.length() looks unwarranted. Nothing in the problem statement requires the first word to be as long as possible. Besides, it introduces a bug: running "baseball" against the dictionary "base", "ball", "baseb" results in "not possible". Do you see why?

Expanding on the above observation, you better гet rid of tempArray at all, and return as soon as the decomposition is found:

    if (dictionaryWordSet.contains(firstWord) && dictionaryWordSet.contains*secondWord))
\$\endgroup\$
1
  • \$\begingroup\$ All very good feedback and until Eric mentioned the reverse, I would have never thought to iterate it in that way. I learned a lot from this challenge and will be sure to apply these lessons to future projects. \$\endgroup\$
    – codejunkie
    Jun 22 at 12:58

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.