Iss27

[thehunte199]

Functioning tests and implementations for getting the number of variables at the global and local level. Note: Still need to implement them in the reporter but I will create a separate story for that.

[thehunte199]

Hey Chris, I had questioned this too and considered creating a single getVariables method that would be used at the Global and Sprites level but to do so seemed like it would tie together Sb2 and Sprites more than necessary without modifying architecture - something I didn't feel comfortable doing without consultation. Right now it maintains loose coupling between Sb2 and Sprites, but increases the amount of duplicate code by a small amount, something I felt might be a decent trade off for a quick implementation. If we can decide on an architecture change though I'd be happy to do my best to make it happen. @whereswaldon

[thehunte199]

@whereswaldon Or maybe the getVariables could be implemented in Sb2 where it is passed a JSONObject and include a conditional if that checks whether the object passed is of type stage or of type sprite and then responds accordingly. Unfortunately then I would have to modify the tests :neutral_face:

[thehunte199]

I'll be glad to refactor according to these suggestions asap. Thanks for the consideration guys :)

On Mon, Apr 10, 2017 at 9:37 PM, bclinthall notifications@github.com wrote:

@bclinthall commented on this pull request.

This will do what we need it to. Good job, James. I've added a couple suggestions for refactoring (same architecture issue Chris was commenting on).

In src/main/java/Sb2.java https://github.com/JaysGitLab/cs-5666-scatt-jacc-caj/pull/47#discussion_r110799274 :

@@ -131,5 +132,24 @@ public int getScriptCountForSprite(String spriteName) { public int[] getScriptLengthsForSprite(String spriteName) { return sprites.getScriptLengthsForSprite(spriteName); } -}

• /**
• • Gets the number of global variables within the stage object.
• • @return numVariables The number of global variables
• */
• public int getGlobalVariableCount() {

For the purposes of this comment, I will use gV to refer to the variable called globalVariables.

@thehunte199 https://github.com/thehunte199, I think we can accomplish this task with one public method and no instance variables. Currently, you have two methods for this task and gV is set up as in instance variable. We don't need to retain gV as part of the state of the Sb2 instance.

I would get rid of public int getGlobalVariableCount() and change private void countGlobalVariables() to a public method that returns an int. Then you can make gV a local variable inside of countGlobalVariables(). Does that make any sense?

In src/main/java/Sb2.java https://github.com/JaysGitLab/cs-5666-scatt-jacc-caj/pull/47#discussion_r110799338 :

@@ -131,5 +132,24 @@ public int getScriptCountForSprite(String spriteName) { public int[] getScriptLengthsForSprite(String spriteName) { return sprites.getScriptLengthsForSprite(spriteName); } -}

• /**
• • Gets the number of global variables within the stage object.
• • @return numVariables The number of global variables
• */
• public int getGlobalVariableCount() {

Consider how confusing my last comment would have been if I had kept saying globalVariables instead of gV.

In src/main/java/Sprites.java https://github.com/JaysGitLab/cs-5666-scatt-jacc-caj/pull/47#discussion_r110799572 :

• if (spriteMap.containsKey(sprite)) {
• countSpriteVariables(spriteMap.get(sprite));
• return variables;
• } else {
• throw new IOException("You should not be searching for sprites"
• • " that don't exist.");
• }
• }
• /**
• • Private method that attempts to find the number variables
• • associated with a sprite and store it as a field variable
• • associated with the sprite.
• • @param sprite the json representation of the sprite object
• • from an sb2 object
• */
• private void countSpriteVariables(JSONObject sprite) {

Same thing here. I'd change this method to public int countSpriteVariables(String spriteName), get rid of public int getSpriteVariableCount(String spriteName) and make int variables local to countSpriteVariables. It would make things much simpler.

That said, the code you've written accomplishes what we need. These are just refactoring suggestions.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/JaysGitLab/cs-5666-scatt-jacc-caj/pull/47#pullrequestreview-31997091, or mute the thread https://github.com/notifications/unsubscribe-auth/AQaIpV306TbwVnKmjRwXJ9BrNnUNjzdMks5rutlIgaJpZM4M3wyd .

[whereswaldon]

@thehunte199 Are you planning to refactor before we merge this, or no?

[whereswaldon]

@thehunte199 What's the word on this?

[thehunte199]

I'm planning to refactor it. Just have been busy with a research paper outline the past few days. I'm going to try to knock it out before I go to bed tonight.

On Thu, Apr 13, 2017 at 8:31 PM, Christopher Waldon < notifications@github.com> wrote:

@thehunte199 https://github.com/thehunte199 What's the word on this?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/JaysGitLab/cs-5666-scatt-jacc-caj/pull/47#issuecomment-294055267, or mute the thread https://github.com/notifications/unsubscribe-auth/AQaIpYIMIUErAN5Fryg1zb1xze9h0lzlks5rvr53gaJpZM4M3wyd .

[thehunte199]

Finished. Ready to be merged after review