Quality Plateau

Concept from the ProgrammersStone:

When one adopts the strategy of forming one's own mental map of a problem domain and attempting to simplify it, one faces the problem of when to stop working on the map. This applies at every level of design. The extraordinary thing is that there almost always is a deep solution, that is significantly simpler than anything else, and manifestly minimal. (There may be more than one way of expressing it, but then the relationship will be manifest.) Although homilies like "You'll know it when you see it!" are undoubtedly true, they don't tell one where to look.

This is followed by a sublime demonstration of the art of refactoring, in which a function from a book on Windows thread programming is reduced from 26 lines to 10. I can't put it better than the authors, so won't try:

Ten lines vs 26. One layer maximum indentation instead of three. No else clauses. Absolutely no nested elses. No ifs! No comments required, less places for bugs to hide.

ExtremeProgramming produces code which converges on the QualityPlateau.

-- DavidHarvey

Alan Carter once posted a message to the progstone list entitled "ADHD Manifesto", although in actual fact it was more about the QualityPlateau and what he calls the "deep structure". He suggests that the QualityPlateau is equivalent to NecessarySufficiency? in mathematics, and good composition in art, as defined by the phrase "if any element were to be missing or changed, the whole composition would be changed". A reformatted version of the message is at: http://www.livejournal.com/users/acheron_hades/98323.html ( BrokenLink 20120529 )

-- SteveDodd

---

Around the third version of the function, the ReleaseMutex? call moves in front of the update of g_dwTimes. If the purpose of the mutex is to provide mutual exclusion while updating the g_dwTimes array (and this seems likely), then they have just introduced a bug into otherwise working code. This creates a race condition that would probably even evade detection by XP UnitTests.

There may be a potential problem with comparing index to MAX_TIMES outside the mutex protected region as well.

However, the refactored final version of the code makes it easier to spot the problem!

-- JimWeirich

It turns out that this problem is actually quite easy to fix: however, the case illustrates the point nicely that the structure of the code by itself is not enough to guide refactoring. Here's the original form of the refactored code:

  DWORD secondThread(void *threadParm)
  {
    while(Index < MAX_TIMES &&
          WaitForSingleObject(Mutex, INFINITE) == WAIT_OBJECT_0)
    {
      ReleaseMutex(Mutex);  // (1)
      Times[Index++] = GetTickCount();  // (2)
    }
    return(0);
  }

Two problems here: (1) This is in the wrong place, as observed above. The authors make a point about "moving the release to the right place", this is clearly wrong. You can simply leave this in the right place for most of the refactoring.

More serious is (2) - Index is global, and may be accessed by other threads. The test in the while-expression is made before the mutex is waited on, which means by the time you enter the loop, it's value may have changed, leading to overwriting the end of the array. The only way around this is to repeat the test inside the loop. Doing this, and inverting the statements, gives the following, which is evidently correct, and still much clearer than the original:

  DWORD secondThread(void *threadParm)
  {
    while(Index < MAX_TIMES &&
          WaitForSingleObject(Mutex, INFINITE) == WAIT_OBJECT_0)
    {
      if (Index < MAX_TIMES)
        Times[Index++] = GetTickCount();
      ReleaseMutex(Mutex);
    }
    return 0;
  }

-- DavidHarvey

This is fixed in the current version of the Programmer's Stone at http://www.reciprocality.org/Reciprocality/r0/index.html, and has been for a long time now, I believe.

-- SteveDodd

EditHint: The connection between the title (plateau) is not explained. Even though I think that I understand what is meant, I cannot be sure that my analogy is correct. -- GunnarZarncke