Bending Light Code Review 3.0

[samreid]

A brief history on the code review of this sim:

1. The initial port was completed by Actual Concepts and reviewed by @samreid in #44. This code review resulted in many significant changes
2. After that review, numerous large changes were made to the simulation, including but not limited to layout, monochromatic light rendering and white light rendering, as well as how modules are structured and inter-related. We are now up to issue #267 or more.
3. @samreid completed "code review 2.0" in #251

Since there has been such massive churn in this simulation, I recommend to treat #251 "code review 2.0" as @samreid's preparations for review by another PhET developer. I think it would be sufficient to start with a small amount of time (say 1-2 hours) and evaluate whether more time is warranted after that. My main questions are:

Coding conventions

• [x] Does the code style meet PhET's code-style guidelines, as described in the “Coding Style Guidelines” section of "PhET Development Overview"?

Documentation

• [x] Are documentation conventions followed, as described in the “Coding Style Guidelines” section of the PhET Development Overview?

Organization, Readability, Maintainability

• [x] Does the organization and structure of the code make sense? Do the model and view contain types that you would expect (or guess!) by looking at the sim? Do the names of things correspond to the names that you see in the user interface?
• [x] Are appropriate design patterns used?
• [x] Is inheritance used where appropriate? Does the type hierarchy make sense?
• [x] Is there any unnecessary coupling? (e.g., by passing large objects to constructors, or exposing unnecessary properties/functions)
• [x] Are the source files reasonable in size? Scrutinize large files with too many responsibilities - can responsibilities be broken into smaller delegates?
• [x] Are any significant chunks of code duplicated?
• [x] Is there anything that should be generalized and migrated to common code?

@ariel-phet can you delegate this?

[ariel-phet]

@samreid - my temptation would be to give this code review to @jessegreenberg especially since he has been interested in WebGl...this might give him more exposure to the types of patterns we have been using with that as a renderer. Any thoughts?

[samreid]

@jessegreenberg would be a great reviewer for what is needed here, but WebGL is only a tiny part of the complete implementation (only 1 of the 64 files deals with WebGL).

To learn more about how we have been using WebGL it would be more instructive to look through a broader range of simulations, searching for WebGLNode.call for subclasses. And I highly recommend the red Professional WebGL Programming book (I believe we have a hard copy or two, and perhaps online copies available through CU): http://www.wrox.com/WileyCDA/WroxTitle/Professional-WebGL-Programming-Developing-3D-Graphics-for-the-Web.productCd-1119968860.html

[samreid]

@jessegreenberg the code is ready for review, please see my specific questions in the issue description above (this is just a subset of the entire code review checklist).

[jessegreenberg]

@ariel-phet and @samreid, I am on it! Happy to do this code review and address the checklist in the issue description.

@samreid, I am aware of the red Professional WebGL Programming book, but I have yet to get my hands on it. Thanks for the recommendation.

To learn more about how we have been using WebGL it would be more instructive to look through a broader range of simulations, searching for WebGLNode.call for subclasses.

Agreed, this would be a great place for me to start learning WebGL in the context of PhET sims.

[jessegreenberg]

I am going to take a look at #269 tomorrow, but I consider this code review complete. Reassigning to @samreid to close this issue unless he thinks there is anything left to discuss or review for this issue.

[samreid]

@jessegreenberg thanks for your time and suggestions, I'll close this issue and continue in the new issues mentioned above.