3
\$\begingroup\$

I'm just starting to learn JavaScript. I implemented the Diamond kata, and I would appreciate if anybody could give me some feedback. I'm particularly concerned about learning the most 'idiomatic' way to do things in JavaScript, style conventions, etc.

The diamond kata consists of the following problem statement:

Given a letter, print a diamond starting with ‘A’ with the supplied letter 
at the widest point.

For example: print-diamond ‘C’ prints

  A
 B B
C   C
 B B
  A

The production code (test code in the Gist):

// Diamond Kata with a TDD approach. First attempt
'use strict';

module.exports.Diamond = Diamond;


function Letter(char, spaces) {
  this._char = char;
  this._spaces = spaces;
}
Letter.prototype.print = function print() {
  return this._char + (
    (this._spaces > 0)
      ? (Array(this._spaces + 1).join(' ') + this._char + '\n')
      : '\n'
  );
}


// Singleton list with all letters
var LETTERS = new function () {
  // Init all upper-case letters
  for (let char=65; char <= 90; char++) {
    let character = String.fromCharCode(char);
    this[character] = new Letter(character, (char - 65) + (char - 65 - 1));
  }
}();
LETTERS.__proto__.iterateLetters = function (character, callback) {
  var chars = Object.keys(this);
  var last = chars.indexOf(character);
  var pos = 0;
  var direction = 1;
  // Forwards
  while (pos >= 0) {
    callback(this[chars[pos]], pos, last);
    if (pos === last) {
      // Reached final character, return to 'A'
      direction = -1;
    }
    pos += 1 * direction;
  }
}


function Diamond (letter) {
  if (typeof(letter) !== 'string' && !(letter instanceof String)) {
    throw new Error('Letter must be an string');
  }
  this._letter = letter;
};
Diamond.prototype.process = function process() {
  var prefix = function (max, current) {
    return Array(1 + max - current).join(' ');
  }
  var result = '';
  LETTERS.iterateLetters(this._letter, function (letter, current, last) {
    result += prefix(last, current);
    result += letter.print();
  });
  return result;
}

This code is meant to execute at a "Node --harmony" environment, although except from the module exportation, this is just plain JavaScript.

A working demo is available here.

This is written using a TDD approach. The test code, as well as this same snippet, can be found in the Gist.

\$\endgroup\$
  • \$\begingroup\$ Where are you running the code? Can you post an example (e.g. on jsfiddle) showing it working? \$\endgroup\$ – RobH Feb 2 '15 at 11:05
  • \$\begingroup\$ @RobH This code is meant to run in a Node environment, but it barely uses any Node feature. I updated the answer with a link to a working demo. Thanks! \$\endgroup\$ – ArnauOrriols Feb 2 '15 at 12:00
  • \$\begingroup\$ If I save your code to a file and run it with node I get an error. char is a reserved word (depending on version) \$\endgroup\$ – RobH Feb 2 '15 at 13:31
2
\$\begingroup\$

I think you are overthinking it. Or maybe this is just not the right problem for TDD? This seems to be a typical interview / Project Euler (kind of) question. Something that doesn't require more testing than running it several times in console.

Why do I say this?--Tests are typically needed when the borderline conditions for the input are not trivially found and the algorithm being tested may take on multiple ways, not easily seen from its definition. In this case we have a very clearly stated borderline condition for inputs (our inputs are characters A-Z) and the algorithm may only take on two paths (a base-case, when 'A' is given) and the "other" cases (given any letter other than 'A'). The algorithm isn't defined for any other input.

Below is a simple version of this problem:

function diamondKata(character) {
    var a = 'A'.charCodeAt(), frame, line,
        width = character.toUpperCase().charCodeAt() - a,
        i = 0, lines = [];

    function blanks(n) { return new Array(n).join(' '); }

    function makeLine() {
        return blanks(width + 1 - i) + frame +
            blanks(i * 2) + frame;
    }
    while (i++ < width) {
        frame = String.fromCharCode(a + i);
        lines.push(makeLine());
    }
    if (lines.length) {
        lines.unshift(blanks(width + 1) + 'A');
        lines = lines.concat(
            lines.slice(0, lines.length - 1).reverse());
    } else {
        lines.push('A');
    }
    return lines.join('\n');
}

Some points about your code: while operations with strings are generally very fast in JavaScript (highly optimized C code), it is still better to create long strings by first pushing all chunks into an array, and then to join it all in one go. The reason for this is that creating strings by consequent concatenation will make a lot of intermediate memory allocations, which would be avoided if the string is created at once.

More on your code: type checking is frowned upon, unless its some library code, where the algorithm may take on multiple paths depending on what kind of object it works with. When you check types only to throw a type error, you are working against the benefits you get from dynamically type-checked language: with dynamically type-checked languages you get these type errors for free (the environment already does that for you).

Compare this to C-like languages, where after dereferncing a pointer and making wrong assumption about the type of the object pointed by the derefernced pointer will spell disaster for the program, unless checked.

Hardcoding the codepoint assigned to characters would be a bad thing to do in any language, unless it's an optimization justified by a profiling session.

\$\endgroup\$
  • \$\begingroup\$ Many thanks @wvxvw, that's exactly the kind of feedback I was looking for! On the point of having over-engineered it, you are more than right. This is by all means not what I would have implemented if this was production code. Yet with katas, the focus is not on solving the (rather silly) problem, but on what you practice/learn while solving it. And I was focused on comprehending the object-oriented Javascript patterns, on getting to know the available unit-testing frameworks for Node applications, and on getting used to Javascript general idioms. That's why I willingly overthought it. \$\endgroup\$ – ArnauOrriols Feb 3 '15 at 8:16
1
\$\begingroup\$

Following the suggestions made by @wvxvw, This is a new implementation, aiming for a more cohesive and simple code:

function printDiamond(letter, rows) {
  if (rows === undefined){
    rows = new Array();
  }

  var padding = Array(rows.length + 1).join(' ');
  if (letter === 'A') {
    rows.unshift(padding + 'A');
  } else {
    var charCode = letter.charCodeAt(0);
    var innerSpaces = Array((charCode - 'A'.charCodeAt(0)) * 2).join(' ');
    rows.unshift(padding + letter + innerSpaces + letter);
    printDiamond(String.fromCharCode(charCode - 1), rows);
  }

  return rows.concat(rows.slice(0, -1).reverse()).join('\n') + '\n';
}
\$\endgroup\$

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.