Journal Articles

CVu Journal Vol 28, #4 - September 2016 + Student Code Critiques from CVu journal.
Browse in : All > Journals > CVu > 284 (10)
All > Journal Columns > Code Critique (70)
Any of these categories - All of these categories

Note: when you create a new publication type, the articles module will automatically use the templates user-display-[publicationtype].xt and user-summary-[publicationtype].xt. If those templates do not exist when you try to preview or display a new article, you'll get this warning :-) Please place your own templates in themes/yourtheme/modules/articles . The templates will get the extension .xt there.

Title: Code Critique Competition 100

Author: Martin Moene

Date: 03 September 2016 16:37:23 +01:00 or Sat, 03 September 2016 16:37:23 +01:00

Summary: Set and collated by Roger Orr. A book prize is awarded for the best entry.

Body: 

Please note that participation in this competition is open to all members, whether novice or expert. Readers are also encouraged to comment on published entries, and to supply their own possible code samples for the competition (in any common programming language) to scc@accu.org.

Note: If you would rather not have your critique visible online, please inform me. (Email addresses are not publicly visible.)

Last issue’s code

I wanted to do something slightly different for the 100th code critique column, so I based this critique on the ‘left-pad’ function that was part of npm but was withdrawn by the author, breaking a large number of components on the Internet.

See http://blog.npmjs.org/post/141577284765/kik-left-pad-and-npm for some more official information about the issue and this blog:http://www.haneycodes.net/npm-left-pad-have-we-forgotten-how-to-program/ for some discussion about some of the issues it raises.

Please feel free to comment on the Javascript code itself, or on the wider issues raised by the story.

Listing 1 contains leftpad.js, and a trivial test page is provided in Listing 2 if you want to play with the function in a browser.

function leftpad (str, len, ch) {
  str = String(str);
  var i = -1;
  if (!ch && ch !== 0) ch = ' ';
  len = len - str.length;
  while (++i < len) {
    str = ch + str;
  }
  return str;
}
Listing 1 
<!DOCTYPE html>
<html>
<head><title>CC100</title></head>
<body>
<h1>Code Critique 100</h1>
<script src="leftpad.js"></script> 
<script>
function testLeftpad() {
  result.innerHTML = '"' + 
  leftpad(text.value, len.value, pad.value) + 
  '"';
}
</script>
<table>
<tr><td>Text to pad</td><td>
  <input type="text" id="text" value="1234">
</td></tr>
<tr><td>Length</td><td>
  <input type="text" id="len" value="10">
</td></tr>
<tr><td>Pad char</td><td>
  <input type="text" id="pad" value="0">
</td></tr>
</table>
<br/><button onclick="testLeftpad()">
Try out leftpad
</button>
<pre id="result"></pre>
</body>
</html>
Listing 2

Critique

Juan Zaratiegui <yozara@outlook.com>

I am not a Javascript fan or expert, but languages have similar foundations, so let’s give it a try.

For a start, let’s look at the function as a whole: we are trying to pad the left of a string with some char until we fill a specified length.

This length comes from the parameter len, and it is never checked against the natural limits: it should be a positive integer, and greater than the original string length to be meaningful.

If the length desired is less or equal to the original string length, no operation should be performed and the original string would be returned.

Now that we have corrected length treatment, let’s look at the fill char. We want 1 fill char, but there is no precaution taken about it. Instead, we have a strange condition that replaces a string that is simultaneously empty and '0' with a space. That will never happen.

Instead we will make three tests: empty string, string of length not 1 and string = Char(NULL). If any of these tests are true, we will use the default string ' ' to fill.

And finally, as a question of cleanness, I will use local variables with names different from the parameters, thus giving the following listing:

  function leftpad (str, len, ch) {
    result = String(str);
    var i = -1;
    if (!ch || ch.length != 1 ||
      ch.charCodeAt(0) == 0) ch = ' ';
    if (len > str.length ) {
      missing = len - str.length;
        while (++i < missing) {
          result = ch + result;
    }
  }
  return result;
}

Commentary

There are a number of issues that could be covered with this story. The original one was that the code as written appears to have a number of problems (or potential problems) but there are some rather wider questions that I might touch on too.

The requirements on the input types and values for leftpad are unclear. Javascript is a dynamically typed language so the three parameters can be filled by a variety of different types at runtime; which ones are expected?

The first argument is converted to a String so can be (almost) any type – although passing it an arbitrary Object, for instance, is unlikely to produce useful output. The likely data types are String and Number and either of these will ‘do the right thing’ but this has to be inferred.

The second argument is the desired output string length. As Juan noted, it’s not clear what the effect of providing an out of range value ought to be. However, the code as written will make no change to str if len is less than the string length or negative. It’s not clear whether the behaviour if the value is non-integral is expected (eg. len = 10.01 pads to 11).

The third argument, ch, is the fill character. Except when it isn’t. The complication is the line that ‘sanitises’ the fill character:

  if (!ch && ch !== 0) ch = ' ';

The first expression, !ch, will be true if ch is undefined, null, false, +0, -0 or NaN, or an empty string. The second expression, ch !== 0, will be true if ch is either not a number or is a non-zero number. So the overall effect of the line is to set the fill character to a space if the argument was undefined, null, false, NaN or an empty string. Note that ch will be undefined when the argument is omitted, hence ‘by default’ the function pads with spaces.

However, as Juan also noted, there is no check that the string representation of ch (used later when actually padding the string) is of length 1. If the length is greater than 1 the code will prepend n copies of the string to result, leading to a wider string than expected being returned. We could use ch.charAt(0) to create a single-character fill. However, if ch is supplied as a number, not as a string, the call to charAt(0) will fail. It might be better to explicitly convert ch into a string first.

This is obviously not the only design option; another option is to prepend enough copies of the string so the new length is a least that desired: leftpad("+-", "test", 7) would then produce as output "+-+-test".

Finally, the performance of the padding itself is non-optimal: the loop creates a new string each time which is one character longer with the new character at the front. For modern (ECMAScript 6) browsers the repeat function can be used to create a string of the right length and remove the loop completely:

  if (len > 0)
    str = ch.repeat(len) + str;

If the code has to run on slightly older browsers which do not implement repeat() then it might be worth supplying a function with similar semantics.

The current version of npm’s leftPad function builds up the padding using a double-and-add algorithm based on the binary decomposition of the length required.

From https://github.com/stevemao/left-pad:

  var cache = [
    '',
    ' ',
    '  ',
    '   ',
    '    ',
    '     ',
    '      ',
    '       ',
    '        ',
    '         '
  ];

  function leftPad (str, len, ch) {
    // convert `str` to `string`
    str = str + '';
    // `len` is the `pad`'s length now
    len = len - str.length;
    // doesn't need to pad
    if (len <= 0) return str;
    // `ch` defaults to `' '`
    if (!ch && ch !== 0) ch = ' ';
    // convert `ch` to `string`
    ch = ch + '';
    // cache common use cases
    if (ch === ' ' && len < 10)
      return cache[len] + str;
    // `pad` starts with an empty string
    var pad = '';
    // loop
    while (true) {
      // add `ch` to `pad` if `len` is odd
      if (len & 1) pad += ch;
      // devide `len` by 2, ditch the fraction
      len >>= 1;
      // "double" the `ch` so this operation
      // count grows logarithmically on `len`
      // each time `ch` is "doubled", the `len`
      // would need to be "doubled" too
      // similar to finding a value in binary
      // search tree, hence O(log(n))
      if (len) ch += ch;
      // `len` is 0, exit the loop
      else break;
    }
    // pad `str`!
    return pad + str;
  }

This is a degree of cleverness that may only be required for some uses of the left pad function; but given how widespread use of the original function was I suspect it is worth doing. I think though that I’d have possibly made the creation of the fill string into a separate function as this can be useful on its own.

The replacement code comes with a simple test program, which is good, but it doesn’t cover some of the troublesome cases for input arguments of unexpected types or ranges. mentioned earlier.

The bigger questions that this raises are at least partly covered in the two links I gave in the critique preamble.

I think for me a couple of the major concerns are:

Taken together this means a number of people had a hard-to-diagnose failure because of a tangle of dependencies outside their direct control.

This seems to me quite a large ‘business’ risk and I’m not sure whether those using this sort of ‘live’ dependency on the Internet are aware of the potential problems.

There is then a security risk of relying on unexamined code; in this day and age I think you have to be careful about the possibility of both code with accidental security holes and of malicious code being injected into a website.

The winner of CC 100

I was a little disappointed that the discussions on accu-general didn’t result in more entries, but thank you to Juan for providing his critique.

He appears to have been led a little astray by the slightly unusual comparison modes supported by Javascript (so the proposed solution no longer handles using a literal 0 as a fill value) but I think he still deserves the prize for this critique.

Code critique 101

(Submissions to scc@accu.org by Oct 1st)

I’m trying to read a list of test scores and names and print them in order.

I wanted to use exceptions to handle bad input as I don’t want to have to check after every use of the >> operator.

However, it’s not doing what I expect.

I seem to need to put a trailing /something/ on each line, or I get a spurious failure logged, and it’s not detecting invalid (non-numeric) scores properly: I get a random line when I’d expect to see the line ignored.

The scores I was sorting:

     -- sort scores.txt --
     34 Alison Day .
     45 John Smith
     32 Roger Orr
     XX Alex Brown

What I expect:

     $ sort_scores < sort_scores.txt
     Line 4 ignored
     32: Roger Orr
     34: Alison Day
     45: John Smith

What I got:

     $ sort_scores < sort_scores.txt
     Line 2 ignored
     Line 3 ignored
     0:
     32: Roger Orr
     34: Alison Day
     45: John Smith

I tried to test it on another compiler but gcc didn’t like

     iss.exceptions(true)

I tried

     iss.exceptions(~iss.exceptions())

to fix the problem.

Can you help me understand what I’m doing wrong?

Listing 3 contains sort_scores.cpp.

#include <iostream>
#include <map>
#include <sstream>
#include <string>
using pair = std::pair<std::string, 
  std::string>;
void read_line(std::string lbufr, 
  std::map<int, pair> & mmap)
{
  std::istringstream iss(lbufr);
  iss.exceptions
#ifdef __GNUC__
    (~iss.exceptions());
#else
    (true); // yes, we want exceptions
#endif
  int score;
  iss >> score;
  auto & name = mmap[score];
  iss >> name.first;
  iss >> name.second;
}
int main()
{
  std::map<int, pair> mmap;
  std::string lbufr;
  int line = 0;
  while (std::getline(std::cin, lbufr))
  try
  {
    ++line;
    read_line(lbufr, mmap);
  }
  catch (...)
  {
    std::cout << "Line " << line
      << " ignored\n";
  }
  for (auto && entry : mmap)
  {
    std::cout << entry.first << ": "
      << entry.second.first << ' '
      << entry.second.second
      << '\n';
  }
}
Listing 3

You can also get the current problem from the accu-general mail list (next entry is posted around the last issue’s deadline) or from the ACCU website (http://accu.org/index.php/journal). This particularly helps overseas members who typically get the magazine much later than members in the UK and Europe.

Notes: 

More fields may be available via dynamicdata ..