{"action":"opened","issue":{"url":"https://api.github.com/repos/phetsims/area-builder/issues/83","labels_url":"https://api.github.com/repos/phetsims/area-builder/issues/83/labels{/name}","comments_url":"https://api.github.com/repos/phetsims/area-builder/issues/83/comments","events_url":"https://api.github.com/repos/phetsims/area-builder/issues/83/events","html_url":"https://github.com/phetsims/area-builder/issues/83","id":122088450,"number":83,"title":"ShapeCreatorNode nits","user":{"login":"pixelzoom","id":3046552,"avatar_url":"https://avatars.githubusercontent.com/u/3046552?v=3","gravatar_id":"","url":"https://api.github.com/users/pixelzoom","html_url":"https://github.com/pixelzoom","followers_url":"https://api.github.com/users/pixelzoom/followers","following_url":"https://api.github.com/users/pixelzoom/following{/other_user}","gists_url":"https://api.github.com/users/pixelzoom/gists{/gist_id}","starred_url":"https://api.github.com/users/pixelzoom/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/pixelzoom/subscriptions","organizations_url":"https://api.github.com/users/pixelzoom/orgs","repos_url":"https://api.github.com/users/pixelzoom/repos","events_url":"https://api.github.com/users/pixelzoom/events{/privacy}","received_events_url":"https://api.github.com/users/pixelzoom/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":{"login":"jbphet","id":3082831,"avatar_url":"https://avatars.githubusercontent.com/u/3082831?v=3","gravatar_id":"","url":"https://api.github.com/users/jbphet","html_url":"https://github.com/jbphet","followers_url":"https://api.github.com/users/jbphet/followers","following_url":"https://api.github.com/users/jbphet/following{/other_user}","gists_url":"https://api.github.com/users/jbphet/gists{/gist_id}","starred_url":"https://api.github.com/users/jbphet/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jbphet/subscriptions","organizations_url":"https://api.github.com/users/jbphet/orgs","repos_url":"https://api.github.com/users/jbphet/repos","events_url":"https://api.github.com/users/jbphet/events{/privacy}","received_events_url":"https://api.github.com/users/jbphet/received_events","type":"User","site_admin":false},"milestone":null,"comments":0,"created_at":"2015-12-14T17:17:43Z","updated_at":"2015-12-14T17:17:43Z","closed_at":null,"body":"While working on Function Builder, I looked at ShapeCreateNode for inspiration.  I noticed a few nits that I thought I'd mention, since they affected my ability to understand the code.   Up to you whether to address either (or neither) of these.\r\n\r\n(1) parent ScreenView\r\n\r\nIn this bit of code (line 97):\r\n\r\n```js\r\n        // Find the parent screen by moving up the scene graph.\r\n        var testNode = self;\r\n        while ( testNode !== null ) {\r\n          if ( testNode instanceof ScreenView ) {\r\n            this.parentScreen = testNode;\r\n            break;\r\n          }\r\n          testNode = testNode.parents[ 0 ]; // Move up the scene graph by one level\r\n        }\r\n```\r\n\r\nThe comments and var name indicate that we're looking for a Screen, when in fact we're looking for a ScreenView.  Recommended to rename `this.parentScreen` to `this.parentScreenView` and adjust doc.\r\n\r\nThis bit of code is called every time a drag starts.  Unless you plan on moving this Node between ScreenViews (unlikely), it's only necessary to find the parent ScreenView the first time a drag is initialized.  Recommended to wrap the above code in `if ( !this.parentScreenView ) { ... }`.\r\n\r\nCould also use an assertion that verifies that you actually found a parent ScreenView.\r\n\r\n\r\n(2) Test for unit square\r\n\r\nNext is this bit of code:\r\n\r\n```js\r\n if ( options.creationLimit < Number.POSITIVE_INFINITY &&\r\n         ( shape.bounds.width !== AreaBuilderSharedConstants.UNIT_SQUARE_LENGTH ||\r\n           shape.bounds.height !== AreaBuilderSharedConstants.UNIT_SQUARE_LENGTH ) ) {\r\n // The ability to set a creation limit ONLY works for unit squares.  The reason for this is that non-unit shapes\r\n...\r\n```\r\n\r\nThis is a dubious method of testing for a unit square.  A circle with diameter === AreaBuilderSharedConstants.UNIT_SQUARE_LENGTH would also pass the test.  Not an issue in the current implementation, but a potential landmine for anyone extending the sim in the future.  A better approach would be have a type hierarchy of Shape (UnitSquare, ...) and test that `shape instanceof UnitSquare`. "}}