Refactor NameManager into a map-like class

[AnmAtAnm]

Names in blockly refer to things which now have Info objects that store their metadata. Making NameManager a map-like class consolidates the name management with the associated variable or procedure data. This solved a few bugs and cleaned up lots of code.

Next step, solve procedure removal.

---

This change is 

[RoboErikG]

Review status: 0 of 17 files reviewed at latest revision, 11 unresolved discussions.

---

blocklylib-core/src/main/java/com/google/blockly/android/control/NameManager.java, line 114 at r1 (raw file):

     * @param name The name to add.
     * @param value The value to associate with the name.
     * @throws IllegalArgumentException If the name is not valid.

javadoc @return

---

blocklylib-core/src/main/java/com/google/blockly/android/control/NameManager.java, line 136 at r1 (raw file):

     * @param nameRequested The requested name to use.
     * @param value The value to associate with the name.
     * @throws IllegalArgumentException If the nameRequested is not valid.

@return

---

blocklylib-core/src/main/java/com/google/blockly/android/control/NameManager.java, line 205 at r1 (raw file):

    public String getExisting(String proposedName) {
        NameEntry<T> existingEntry = mCanonicalMap.get(makeCanonical(proposedName));
        return existingEntry == null ? proposedName : existingEntry.mDisplayName;

s/proposedName/null

---

blocklylib-core/src/main/java/com/google/blockly/android/control/VariableNameManager.java, line 1 at r1 (raw file):

package com.google.blockly.android.control;

copyright header

---

blocklylib-core/src/main/java/com/google/blockly/android/control/VariableNameManager.java, line 12 at r1 (raw file):

// This abstract class is required because we really want to store VariableInfoImpl objects
// inside the WorkspaceStats (with things like field and procedure counts).
public abstract class VariableNameManager<VI extends VariableInfo> extends NameManager<VI> {

It looks like the Impl version only depends on getting direct access to the list of procedure names for adding and removing procedures on a VariableInfo. Why not just add the addProcedure and removeProcedure methods to the VariableInfo interface?

---

blocklylib-core/src/main/java/com/google/blockly/android/control/VariableNameManager.java, line 79 at r1 (raw file):

    }

    public void removeProcedureArg(String varName, String procedureName) {

javadoc

---

blocklylib-core/src/main/java/com/google/blockly/android/control/WorkspaceStats.java, line 444 at r1 (raw file):

        @Override
        protected void markVariableAsProcedureArg(VariableInfoImpl varInfo, String procedureArg) {

s/procedureArg/procedureName

---

blocklylib-core/src/main/java/com/google/blockly/android/control/WorkspaceStats.java, line 445 at r1 (raw file):

        @Override
        protected void markVariableAsProcedureArg(VariableInfoImpl varInfo, String procedureArg) {
            if (varInfo.mProcedures == null) {

Why does the name manager need to know how the variable info adds and removes procedure names?

---

blocklylib-core/src/main/java/com/google/blockly/android/control/WorkspaceStats.java, line 452 at r1 (raw file):

        @Override
        protected void unmarkVariableAsProcedureArg(VariableInfoImpl varInfo, String procedureArg) {

s/procedureArg/procedureName

---

blocklytest/src/androidTest/java/com/google/blockly/android/control/VariableNameManagerTestImpl.java, line 1 at r1 (raw file):

package com.google.blockly.android.control;

copyright

---

blocklytest/src/androidTest/java/com/google/blockly/android/control/VariableNameManagerTestImpl.java, line 26 at r1 (raw file):

    @Override
    protected void unmarkVariableAsProcedureArg(VariableInfoImpl varInfo, String procedureArg) {
        varInfo.mProcedureNames.add(procedureArg);

s/add/remove

---

Comments from Reviewable

[AnmAtAnm]

Review status: 0 of 19 files reviewed at latest revision, 11 unresolved discussions, some commit checks broke.

---

blocklylib-core/src/main/java/com/google/blockly/android/control/NameManager.java, line 114 at r1 (raw file):

Previously, RoboErikG wrote…

javadoc @return

Done.

---

blocklylib-core/src/main/java/com/google/blockly/android/control/NameManager.java, line 136 at r1 (raw file):

Previously, RoboErikG wrote…

@return

Done.

---

blocklylib-core/src/main/java/com/google/blockly/android/control/NameManager.java, line 205 at r1 (raw file):

Previously, RoboErikG wrote…

s/proposedName/null

Done.

---

blocklylib-core/src/main/java/com/google/blockly/android/control/VariableNameManager.java, line 1 at r1 (raw file):

Previously, RoboErikG wrote…

copyright header

Done.

---

blocklylib-core/src/main/java/com/google/blockly/android/control/VariableNameManager.java, line 12 at r1 (raw file):

Previously, RoboErikG wrote…

It looks like the Impl version only depends on getting direct access to the list of procedure names for adding and removing procedures on a VariableInfo. Why not just add the addProcedure and removeProcedure methods to the VariableInfo interface?

And creating new instances, so your suggestion doesn't fully get rid of this class. Still, an improvement.

---

blocklylib-core/src/main/java/com/google/blockly/android/control/VariableNameManager.java, line 79 at r1 (raw file):

Previously, RoboErikG wrote…

javadoc

Done.

---

blocklylib-core/src/main/java/com/google/blockly/android/control/WorkspaceStats.java, line 444 at r1 (raw file):

Previously, RoboErikG wrote…

s/procedureArg/procedureName

Removed method, per above.

---

blocklylib-core/src/main/java/com/google/blockly/android/control/WorkspaceStats.java, line 445 at r1 (raw file):

Previously, RoboErikG wrote…

Why does the name manager need to know how the variable info adds and removes procedure names?

Fixed, per above comment.

---

blocklylib-core/src/main/java/com/google/blockly/android/control/WorkspaceStats.java, line 452 at r1 (raw file):

Previously, RoboErikG wrote…

s/procedureArg/procedureName

Removed method, per suggestion above.

---

blocklytest/src/androidTest/java/com/google/blockly/android/control/VariableNameManagerTestImpl.java, line 1 at r1 (raw file):

Previously, RoboErikG wrote…

copyright

Done, here and elsewhere.

---

blocklytest/src/androidTest/java/com/google/blockly/android/control/VariableNameManagerTestImpl.java, line 26 at r1 (raw file):

Previously, RoboErikG wrote…

s/add/remove

Removed method

---

Comments from Reviewable

[RoboErikG]

Mostly nits. Feel free to submit once fixed.

---

Review status: 0 of 19 files reviewed at latest revision, 17 unresolved discussions, some commit checks broke.

---

blocklylib-core/src/main/java/com/google/blockly/android/control/NameManager.java, line 137 at r2 (raw file):

     * @param nameRequested The requested name to use.
     * @param value The value to associate with the name.
     * @return The final name used to store V.

s/V/the value/

---

blocklylib-core/src/main/java/com/google/blockly/android/control/VariableNameManager.java, line 92 at r2 (raw file):

    /**
     * Removed any association between the variable {@code argName} with {@code procedureName}. This

s/Removed/Removes/ s/argName/varName/

---

blocklylib-core/src/main/java/com/google/blockly/android/control/VariableNameManager.java, line 93 at r2 (raw file):

    /**
     * Removed any association between the variable {@code argName} with {@code procedureName}. This
     * does not remove the variable, if its role as a procedure argument was the last know use.

nit, empty line between description and params? It looks like we've been really inconsistent in our formatting here. What would you like to be the standard?

---

blocklylib-core/src/main/java/com/google/blockly/android/control/VariableNameManager.java, line 93 at r2 (raw file):

    /**
     * Removed any association between the variable {@code argName} with {@code procedureName}. This
     * does not remove the variable, if its role as a procedure argument was the last know use.

nit, no comma after variable.

---

blocklylib-core/src/main/java/com/google/blockly/android/control/VariableNameManager.java, line 94 at r2 (raw file):

     * Removed any association between the variable {@code argName} with {@code procedureName}. This
     * does not remove the variable, if its role as a procedure argument was the last know use.
     * @param varName The name of the variable that was once a procedure argument.

"The name of the variable that was being referenced by the procedure" "The name of the procedure that is no longer referencing the variable."

---

blocklylib-core/src/main/java/com/google/blockly/android/control/VariableNameManager.java, line 106 at r2 (raw file):

    /**
     * Constructs a new VariableInfo object with the given name
     * @param varName The name fo the variable to create.

s/fo/of

---

Comments from Reviewable

[AnmAtAnm]

Review status: 0 of 19 files reviewed at latest revision, 17 unresolved discussions, all commit checks successful.

---

blocklylib-core/src/main/java/com/google/blockly/android/control/NameManager.java, line 137 at r2 (raw file):

Previously, RoboErikG wrote…

s/V/the value/

Done.

---

blocklylib-core/src/main/java/com/google/blockly/android/control/VariableNameManager.java, line 92 at r2 (raw file):

Previously, RoboErikG wrote…

s/Removed/Removes/ s/argName/varName/

Keeping argName, because we are declaring the variable to be an argument with this function.

---

blocklylib-core/src/main/java/com/google/blockly/android/control/VariableNameManager.java, line 93 at r2 (raw file):

Previously, RoboErikG wrote…

nit, empty line between description and params? It looks like we've been really inconsistent in our formatting here. What would you like to be the standard?

I don't feel a strong need to choose one over the other. In longer doc comments, the readability improvements are valuable. In very short comments, I feel the gains are limited. If a function can be clearly described in a sentence, then the spacing increase of another line is proportionally significant (e.g., sentence + blank + param = 150% * (sentence + param)). Neither improve the generated documentation.

---

blocklylib-core/src/main/java/com/google/blockly/android/control/VariableNameManager.java, line 93 at r2 (raw file):

Previously, RoboErikG wrote…

nit, no comma after variable.

Done.

---

blocklylib-core/src/main/java/com/google/blockly/android/control/VariableNameManager.java, line 94 at r2 (raw file):

Previously, RoboErikG wrote…

"The name of the variable that was being referenced by the procedure" "The name of the procedure that is no longer referencing the variable."

Done.

---

blocklylib-core/src/main/java/com/google/blockly/android/control/VariableNameManager.java, line 106 at r2 (raw file):

Previously, RoboErikG wrote…

s/fo/of

Done.

---

Comments from Reviewable

[RoboErikG]

Review status: 0 of 19 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

---

blocklylib-core/src/main/java/com/google/blockly/android/control/VariableNameManager.java, line 92 at r2 (raw file):

Previously, AnmAtAnm (Andrew n marshall) wrote…

Keeping argName, because we are declaring the variable to be an argument with this function.

I read this as referring to the parameter, which still says varName. Without other context in this file 'argName' doesn't make sense. Did you mean to change the parameter name?

---

Comments from Reviewable