Title: Code Critique Competition 110 Part II

Author: Bob Schmidt

Date: 04 March 2018 22:19:03 +00:00 or Sun, 04 March 2018 22:19:03 +00:00

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

Body: 

[Web Editor's Note: Article is continued from here.]

References

[1] www.elbeno.com/blog/?p=1318

[2] https://stackoverflow.com/questions/3582608/how-to-correctly-implement-custom-iterators-and-const-iterators

Commentary

There’s not much left to say so my commentary is brief. The presenting problem contained undefined behaviour (UB), which in this case meant different people found the code worked differently depending on the compiler and flags they used. On of the nasty problems with UB is this unpredictability: code containing UB may work perfectly for years and then stop working when something changes – sometimes something very small is enough to trigger it.

Fortunately the tools are improving at detecting UB statically or dynamically – it is well worth investigating what is available for your target platform(s).

In this case, the UB came from the poorly written iterator class. As several entries pointed out, it’s generally better to try and find an available library for such components than to write it yourself, at least initially. However, if you do fail to find a suitable pre-written class, it is worth searching for help online. Last millennium there was a great book written by Matt Austern (Generic Programming and the STL: Using and Extending the C++ Standard Template Library) – but I’m not aware of a similar publication for modern C++.

It is important for C++ programmers to remember that the standard library is generally value-based and so objects may be copied which is what happens here to the arguments given to the generate() call

The Winner of CC 110

The four critiques remaining in the C++ world all found and fixed the two main issues. However, there are other problems with random number generation: both the distribution and the seed, as James noted and Pal and Jason demonstrated.

The solution seemed slightly over-engineered as there didn’t really seem to be any good reason to create two collections of the scores and then zip them together. James and Pal both pointed to simplifications that remove the need to use zipit at all. I think this is a more helpful direction for the writer of the code than trying to fix the problems with their implementation, but Jason did a pretty thorough job of listing a number of the things that would need considering if a ‘proper’ iterator is actually required.

Russel’s approach was very different; seeing what changes and what stays the same when one re-implements similar logic in a different language is very interesting. As he says, the right language to use for a particular problem depends heavily on what language(s) you are ‘at home’ in.

There were several good critiques this time – thank you to all those who spent time putting together their entry! Given that his suggestion was used in this critique Jason has requested, in the interests of fairness, that he would like not to be considered for the prize. Of the others, I have awarded the prize for this issue to Balog Pal (I particularly liked his phrasing about mixing good and bad code).

Code Critique 111

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

I’ve written a simple program to print the ten most common words in a text file supplied as the first argument to the program. I’ve tried to make it pretty fast by avoiding copying of strings. Please can you review the code for any problems or improvements.

What would you comment on and why?

Listing 3 contains the code. (Note: if you want to try compiling this on a pre-C++17 compiler you can replace string_view with string and most of the issues with the code remain unchanged.)

#include <algorithm>
#include <fstream>
#include <iostream>
#include <map>
#include <sstream>
#include <string_view>
#include <unordered_map>
#include <vector>

int main(int argc, char **argv)
{
  std::unordered_map<std::string_view, size_t> 
    words;
  std::ifstream ifs{argv[1]};
  std::string ss{
    std::istreambuf_iterator<char>(ifs),
    std::istreambuf_iterator<char>()};
	auto *start = ss.data();
  bool inword{};
  for (auto &ch : ss)
  {
    bool letter = ('a' <= ch && ch <= 'z' ||
      'A' <= ch && ch <= 'Z');
    if (inword != letter)
    {
      if (inword)
      {
        std::string_view word(
          start, &ch - start);
        ++words[word];
      }
      else
      {
        start = &ch;
      }
      inword = !inword;
    }
  }
  std::map<size_t, std::string_view> m;
  for (auto &entry : words)
  {
    auto it = m.lower_bound(entry.second);
    if (it != m.begin() || m.empty())
    {
      m.insert(it,
        {entry.second, entry.first});
      if (m.size() > 10)
      {
        m.erase(m.begin());
      }
    }
  }
  for (auto &entry : m)
  {
    std::cout << entry.first << ": "
      << entry.second << '\n';
  } 
}
			

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 ..