Project Code Review - LLNL-Collaboration/uiuc2016 GitHub Wiki

Project Code Review

C++ Blueprint Server

General Comments

  • Instead of definitions for all functions at the top of the file, just organize the functions so that dependency functions are defined before dependent functions.
  • Adopt the bracing style used by the remainder of the Conduit source code for the sake of consistency.
  • Instead of accessing nested Conduit data using n[x][y][z], try using n[x/y/z].
  • Try to reformat some of the lines so that they contain 80 or fewer characters.
  • Revise the code so that it isn't dependent on the existence of the radial and braid fields.
    • The simulate function can be changed to perturb one arbitrary field.
    • The verify function should be altered so that it doesn't require the existence of these fields.
  • Replace magic numbers with constant variables or passed in values.

Simulate Function

  • Instead of returning a new node with the updated data set, just update the data of the given node in-place.

Generate Update Node Function

  • It would be good to use the names of the original Blueprint subtrees in the update node (e.g. coords and fields).
  • Remove the code that adds connectivity values to the update node as its no longer needed.

Verify Node Function

  • Change negated string equality to string inequality in the shape comparison verification line.

Generate JSON Node Function

  • Introduce some spacing to break this function into logical chunks.

Test (Main) Function

  • Instead of always using wsock_path as the prefix, it would be nice if the prefix could be specified with a command-line argument.
  • Change initial_data to be a boolean value instead of an integer.

Front-End Mesh Viewer

In general, the code was pretty well organized and easy to understand

  • client.js: Please clarify and add a few comments

    Line 27: /* The C++ server is not configured to receive data from its client. */ -- what does this mean?

  • index.html: Minor tweak

    Line 4: Page title should be changed. Perhaps "Blueprint mesh viewer" ?

  • meshviewer.js: In general, it would be nice if more information about the mesh was inferred from the blueprint file. For example, the element types (quads), the field names and their associations. Some of the style could be moved to the css file. As a future enhancement, it would be nice if the users could select their own colormap for the data.

    Line 57: Looks like the client is hardcoded to quads. In general this should be determined based on the blueprint.

    Line 73: Looks like the fields are hardcoded (e.g. braid, radial, vel). These should also be determined from the blueprint

    Line 230: Color map is hard-coded. Might want to generalize the color-map a bit (future UI improvement)

    Line 245: Efficiency -- Since we know the size, it would probably be more efficient to allocate it at once rather than pushing incrementally into an empty list

    Line 320: The style for the zones/nodes is currently hard-coded per element. This can be setup in a css file if you give the zone group an id

  • ws library: This is no longer being used. Please remove.

  • testMesh.json: Consider moving this to a 'data' directory.