Created separate file for VariableMap by marisaleung · Pull Request #1079 · RaspberryPiFoundation/blockly

marisaleung · 2017-05-03T21:12:27Z

This change is

rachel-fenichel · 2017-05-03T21:28:00Z

Unit tests are failing, since you added a new file.

Please rebuild and add only blockly_uncompressed.js to this PR.

marisaleung · 2017-05-03T21:32:22Z

Done

On Wed, May 3, 2017 at 2:28 PM, Rachel Fenichel ***@***.***> wrote: Unit tests are failing, since you added a new file. Please rebuild and add only blockly_uncompressed.js to this PR. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1079 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AZ_e9KZu3KTz6wEO8wwt_NI8iJTKbbf6ks5r2PFngaJpZM4NP9cK> .

rachel-fenichel

Some nits but generally looks good to me.

Can any of the workspace tests be moved into a new test file, now that you've moved a bunch of the logic to be internal to VariableMap?

Github says you've got merge markers left in your uncompressed file.

rachel-fenichel · 2017-05-03T21:33:37Z

core/variable_map.js

+    this.createVariable(newName, '');
+    console.log('Tried to rename an non-existent variable.');
+  }
+  else if (variableIndex == newVariableIndex ||


Nit: Move the else if onto the previous line

rachel-fenichel · 2017-05-03T21:35:15Z

core/variable_map.js

+                  + variable.getId() + '" which conflicts with the passed in ' +
+                  'id, "' + opt_id + '".');
+    }
+    return;


This is the case where nothing went wrong but the variable already existed, correct? If so, add a comment: "// The variable already exists and has the same id and type."

rachel-fenichel · 2017-05-03T21:57:56Z

core/variable_map.js

+    this.variableMap_[opt_type] = [variable];
+  }
+  // Else append the variable to the preexisting list.
+  else {


nit: else should share a line with the previous closing brace. You can just put the comment inside here, or else remove it.

rachel-fenichel · 2017-05-03T21:59:16Z

core/variable_map.js

+ */
+Blockly.VariableMap.prototype.deleteVariable = function(variable) {
+  var type = variable.type;
+  for (var i = 0, tempVar; tempVar = this.variableMap_[type][i]; i++) {


Store this.variableMap_[type] in a variable outside of the loop for clarity.

rachel-fenichel · 2017-05-03T21:59:57Z

core/variable_map.js

+Blockly.VariableMap.prototype.deleteVariable = function(variable) {
+  var type = variable.type;
+  for (var i = 0, tempVar; tempVar = this.variableMap_[type][i]; i++) {
+    if (Blockly.Names.equals(tempVar.name, variable.name)) {


Why not check by id? Then you don't have to depend on additional blockly code--it's just a string comparison.

rachel-fenichel · 2017-05-03T22:03:05Z

core/variable_map.js

+  var type = variable.type;
+  for (var i = 0, tempVar; tempVar = this.variableMap_[type][i]; i++) {
+    if (Blockly.Names.equals(tempVar.name, variable.name)) {
+      delete this.variableMap_[type][i];


I think you don't need to call delete--it should get handled by gc once it's removed from the list, and variableModel doesn't have any special disposal code.

rachel-fenichel · 2017-05-03T22:03:41Z

core/variable_map.js

+ * Find the variable by the given name and return it. Return null if it is not
+ *     found.
+ * @param {!string} name The name to check for.
+ * @return {?Blockly.VariableModel} the variable with the given name.


the variable with the given name, or null if it was not found.

rachel-fenichel · 2017-05-03T22:05:27Z

core/variable_map.js

+};
+
+/**
+ * Find the variable with the specified type. If type is null, return list of


"Get a list containing all of the variables of a specified type. If type is null, ..."

marisaleung · 2017-05-03T23:22:05Z

Review status: 0 of 6 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

core/variable_map.js, line 80 at r1 (raw file):