Deeplocal / android-things-handbot

Android Things HandBot
MIT License
2 stars 3 forks source link

Syntax nits #5

Open proppy opened 6 years ago

proppy commented 6 years ago

Consider renaming class member to mMember: https://github.com/Deeplocal/android-things-handbot/blob/master/app/src/main/java/com/example/sewl/androidthingssample/Classifier.java#L16

Move hardcoded values to top level constants: https://github.com/Deeplocal/android-things-handbot/blob/master/app/src/main/java/com/example/sewl/androidthingssample/FingerController.java#L61 https://github.com/Deeplocal/android-things-handbot/blob/master/app/src/main/java/com/example/sewl/androidthingssample/ImageClassifierActivity.java#L135 https://github.com/Deeplocal/android-things-handbot/blob/master/app/src/main/java/com/example/sewl/androidthingssample/LightRingControl.java#L134

Use writeToLEDStrips helper methods consistently or remove it: https://github.com/Deeplocal/android-things-handbot/blob/master/app/src/main/java/com/example/sewl/androidthingssample/LightRingControl.java#L125 https://github.com/Deeplocal/android-things-handbot/blob/master/app/src/main/java/com/example/sewl/androidthingssample/LightRingControl.java#L96 https://github.com/Deeplocal/android-things-handbot/blob/master/app/src/main/java/com/example/sewl/androidthingssample/LightRingControl.java#L77

Move pure methods and calculated constants as static members: https://github.com/Deeplocal/android-things-handbot/blob/master/app/src/main/java/com/example/sewl/androidthingssample/LightRingControl.java#L133 https://github.com/Deeplocal/android-things-handbot/blob/master/app/src/main/java/com/example/sewl/androidthingssample/LightRingControl.java#L86 https://github.com/Deeplocal/android-things-handbot/blob/master/app/src/main/java/com/example/sewl/androidthingssample/LightRingControl.java#L183 https://github.com/Deeplocal/android-things-handbot/blob/master/app/src/main/java/com/example/sewl/androidthingssample/RockPaperScissors.java#L126

Factor common functionality in helpers methods: https://github.com/Deeplocal/android-things-handbot/blob/master/app/src/main/java/com/example/sewl/androidthingssample/LightRingControl.java#L111 https://github.com/Deeplocal/android-things-handbot/blob/master/app/src/main/java/com/example/sewl/androidthingssample/LightRingControl.java#L220 https://github.com/Deeplocal/android-things-handbot/blob/master/app/src/main/java/com/example/sewl/androidthingssample/RockPaperScissors.java#L195 (for setting wait state transition + wait time)

Use CamelCase for Enums: https://github.com/Deeplocal/android-things-handbot/blob/master/app/src/main/java/com/example/sewl/androidthingssample/SimonSays.java#L67 https://github.com/Deeplocal/android-things-handbot/blob/master/app/src/main/java/com/example/sewl/androidthingssample/StandbyController.java#L45

Consider using resource directly instead of mapping them to constants: https://github.com/Deeplocal/android-things-handbot/blob/master/app/src/main/java/com/example/sewl/androidthingssample/SoundController.java#L12

Fix indentation: https://github.com/Deeplocal/android-things-handbot/blob/master/app/src/main/java/com/example/sewl/androidthingssample/TensorflowImageOperations.java#L54

sewl-dl commented 6 years ago

Hey @proppy -- I'm not sure I follow this one:

https://github.com/Deeplocal/android-things-handbot/blob/master/app/src/main/java/com/example/sewl/androidthingssample/RockPaperScissors.java#L195

Could you give me an example of the refactor you'd like to see?

sewl-dl commented 6 years ago

@proppy -- for the direct resource mapping in https://github.com/Deeplocal/android-things-handbot/blob/master/app/src/main/java/com/example/sewl/androidthingssample/SoundController.java#L12, I like the idea (getting rid of a bunch of code always feels great), but I do like that the hard typing protects you from passing the wrong kinda of resource.

Having said that, I'm not super attached to it. If you'd rather it a direct resource map, I'm game.

proppy commented 6 years ago

https://github.com/Deeplocal/android-things-handbot/blob/master/app/src/main/java/com/example/sewl/androidthingssample/RockPaperScissors.java#L195 Could you give me an example of the refactor you'd like to see?

It seems that every time you switch to a different state, you set a transition time, maybe there could be a setCurrentState method that take an optional transition time as an argument.

@proppy -- for the direct resource mapping in https://github.com/Deeplocal/android-things-handbot/blob/master/app/src/main/java/com/example/sewl/androidthingssample/SoundController.java#L12, I like the idea (getting rid of a bunch of code always feels great), but I do like that the hard typing protects you from passing the wrong kinda of resource.

But I think the compile should fail if you point to a non existing resource.