#1490 ✓ checked-in
David Humphrey

No else after return/throw

Reported by David Humphrey | July 15th, 2011 @ 03:50 PM | in 1.3.0 (closed)

We have places where we break this rule. We need to return early, not maintain unnecessary branches. For example, in PImage.resize:

     this.resize = function(w, h) {
        if (this.isRemote) { // Remote images cannot access imageData                             
          throw "Image is loaded remotely. Cannot resize.";
        } else {

Comments and changes to this ticket

  • Pomax

    Pomax July 16th, 2011 @ 01:48 PM

    those two things are not mutually exclusive. this code returns early and branches. else after a throw or return is indeed silly, let's get some regexp going on to find all instances.

  • Lonnen

    Lonnen July 17th, 2011 @ 03:28 PM

    • Assigned user changed from “David Humphrey” to “Lonnen”
  • Lonnen

    Lonnen July 18th, 2011 @ 05:11 PM

    Did some work, but I'm going to have to revisit this later in the week. Leaving some notes for when I come back.

    code: https://github.com/Lonnen/processing-js/blob/t1490-no-else/processi...

    The fake-dom had to be extended because a new code path was exposed by the patch, and processing-js/test/parser/Fry-Visualizing-Data/ch06-zipdecode/round_09c_focus_handling/round_09c_focus_handling.pde was failing. This path needs, at least, a ref test before this is ready for review. Follow the call graph up starting from Drawing3D.prototype.text$line.

  • Lonnen

    Lonnen July 24th, 2011 @ 12:40 AM

    Branch: https://github.com/Lonnen/processing-js/tree/t1490-no-else
    Commit: https://github.com/Lonnen/processing-js/commit/efd5d8ae2995f212c052...

    Ok. The code path was exposed through error on my part. There was a series of if / else statements where some returned and some didn't. I broke it after the returning else, causing condition 1 to execute the code for condition 1 and 3.

    Whoever reviews this is going to need to go over the diff carefully. The tests all pass, and I've been over it a few times, but its a long series of almost-the-same changes and its easy to make a simple mistake.

    Additionally, fixing this hiccup lead me to another question. There are places in the code base where we have if / else switches that could have returns added to them. Is this worthwhile, and if so do we do this in a second ticket? (example: https://github.com/Lonnen/processing-js/blob/80b0832b8a94fb45cf01aa...)

  • David Humphrey

    David Humphrey July 24th, 2011 @ 01:47 AM

    • Assigned user changed from “Lonnen” to “David Humphrey”
    • State changed from “assigned” to “peer-review-requested”
  • David Humphrey

    David Humphrey July 25th, 2011 @ 05:24 PM

    • Assigned user changed from “David Humphrey” to “Lonnen”
    • State changed from “peer-review-requested” to “review-needs-work”

    This is going to require some rebasing, or careful merging, since I notice you've touched some code I've since changed in develop.

    PR-

    • I think this is better done with a break:
    +        if(this.nameTable.length > 0) {                                                                             
    +          for(i = 0, j = this.nameTable.length; i < j || found; i++) {      
    +            if(this.nameTable[i].getName === child) {                      
    +              found = this.nameTable[i];
    
    should probably be:
    
    +        if(this.nameTable.length > 0) {                                                                             
    +          for(i = 0, j = this.nameTable.length; i < j; i++) {      
    +            if(this.nameTable[i].getName === child) {                      
    +              found = this.nameTable[i];
    +              break;
    
    • I don't see the point in these IF checks, since XMLElement also checks argument length, and acts accordingly. Ask Pomax if this change is OK:
          createElement: function () {
            if (arguments.length === 2) {
              return new XMLElement(arguments[0], arguments[1], null, null);
            } 
            return new XMLElement(arguments[0], arguments[1], arguments[2], arguments[3]);
    
    should be
    
          createElement: function () {
            return new XMLElement(arguments[0], arguments[1], arguments[2], arguments[3]);
          }
    
    • Same for hasAttribute, getStringAttribute, getFloatAttribute, etc...just pass the args through, since getAttribute does the checks. Confirm with Pomax.

    • Cache this.children:

    +        if (this.children.length === 1 && this.children[0].type === "TEXT") {
              return this.children[0].content; 
    
    should be
    
    var children = this.children
    if (children.length === 1 && children[0].type === "TEXT") {
      return children[0].content; 
    
    • Use named arguments when present. p.subset should be (code untested):
         p.subset = function(array, offset, length) {
           var end = length ? offset + length : array.length;
           return array.slice(offset, end); 
         };          
    
    • Same for p.expand (code untested):
    p.expand = function(array, newSize) {
      var temp = array.slice(0),
          newSize = newSize || array.length * 2;
      temp.length = newSize;
      return temp;
    };
    
    • Don't use isNaN, use a!==a
    if (isNaN(binaryString)) { 
    
    should be
    
    if (binaryString !== binaryString)
    
    • Kill the extra paths in createFont, and update the docs above it about not supporting smooth, charset:
    p.createFont = function(name, size, smooth, charset) {
      // NOTE: smooth and charset are ignored in processing.js
      p.textSize(size);
      return p.loadFont(name);
    
    };
    

    After you fix this, I'd like to get both Jon and Pomax to review as well, since this is so massive.

  • Lonnen

    Lonnen July 26th, 2011 @ 03:28 AM

    I don't see the point in these IF checks, since XMLElement also checks argument length, and acts accordingly. Ask Pomax if this change is OK

    If arguments[3] is passed when arguments.length==2 the new function's arguments will look like it has length==4. The actual values are padded with undefined values. I started making these changes and some of them were benign, but then our tests started failing, and I ended up rolling them back.

    I finished the other edits for this comments, but it's late so the rebase will have to wait until tomorrow.

  • Lonnen

    Lonnen July 26th, 2011 @ 01:23 PM

    10:16am July 26, 2011 - Processing.js IRC - Pomax:

    hmm. there's no point in if checks with null arguments, they're going to already be undef just make new XMLElememnt test for undef instead and there no need for branching in createElement anymore.

  • Lonnen

    Lonnen July 26th, 2011 @ 02:22 PM

    11:02 am July 26, 2011 - Processing.js IRC -
    Lonnen:

    when I look at the XMLElement its checking arguments.length its not checking for nulls

    Pomax:

    this function needs explicit arguments. jbuck already added that in a lot of places, but XMLElement needs constructor args too I recommend filing a separate ticket for that, rather than tackling that in 1490

    done: https://processing-js.lighthouseapp.com/projects/41284/tickets/1540...

  • Lonnen

    Lonnen July 26th, 2011 @ 04:36 PM

    • Assigned user changed from “Lonnen” to “Jon Buckley”
    • State changed from “review-needs-work” to “peer-review-requested”

    The linter complains that if (binaryString !== binaryString) is a "Weird relation", so I've stuck with isNaN for the moment.

    Branch: https://github.com/Lonnen/processing-js/tree/t1490-no-else
    Commit(s):
    https://github.com/Lonnen/processing-js/commit/99ad7081a12649f42cbb... (the merge)
    https://github.com/Lonnen/processing-js/commit/dfe1d3a4a9d86c5f8257... (today PR changes)
    https://github.com/Lonnen/processing-js/commit/2877868863e4775459c9... (last nights PR changes)
    https://github.com/Lonnen/processing-js/commit/efd5d8ae2995f212c052... (first diff)

    You'll probably want to branch diff this one.

    Tests Completed - 0 failed, 15 known failures, 146 passed, 161 total.
    TEST SUMMARY: 2303 passed, 0 failed (6 known), 2309 total

  • Lonnen
  • Jon Buckley

    Jon Buckley July 26th, 2011 @ 05:34 PM

    All tests pass in browser and js shell on OS X
    I need to finish the review later, but it looks good so far!

  • Jon Buckley

    Jon Buckley July 27th, 2011 @ 01:56 PM

    • Assigned user changed from “Jon Buckley” to “Lonnen”
    • State changed from “peer-review-requested” to “review-needs-work”
    • Milestone order changed from “81” to “0”

    PR-:

    • There's one more function that could use some no elsing, getChild @@ -4733,29 +4735,29 @@
    • There's a bunch of lines with trailing whitespace. This can probably be fixed with a regex
    • p.expand probably needs to lose temp.length = ary.length * 2; because the newSize variables takes the doubling into account
    • p.subset doesn't match P5 behaviour when length === 0. This can be fixed by changing the ternary to length !== undef. You can also add a unit test to subset.pde:
      • _checkEqual(subset(aa1, 1, 0), {});

    For future merge conflict Jon, the only thing added to fake-dom.js was:

    •             drawElements: __empty_func__,
      
  • Lonnen

    Lonnen July 29th, 2011 @ 04:21 PM

    • State changed from “review-needs-work” to “super-review-requested”
    • Assigned user changed from “Lonnen” to “Jon Buckley”
  • Jon Buckley
  • Jon Buckley

    Jon Buckley August 2nd, 2011 @ 11:52 AM

    • State changed from “super-review-requested” to “review-looks-good”
  • David Humphrey

    David Humphrey August 2nd, 2011 @ 02:15 PM

    The only thing I'd say about this patch is that I get 20,435ms (this patch) vs. 18,860ms (develop) for perf tests on Mac/FF5. So either we landed a bunch of stuff that's faster on develop recently, or this patch makes us slower. It would be good to do some testing after we land it.

  • Jon Buckley

    Jon Buckley August 2nd, 2011 @ 08:52 PM

    develop

    • Chrome = 12058ms
    • Firefox 5 = 14911ms
    • Safari = 21798ms
    • Opera = 42709ms

    develop + 1490

    • Chrome = 16107ms
    • Firefox 5 = 14613ms
    • Safar = 21873ms
    • Opera = 42247ms

    Do we want to land this now, or figure out why we got slower with this patch?

  • Jon Buckley

    Jon Buckley August 2nd, 2011 @ 09:03 PM

    • Assigned user changed from “Jon Buckley” to “David Humphrey”
    • State changed from “review-looks-good” to “super-review-requested”

    Assigning to Dave for a decision

  • Jon Buckley

    Jon Buckley August 3rd, 2011 @ 04:43 PM

    • State changed from “super-review-requested” to “review-needs-work”
    • Assigned user changed from “David Humphrey” to “Lonnen”

    We need to figure this out. Here's a table with the slow tests:

    
    path                        |develop|t1490
    ----------------------------+-------+-----
    pimage-blend-blend.pde      |   153 |  204
    pimage-blend-add.pde        |   198 |  263
    pimage-blend-subtract.pde   |   213 |  254
    pimage-blend-darkest.pde    |   212 |  275
    pimage-blend-lightest.pde   |   203 |  251
    pimage-blend-difference.pde |   179 |  231
    pimage-blend-exclusion.pde  |   185 |  253
    pimage-blend-multiply.pde   |   179 |  228
    pimage-blend-screen.pde     |   179 |  233
    pimage-blend-overlay.pde    |   572 | 1253
    pimage-blend-hardlight.pde  |   595 | 1255
    pimage-blend-softlight.pde  |   560 | 1247
    pimage-blend-dodge.pde      |   582 | 1279
    pimage-blend-burn.pde       |   586 | 1280
    

    So somehow the changes here made blend() really slow, especially the last 5.

  • David Humphrey

    David Humphrey August 3rd, 2011 @ 10:15 PM

    I sort of don't believe that Lonnen's patch could have done this. Are we sure it was this commit that did it, and there's nothing else between t1490 and develop?

  • Lonnen

    Lonnen August 4th, 2011 @ 12:55 AM

    Jbuck -- if you have a minute, you can clone your develop branch to a new branch, then git rebase -i <last sha from my merge>~5 and simply delete my stuff. Let rebase do the work and see if the merge conflicts are horrible. There should be one trivial one, and if that's the only one you'll have a good comparison for times with and without my check in.

  • Lonnen

    Lonnen August 4th, 2011 @ 12:59 AM

    Will do it myself if I get the time, but that could be a few days to a week.

  • Jon Buckley

    Jon Buckley August 4th, 2011 @ 12:50 PM

    • Assigned user changed from “Lonnen” to “Jon Buckley”
    • State changed from “review-needs-work” to “review-looks-good”

    There's nothing else between t1490 and develop. But there is a brand new stable version of Chrome pushed out on August 2nd when I started my testing: http://googlechromereleases.blogspot.com/2011/08/stable-channel-upd...

    After checking out develop and various other points in our history, all of them suffer from the same problem where on a fresh tab the perf tests work fine, but page refreshes slow down. SR+

  • David Humphrey
  • Jon Buckley

    Jon Buckley August 4th, 2011 @ 01:19 PM

    • State changed from “review-looks-good” to “staged”

    Staged: https://github.com/jbuck/processing-js/commit/f8fd0650ecba35de00095...

    And I got 11670ms in Chrome on the first run, so no perf loss :)

  • Jon Buckley

    Jon Buckley September 10th, 2011 @ 05:34 PM

    • State changed from “staged” to “checked-in”
    • Milestone order changed from “82” to “0”

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

Processing.js is an open programming language for people who want to program images, animation, and interactions for the web without using Flash or Java applets. Processing.js uses Javascript to draw shapes and manipulate images on the HTML5 Canvas element. The code is light-weight, simple to learn and makes an ideal tool for visualizing data, creating user-interfaces and developing web-based games.

Shared Ticket Bins

Pages