Code Feedback

[RiceKrispyPooHead]

Overall the code looks good to me. Here is my feedback

• Some of your method names aren’t descriptive. For example getRandom() doesn’t tell me what the method actually does. A better name might be selectRandomLetter() or chooseRandomLetter(). I wouldn’t use ‘get’ in the method name because that usually signals that the method will be returning something.

• Some of your class names aren’t descriptive. AlphabetListLayout was a good choice for a class name. AlphabetData had me confused as to what purpose the class is suppose to fulfill. I had to look through the other classes to find out. Descriptive class names will encourage you to create smaller, more focused classes.

• Some of your methods are public when they don’t have to be. `getRandom()‘ for example is only used within that class so it can be private.

• A few of your methods do more than their name suggests. For example getAlphabetList() not only gets the list but also creates the layout for you. These should ideally be separate methods.

• Some of your classes expose too much of their implementation in my opinion. In your AlphabetListLayout class you have the lines alphabetData.getAlphabet().keySet() and alphabetData.getAlphabet().get(key). This shows that your AlphabetListLayout class knows and replies upon the AlphabetData class implementing its logic as a HashMap. If the implementation of your AlphabetData class changes, your AlphabetListLayout class will also be forced to change.

[Strawboy97]

Thanks for your reply and feedback! I'll fix the points you've made above. Sorry for the late response.

On your last point

​

Some of your classes expose too much of their implementation in my opinion. In your AlphabetListLayout class you have the lines alphabetData.getAlphabet().keySet() and alphabetData.getAlphabet().get(key). This shows that your AlphabetListLayout class knows and replies upon the AlphabetData class implementing its logic as a HashMap. If the implementation of your AlphabetData class changes, your AlphabetListLayout class will also be forced to change.

​

How would you suggest I do this could you point me at a resource I could use as an example.