Skip to main content
added 332 characters in body
Source Link
Rene Saarsoo
  • 2.1k
  • 11
  • 14

A few things immediately caught my eye:

  • You're using a weird construct: new function() {}. The function(){} expression already creates a new function, but in addition to that you're using it with new to invoke it as a constructor for new object. The way you're using it, you could easily replace it with just plain object literal. For example in js_util:

      js_util = {
          isWhitespaceCharacter: function(ch) {
              return (ch == ' ' || ch == '\n' || ch == '\r' || ch == '\t');
          },
          ...
      };
    

    But you really need to familiarize yourself with JavaScript's prototypal object model. Otherwise you're just programming by coincidence - without really knowing what the code does.

  • Don't use the with statement. It makemakes code hard to read and reason about. It's one of the grand mistakes in JavaScript language. Avoid it. In your case, just prefix the instance variables/functions with this. - it's not so long to write.

  • It's very unusual to place the opening { on a separate line in JavaScript. This kind of indentation style is rarely seen. I suggest you adopt the common style of placing { on the same line as if etc.

  • The common variable naming convention in JavaScript is to use camelCase not under_scores.

  • Your is_whitespace function could be much more easily implemented with just a simple regular expression:

      isWhitespace: function(s) {
          return /^\s*$/.test(s);
      }
    

And don't take my word for it. Run your JavaScript through an analyzer like JSHint - it will warn you about these and other issues.

PS. I think overall your code is very nicely and cleanly written. It just falls short on looking and feeling like JavaScript.

A few things immediately caught my eye:

  • You're using a weird construct: new function() {}. The function(){} expression already creates a new function, but in addition to that you're using it with new to invoke it as a constructor for new object. The way you're using it, you could easily replace it with just plain object literal. For example in js_util:

      js_util = {
          isWhitespaceCharacter: function(ch) {
              return (ch == ' ' || ch == '\n' || ch == '\r' || ch == '\t');
          },
          ...
      };
    

    But you really need to familiarize yourself with JavaScript's prototypal object model. Otherwise you're just programming by coincidence - without really knowing what the code does.

  • Don't use the with statement. It make code hard to read and reason about. It's one of the grand mistakes in JavaScript language. Avoid it. In your case, just prefix the instance variables/functions with this. - it's not so long to write.

  • It's very unusual to place the opening { on a separate line in JavaScript. This kind of indentation style is rarely seen. I suggest you adopt the common style of placing { on the same line as if etc.

  • The common variable naming convention in JavaScript is to use camelCase not under_scores.

And don't take my word for it. Run your JavaScript through an analyzer like JSHint - it will warn you about these and other issues.

A few things immediately caught my eye:

  • You're using a weird construct: new function() {}. The function(){} expression already creates a new function, but in addition to that you're using it with new to invoke it as a constructor for new object. The way you're using it, you could easily replace it with just plain object literal. For example in js_util:

      js_util = {
          isWhitespaceCharacter: function(ch) {
              return (ch == ' ' || ch == '\n' || ch == '\r' || ch == '\t');
          },
          ...
      };
    

    But you really need to familiarize yourself with JavaScript's prototypal object model. Otherwise you're just programming by coincidence - without really knowing what the code does.

  • Don't use the with statement. It makes code hard to read and reason about. It's one of the grand mistakes in JavaScript language. Avoid it. In your case, just prefix the instance variables/functions with this. - it's not so long to write.

  • It's very unusual to place the opening { on a separate line in JavaScript. This kind of indentation style is rarely seen. I suggest you adopt the common style of placing { on the same line as if etc.

  • The common variable naming convention in JavaScript is to use camelCase not under_scores.

  • Your is_whitespace function could be much more easily implemented with just a simple regular expression:

      isWhitespace: function(s) {
          return /^\s*$/.test(s);
      }
    

And don't take my word for it. Run your JavaScript through an analyzer like JSHint - it will warn you about these and other issues.

PS. I think overall your code is very nicely and cleanly written. It just falls short on looking and feeling like JavaScript.

Source Link
Rene Saarsoo
  • 2.1k
  • 11
  • 14

A few things immediately caught my eye:

  • You're using a weird construct: new function() {}. The function(){} expression already creates a new function, but in addition to that you're using it with new to invoke it as a constructor for new object. The way you're using it, you could easily replace it with just plain object literal. For example in js_util:

      js_util = {
          isWhitespaceCharacter: function(ch) {
              return (ch == ' ' || ch == '\n' || ch == '\r' || ch == '\t');
          },
          ...
      };
    

    But you really need to familiarize yourself with JavaScript's prototypal object model. Otherwise you're just programming by coincidence - without really knowing what the code does.

  • Don't use the with statement. It make code hard to read and reason about. It's one of the grand mistakes in JavaScript language. Avoid it. In your case, just prefix the instance variables/functions with this. - it's not so long to write.

  • It's very unusual to place the opening { on a separate line in JavaScript. This kind of indentation style is rarely seen. I suggest you adopt the common style of placing { on the same line as if etc.

  • The common variable naming convention in JavaScript is to use camelCase not under_scores.

And don't take my word for it. Run your JavaScript through an analyzer like JSHint - it will warn you about these and other issues.