[racket-dev] RFC: Coding Guidelines

On Aug 19, Jay McCarthy wrote:
> I hope I've incorporated much of the feedback from this thread:
> 
> http://faculty.cs.byu.edu/~jay/tmp/201008161509-guidelines.html

I (finally) read this and the thread that went on at the time, and I
don't see any point in all of this, besides a vage plea to encourage
tests, and a slightly more concrete (but impractical) call for stress
tests.

* It talks about code in general -- no distinction between introducing
  new functionality, and fixing existing code.  It looks like the
  emphasis is on the former, but that's not clear.  These two cases
  are very different.

* For the latter case of fixing someone else's code, much of this is
  irrelevant.  If I see a bug in a library, and I tell the author
  about it but it is not fixed after a while -- if the fix is simple,
  I'll probably push it.  (*I*'ll probably fix it right away.)  But
  does that mean that the responsibility of keeping the code (and
  therefore maintaining a test suite) is now all mine?  The text makes
  it look that way, but if these rules were enforced in some concrete
  way (for example -- you touch the slideshow code, and you get to be
  responsible for all its bugs) the net result would be no fixes.

* So obviously some concrete ownership change is out of the question.
  Is there some implicit change, or something less radical like "you
  own your changes"?  Probably -- but such things are hard to
  impossible to formalize in any way.  I think that ultimately it's
  something that gets settled between the owner of the code and the
  person fixing it.

  As someone who was very often at the fixing side, I've just learned
  to deal with things on a case by case basis.  In some cases the
  owner is happy for any fix, even if testing it is very difficult (or
  if the fix is obvious that there's no need for a test).  In other
  cases the owner rejected any change and my fixes got dumped -- and I
  learned to completely avoid such pieces of code.  It yet other cases
  the owner is relatively new, and I'll do more radical changes.  The
  bottom line is that I don't see any formal way to formalize sane
  rules and keep moving at the same time.

* "As long as your code fulfills its promises" seems like a weasel
  passage that can admit anything -- it's me who writes the code and
  me who does the promising, so I can always claim that I never
  promised [whatever you don't like about it, including bugs].

* There's an emphasis on stress testing -- but I don't see anything
  concrete there, and I don't see any way of making any concrete
  conclusions wrt such testing.  To give a few examples:

  - We still have the `seqn' operators in, and they still imply a
    runtime cost of ~200.  What should I do in light of the stress
    tests passage?
    - Add a test that will actually break, knowing that fixing it is
      not a matter of some local change to the code?
    - Remove the code, because a 200x is bad enough to be considered a
      bug?
    - Spend a month rewriting how sequences are represented to
      actually solve the problem?
    - Shut up about it, and learn to ignore it?

  - The unstable/queue code had a much smaller performance hit, but
    the code in question was simple enough to make that hit much more
    noticeable, so in that case even a small 5x factor is bad.

  - In Hari's code there is generally a similar performance hit, but
    in that case the added functionality (being pure) is worth it, and
    the code is less simple (certainly not something I'd want to write
    myself whenever I need purity).  So a 5x factor is not a problem.

* A minor note: the text refers to the tests/stress/stress library,
  but that code has no real documentation.  (Looking at the code,
  there's some aspects that should really go into `time' -- like the
  ability to run the body multiple times, and average the results.)

* "\"Primum non nocere\"" -- after looking this up (bad for such a
  document), I strongly disagree with it.  IIUC, it reads as "if it
  works, don't mess with it" -- which is a recipe for an ever growing
  codebase with almost no changes to existing code.  There are places
  where this does apply (eg, everything around drracket involve
  subtle state-based issues that random by-passers are unaware of),
  and places where it clearly should not apply.

  The question should really involve ownership: your own dialog with
  the owner of the code, or your willingness to become its owner.

* Speaking about ownership -- IMO it is one of the important aspect of
  any "coding guidelines", yet there is nothing about it in the text.
  So IMO this is a huge omission.

  More specifically, much of my criticism of the unstable collection
  is related to ownership -- code being dumped as is and left as an
  orphan with noone to take care of problems.  (Yes, there's a policy
  of clarifying the owner of each bit, but the bottom line is that
  except for a few things, there was no discussion on it and many of
  the comments that I wrote are unanswered.)

* Thinking about this, I think that a proper document would be
  something that makes me view the unstable collection more favorably.
  (No, that's not impossible.)  Right now, it looks pretty decent when
  judging by these guidelines -- the functions are all documented and
  tested -- the problems are in other aspects.

* Yet another huge ommision from such a document is style.  This comes
  in several flavors:

  - Low-level aspects of the files: no newline at the end of a file,
    using tabs, empty lines in weird places, spaces at end of lines,
    etc.

  - Higher level: inconsistent indentation, use of "#lang" vs "(module
    ...)", style of in-code comments, etc.

  - Even higher level: consistent naming schemes for module exports,
    deciding what is part of the external API and what's internal,
    splitting a library into too many or two few modules, splitting a
    module into too many or too few functions, adding too many
    expensive contracts or not adding contracts at all, dealing with
    code dependencies.

  - Also includes meta-information: what should commit messages
    include? what group of changes makes a good commit? what should be
    included in release announcements? why don't we have a changelog?

-- 
          ((lambda (x) (x x)) (lambda (x) (x x)))          Eli Barzilay:
                    http://barzilay.org/                   Maze is Life!