Matrix transposition of a string

Question

Per a Reddit Daily Programming Challenge, I am performing a matrix transposition on a string using JavaScript.

Example input: 'The quick brown fox jumped over the lazy dog'

Output:

Tqbfjotld
hurouvhao
eioxmeezg
 cw pr y
 kn e
 d

While my code works, I wanted to know:

1. From a readability standpoint, would it be better to break the code up into smaller functions?
2. If there's anything in the code that can be simplified.

transposeString('The quick brown fox jumped over the lazy dog');
function transposeString(str) {
 if (str.indexOf(' ') === -1) {
 var map = Array.prototype.map;
 var output = map.call(str, function (char) {
 return char + '\n';
 });
 return output.join('');
 } else {
 var master = [],
 longestWord = '';
 words = str.split(' ');
 words.forEach(function(word) {
 if (word.length >= longestWord.length) longestWord = word;
 });
 longestWord.split('').forEach(function() {
 master.push([]);
 });
 var treatedWords = words.map(function (word) {
 if (word.length < longestWord.length) {
 var spaces = Array(longestWord.length - word.length + 1).join(' ');
 return word = word + spaces;
 } else {
 return word;
 }
 });
 treatedWords.forEach(function (word) {
 var n = 0;
 word.split('').forEach(function (char) {
 master[n].push(char);
 n++;
 });
 });
 master.forEach(function (arr) {
 console.log(arr.join(''));
 });
 return;
 }
}

Answer 1

Since you asked if the code could be simplified or made more readable, let's explore other programming styles.

(Note: Remarks in my first review about handling special cases and the returning a value still apply.)

---

First, I would like to point out that a "bland" solution, building the output one character at a time, would actually be slightly shorter than your original:

function transposeString(str) {
 if (str == null) return str;
 var words = str.split(' ');
 var out = [];
 for (var c = 0; ; c++) {
 var line = [];
 var done = true;
 for (var w = 0; w < words.length; w++) {
 if (c < words[w].length) {
 done = false;
 line.push(words[w].charAt(c));
 } else {
 line.push(' ');
 }
 }
 if (done) {
 break;
 } else {
 out.push(line.join(''));
 }
 }
 return out.join("\n");
}

---

Your solution feels a little like functional programming, though not quite. What if you go all the way with functional programming? Your solution was already quite close; you just need to replace .forEach() with .map(). Here's how it could look:

function transposeString(str) {
 if (str == null) return str;
 // Wrapper for String.length
 function length(word) { return word.length; };
 // range(3) -> [0, 1, 2]
 // http://stackoverflow.com/a/11128802/1157100
 function range(n) {
 return Array(n).join().split(',').map(function(e, i) { return i; });
 };
 var words = str.split(' ');
 var maxWordLength = Math.max.apply(null, words.map(length));
 // These functions could be anonymous; I've named them for clarity
 return range(maxWordLength).map(function outputLineUsingNthChar(n) {
 return words.map(function nthCharWithSpacePadding(word) {
 return return n < word.length ? word.charAt(n) : ' ';
 }).join('');
 }).join("\n");
}

That is already shorter, even though JavaScript makes max(words.length) and range() awkward. If you use a library like Underscore.js to provide support for max() and range(), it could be very compact. I'd take advantage of that compactness as an opportunity to comment generously.

function transposeString(str) {
 if (str == null) return str;
 var words = str.split(' ');
 var maxWordLength = _.max(_.pluck(words, 'length'));
 return _.range(maxWordLength).map(
 // Generate a line of output using the nth character of each word
 function outputLineUsingNthChar(n) {
 return words.map(
 // Extract the nth character of a word, space-padded if needed
 function nthCharWithSpacePadding(word) {
 return n < word.length ? word.charAt(n) : ' ';
 }
 ).join('');
 }
 ).join("\n");
}

Answer 2

Here you go, with comments.

Obviously, in production I would have only a handful of comments and not the exhaustive ones I put here. However this is the sort of points I might make during a code review.

When I write code I optimize for it for communicating to a human reader what I intend the code to do. In other words - I want it to read like an essay. Therefore, I extract anything detail-heavy into small helper methods at the bottom of the script. The idea is that someone should open my script and the first thing they see should give them a top-level overview of the general flow of logic.

I didn't comment at all on your solution itself, instead just cleaning up the algorithms that were already there.

transposeString('The quick brown fox jumped over the lazy dog');
//Always comment public apis with usage and your intent when writing it.
//Transpose a string of words into a vertical matrix
//from http://www.reddit.com/r/dailyprogrammer/comments/1m1jam/081313_challenge_137_easy_string_transposition/
function transposeString(str) {
 // the ~ (ones complement) operator makes indexOf work like 'contains'
 // ~(-1) == 0, ~(x != -1) != 0
 // not sure why you need this condition at all however in terms of the problem - seems like everything
 //would just work
 if (!~str.indexOf(' ')) 
 //definitely don't have two large branches of conditional logic in the same function, put them 
 //in separate well-named functions
 //Also always prefer to return early to get the simple paths out of the way in conditional logic. 
 //The whole "single point of exit" advice is from back in the day of monolithic functions and gotos all over the place
 return makeVerticalString(str);
 //My preference is to use a single var block with everything aligned and commas preceeding
 //the terms. I have good reasons for this but not anything that I care to get into without
 //being super-pedantic. It's ok to have a different style here.
 var words = str.split(' ') //you were misssing var here
 ,longestWord = getLongestWord(words) //give algorithms their own function
 //word matrix - I recommend commenting this so that you know it's a matrix and not a flat array
 //also note that it is much cleaner to use a map than a forEach and explicit push
 //When you see array.push() anywhere it can almost always be cleaned up with a map
 ,transposedMatrix = longestWord.split('').map(function() { return [] }) 
 //give a name to the thing that you're doing here and extract the algorithm to its own function
 //also rename 'treated' to 'padded' to be more explicit
 ,paddedWords = words.map(padWordsShorterThan(longestWord.length));
 ;
 //again, move the actual algorithm to its own labeled function
 paddedWords.forEach(addEachLetterToARow(transposedMatrix));
 transposedMatrix.forEach(function (arr) {
 console.log(arr.join(''));
 });
 //no need for return if you're not returning anything
 //A line to deliniate private helper functions
 //////////////////////////////////////////////////////////////////////////////////////////////
 function makeVerticalString(str) {
 //You're reducing an array to a single value so really reduce is what you want here
 Array.prototype.reduce.call(str, function(m, c) { return m+c+'\n'}, '')
 }
 function getLongestWord(words) {
 //again going from an array to a single value - we can use reduce here
 return words.reduce(function(longest, thisWord){ return thisWord.length >= longest.length ? thisWord : longest}, '');
 }
 //a function that returns a function can be a good tool for controlling scope while keeping things readable
 //note that the inner function still has a name so that it shows up properly in callstack traces (rather than "anonymous")
 function padWordsShorterThan(threshold) { return function padWordsShorterThan(word) { 
 if (word.length >= threshold)
 return word; 
 //no need for else clauses when you return early
 return word + repeatStr(" ", longestWord.length - word.length + 1);
 }}
 function repeatStr(str, times) {
 //you have a neat little way of implementing a character repeat but its difficult to read - drop it in its own method
 return Array(times).join(str)
 }
 //Naming anonymous functions not only helps make clear what they're doing
 //but makes them show up named in a debugging callstack rather than "anonymous"
 //Rule of thumb - name anonymous functions that are doing significant logic
 function addEachLetterToARow(matrix) { return function addEachLetterToARow(word) {
 var n = 0;
 word.split('').forEach(function addToRow(char) {
 matrix[n].push(char); //note that you have a possible error condition here where matrix[n] does not exist. Do you want to check for that explicitly?
 n+=1; //I agree with Doug Crockford that ++ is wierd and error-prone. += reads much better in my opinion
 });
 } }
}

Answer 3

I would consider it bad form for your function to console.log() its output instead of returning a value, because it severely harms code reusability.

I don't think that the one-word case deserves special treatment, since it's just a trivial optimization of the general case. Even the empty string doesn't need special handling. A null might deserve a special case, if you decide to handle it.

You omitted the var keyword when defining words.

Instead of appending empty arrays to master using .forEach(), you could define it all at once using .map():

var master = longestWord.split('').map(function() { return []; });

The name treatedWords is insufficiently self-documenting. I suggest paddedWords.

function transposeString(str) {
 if (str == null) return null;
 var words = str.split(' ');
 var longestWord = '';
 words.forEach(function(word) {
 if (word.length >= longestWord.length) longestWord = word;
 });
 var master = longestWord.split('').map(function() { return []; });
 var paddedWords = words.map(function (word) {
 if (word.length < longestWord.length) {
 var spaces = Array(longestWord.length - word.length + 1).join(' ');
 return word = word + spaces;
 } else {
 return word;
 }
 });
 paddedWords.forEach(function (word) {
 var n = 0;
 word.split('').forEach(function (char) {
 master[n].push(char);
 n++;
 });
 });
 return master.map(function (line) {
 return line.join('');
 }).join("\n");
}