Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Please let me know a better approach to solve below problem:

Problem: Vertically arrange the words in a string and print on console.

Sample Input:

Hello Jack

Output:

H J
e a
L c
L k
o

My solution:

public static void toVerticalWords(String str){

        //split the words by whitespace
        String[] strArr = str.split("\\s");
        int maxWordLen = 0;

        //get the longest word length
        for(String strTemp : strArr) {
            if(strTemp.length() > maxWordLen)
                maxWordLen = strTemp.length();
        }

        //make a matrix of the words with each character in an array block
        char[][] charArr = new char[strArr.length][maxWordLen];
        for(int i=0; i<strArr.length; i++) {
            int j=0;
            for(char ch : strArr[i].toCharArray()){
                charArr[i][j] = ch;
                j++;
            }
        }

        //print the vertical word pattern, or transpose of above matrix (2D array)
        for(int j=0; j<maxWordLen; j++) {
            for(int i=0; i<strArr.length; i++) {
                if (i!=0)
                    System.out.print(" ");
                System.out.print(charArr[i][j]);
            }

            System.out.println();
        }
    }
share|improve this question
    
Not an answer, but an idea: In the end you are requesting the "zip" function, which exist in several functional programming languages. Java also got functional elements with the Version SE 8. Unfortunately the zip-function is not implemented by the standard libraries. But it is doing exactly, what you implemented - only in a more generic way. Here is an example implementation with no functional syntax: stackoverflow.com/questions/3833814/… –  Wintermute Apr 8 at 10:36
    
@Wintermute Think its an overkill in the context of this question. But a good share. I will see. –  xploreraj Apr 8 at 12:24

5 Answers 5

up vote 10 down vote accepted
  1. Looking at this code:

    if(strTemp.length() > maxWordLen)
        maxWordLen = strTemp.length();
    

    This could be reduced to:

    maxWordLen = Math.max(maxWordLen,strTemp.length());
    
  2. Inline the incrementation:

    charArr[i][j++] = ch;
    

    Or just use System.arraycopy:

    char[][] charArr = new char[strArr.length][maxWordLen];
    for (int i = 0; i < strArr.length; i++) {
        System.arraycopy(strArr[i].toCharArray(), 0, charArr[i], 0, strArr[i].length());
    }
    
  3. Either inline the output:

    System.out.print((i != 0 ? " " : "") + charArr[i][j]);
    

    or at least place braces:

    if (i!=0){
       System.out.print(" ");
    }
    

    4 . If you wonder about other solutions, check this one:

    int i = 0;
    
    boolean hasMoreChars = true;
    
    while (hasMoreChars) {
    
        hasMoreChars = false;
    
        for (String str : strArr) {
    
            if (i < str.length()) {
    
                hasMoreChars = true;
    
                System.out.print(str.charAt(i) + " ");
            }
            else {
                //two spaces, added by xploreraj in Edit
                //in case a shorter word comes before longer word
                //we must compensate the absence of character with space
                System.out.print("  ");
            }
        }
        System.out.println();
        i++;
    }
    
share|improve this answer
    
Thanks, noted. But I am expecting a better logic which is reader friendly. –  xploreraj Apr 8 at 12:18
    
This 4th point code is neat and nice. But it misses out on the whitespace in case a smaller word comes in front of a bigger word. And the bigger word letters will come below previous smaller word. –  xploreraj Apr 8 at 14:44
1  
I would explicitly recommend against in-lining an increment (number 2). Errors involving whether the increment returns the original or the new value are well known. (I personally can never keep it straight.) Also, embedding side effects in a line that is doing something else makes your logic dense enough that interpreting the code becomes more difficult; it is easier to understand that side effects are occurring when explicitly separate calls are made. (I would argue that for readability and simplicity, increment shouldn't even have a return value, personally.) –  jpmc26 Apr 8 at 17:21
    
The point 4 is concise and easily understandable. Accepting as answer (alternate easy logic). –  xploreraj Apr 9 at 7:33

I assume you are only trying to work with ASCII characters, but I would like to point out a few Unicode-related failure modes, which can be exhibited by the following code:

String[] funkyStrings = {
    "foo bar",
    "foo ba\u0308r",
    "\ud835\udcbb\u2134\u2134 \ud835\udcb7\ud835\udcb6\ud835\udcc7"
};
for (String str : funkyStrings) {
    System.out.println(str);
    toVerticalWords(str);
}

The first string produces ordinary output:

foo bar
f b
o a
o r

The second string contains a grapheme cluster that is made up from multiple code points – the a and an accent ̈ which together create the glyph ä (← that's the precomposed form showing how it should look, as not all fonts handle combined characters correctly)

foo bär
f b
o a
o ̈
 r

The third string uses mathematical glyphs which have code points beyond U+FFFF. Because Java is slightly Unicode-retarded and can't deal with such high code points properly, its chars are actually UTF-16 code units (16 bit wide, far to small for what we are dealing with here). Therefore, higher code points have to be specified as a pair of surrogate halves. The encoding for U+1D4BB is d835 dcbb in UTF-16BE. This now produces the following output:

𝒻ℴℴ 𝒷𝒶𝓇
? ?
? ?
ℴ ?
ℴ ?
 ?
 ?

Only the is displayed correctly, because it has a lower code point of U+2134. For the other characters, the surrogate halves are separated from each other, leading to invalid characters.

Not shown here: how fullwidth and halfwidth charaters can mess up your vertical layout.


The moral of the story: code points, UTF-16 code units, characters and glyphs are different things. Dealing with them correctly is excessively difficult, and I don't know about any suitable tools. If you are not prepared to deal correctly with any input you may encounter, then check that the input conforms to a subset you are comfortable with.

for (char c : str) {
    if (c > 0x7F) {
        throw new IllegalArgumentException("The input string may only contain ASCII code points");
    }
}
share|improve this answer
1  
At this moment I am not concerned by non-ASCII things or exceptions, just betterment of a simple logic. Thank you for sharing the risk, it is a new learning although over the head knowledge slightly. Noted. –  xploreraj Apr 8 at 12:16

At the moment, your current code does not handle properly the fact that a short word can be followed by a long word. The input "Hello Jack the Magnificient" can show you what happens.

Two solutions to this problem :

  1. you can add a loop to add the missing whitespaces.

        for ( ; j <maxWordLen ; j++)
        {
            charArr[i][j] = ' ';
        }
    
  2. You change the whole structure of the loop :

        char[] charArray = strArr[i].toCharArray();
        for (int j=0 ; j <maxWordLen ; j++)
        {
            charArr[i][j] = (j < charArray.length) ? charArray[j] : ' ';
        }
    

The second solution is likely to be slower but I find it more elegant.


Tiny detail, you can write :

            if (i!=0)
                System.out.print(" ");
            System.out.print(charArr[i][j]);

in a more concise way :

           System.out.print(charArr[i][j] + " ");

(this will add trailing whitespaces but who cares really?)

share|improve this answer
    
It works for "Hello Jack the Magnificient" because the character array will have length of 'Magnificient' as its the longest word and for other words, the remaining char array block will have default '\0' character which is not printed anyway. Please check. For second part, yes I agree, but just I'm able to leave trailing whitespace, I did that way. –  xploreraj Apr 8 at 12:10
    
I have retried and I confirm it looks wrong : ideone.com/xYstrt –  Josay Apr 8 at 12:27
    
Ideone may be is having problems with the '\0' character, and this can really vary across different consoles, but in Eclipse Juno, and CMD, its proper. –  xploreraj Apr 8 at 12:32
    
It doesn't work on my computer neither so there is an actual issue even if you cannot see it in your setup. The point is that the array should be filled with whitespaces and not '\0'. I've raised the issue, now I'll let you decide if you want to consider this as a problem. –  Josay Apr 8 at 12:36
    
OK. I am into it. –  xploreraj Apr 8 at 14:10

Variable Names

Your variables names are unnecessarily abbreviated and genetic. Also suffixes such as Arr are usually unnecessary. Here some examples I'd use:

  • str => sentence
  • strArr => words
  • strTemp => word

Algorithm

You could drop copying the strings into the character array, by using strArr directly and simply checking if j exceeds the length of the word and printing a space if it does.

Also getting the maximal word length could also be avoided, if you check during output, if all strings are shorter than j.

My version

public static void toVerticalWords(String sentence){
    String[] words = sentence.split("\\s");

    boolean allWordsEnded;
    int row = 0;

    do {
        allWordsEnded = true;

        char[] output = new char[words.length * 2];
        int column = 0;
        for (String word : words) {
            char c;
            if (row < word.length()) {
                c = word.charAt(row);
                allWordsEnded = false;
            } else {
                c = ' ';
            }
            output[column++] = c;
            output[column++] = ' ';
        }
        if (!allWordsEnded) {
            System.out.println(output);
        }
        row++;
    }
    while (!allWordsEnded);
}

The main loop is a bit more complicated, because I cache the line to be written, so when all words have ended, it won't output a line of spaces.

share|improve this answer
    
Agree with your conventions. –  xploreraj Apr 9 at 8:25

You don't really need to compute the maxWordLen or the matrix. Once there is nothing else to print, stop!

Here is my solution:

public static void toVerticalWords(String str) {
    String[] words = str.split("\\s+");
    boolean remained = true;

    for (int j = 0; remained; j++) {
        remained = false;
        for (int i = 0; i < words.length; i++) {
            remained = remained || (words[i].length() > j);
            System.out.print(words[i].length() > j ? words[i].charAt(j) : " ");
        }
        System.out.println();
    }
}
share|improve this answer
    
Yes, you are right. But in the initial code, I held on to the maximum word which I could use for printing blank or whitespace in case of shorter words. However optimal versions of the code have come along since then. –  xploreraj Apr 10 at 11:44

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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