On: Concord, our GPL outliner

[Stratoscope]

I've been browsing through the code a bit. It has a few problems.

First, the brace and indentation style is extremely unusual and hard to follow. I have never seen this style used in another curly-brace-language project anywhere.

I realize that brace and indentation is a very personal preference, but I've had no trouble following a variety of styles - until this one. Here's an example:

  if(c.op.inTextMode()){
      c.op.focusCursor();
      c.editor.restoreSelection();
      }else{
          c.pasteBinFocus();
          }

The if block and matching else block have different indentation levels? What? That does seem to be the pattern followed in a number of places.

How would you follow this style if there is a chain of else-if's? I don't see any examples in the code, but it seems it would have to look like this to be consistent:

  if(a){
      b();
      }else if(c){
          d();
          }else if(e){
              f();
              }else{
                  g();
                  }

OTOH, this function follows two different indentation styles for its two if/else blocks:

  this.callbacks = function(callbacks) {
      if(callbacks) {
          this.root.data("callbacks", callbacks);
          return callbacks;
      } else {
          if(this.root.data("callbacks")) {
              return this.root.data("callbacks");
              } else {
                  return {};
                  }
          }
      };

Try this quiz: Without loading the code into a brace-matching editor, just eyeball that function and see if you can tell which open brace matches with which close brace.

The function could be much simpler too:

  this.callbacks = function( callbacks ) {
      return callbacks ?
          this.root.data( "callbacks", callbacks ) :
          this.root.data( "callbacks" ) || {};
  };

Also in a half-dozen places or so, the code uses a for..in loop to iterate over an array. This type of loop should never be used with an array for two reasons: If any other code in the page extends Array.prototype the code will break, and the order of iteration is not guaranteed. These loops need to be converted to numeric for loops or $.each() or any similar alternative.

One last point that is just a matter of taste, but I think the code would benefit quite a bit if it followed the popular jQuery convention of using a $ prefix on variables that contain jQuery objects.

I love outliners, and I'd really like to contribute to this project, but the code is so strangely formatted that I'd have a really hard time with it. I wonder if the authors would consider adopting a more conventional coding style, or if they just like it the way it is?

[davewiner → Stratoscope]

I think the programmer was just getting comfortable with coding in an outliner when he wrote that code. I would consider that indentation a bug, when and if I work in those areas, I'll fix the indentation there. Thanks for pointing it out.

[Stratoscope → davewiner]

Ah, thanks, Dave, and I'm sorry I went a bit overboard on my remarks about the code! The project does look promising; now I'm encouraged to check it out some more. :-)

[davewiner → Stratoscope]

Not at all! :-)

I am a total bugger about formatting code.

The outliner approach is pretty good but not perfect.

If-then-else is the sore thumb, for sure.

Along with try-catch.

But overall, outliners are a huge win for writing code.

Let's be hardasses about this stuff, but also understanding.

I think your approach is perfect.