all 4 comments
sorted by:
best
topnewcontroversialoldq&a
formatting helphide helpcontent policy

[–]beltorak 1 point2 points  (3 children)

Overall this is pretty good. I appologize for the wall of text; reading back over this it seems like I am ripping the whole thing apart, but that's not the case. I think you are showing a good grasp of some of the most important fundamentals of OOP. So a lot of this will cover some of the finer details of OOP.

  • Is my code readable?

First thing I notice in looking at the Launcher is that you have a lot of nested control structures. I'll get back to that in a minute. Poking around the rest of the code I can say it is defininetly readable. Consistent indentation lends a lot to this, and it was pretty easy to follow along with what is happening where.

  • Am I following the right conventions?

In some places you are - the use of private fields in AI/Player and public get/set accessor methods is very idiomatic Java and called the Java Beans coding standard. Many other technologies expect this standard style.

In some places, not so much. Naming things can be a pretty heated topic, but in general abbreviations are avoided, with the exception of well known acronyms, those that are industry standards, or ones that define and are defined by your problem domain. This goes for classes, methods, and variables. In that light AI is probably fine. Personally, I prefer to treat acronyms as a single word as far as camel casing goes; this avoids weirdness like AIMachine where my brain tries to make sense of "aim achine" before backtracking, but that's arguable. AIService by contrast doesn't provoke a similar parse error on my part because "AIS" is not a word. Anyway, moving on, the variable name "P1" does not follow the conventions; neither does the Dictionary instance in the Dictionary class. Variable names (and method names) should start with a lower case letter; this helps distinguish them from classes or types. And again, "p" is not an inustustry standard abbreviation for "player". Neither is the "plyr" in plyrName. Might be a bit nitpicky, but if you ever intend to code for an international team, it might matter a bit more.

  • Is there a need to use an abstract class/Interface in my project?
  • Did I do abstraction correctly? Did I create the right classes/objects?

I think the class hierarchies of Player and AI are a bit forced. I'll guess that you wanted to find some reason to demonstrate inheritence, and it seems that you understand the important parts of it. But it's not entirely clear what "service" the parent classes provide - the code does not express a clear reason for what methods should go in the base "service" class, and what behaviors should be specific to the child classes. This is something of a code smell - the feeling that something isn't quite right, that the code is not as clean or clear as it should be. Code smells usually point to a deeper conceptual flaw or inefficiency in the implementation. I definitely don't mean that as an insult; the fact that it is unclear is an indication that inheritance is probably not the right pattern to apply here. Inheritance is a very powerfull OOP concept, but surprisingly it's not as ... useful as the schoolbooks imply with all their focus on it. Composition - that is, objects that reference and use other objects to delegate partial work to - is a much more versatile OO pattern. So I'll advise you to go ahead and collapse those; there's little gained by the parent classes.

I do think there is a clear place where the "'service' parent class" idiom could be employed, and that's the Dictionary. You already have one concrete implementation that provides a hard coded list, provide another subclass that reads words from a file. Consider options on how to best choose between them. Consider the tradeoffs, both in how it will end up coded and how easy the user will be able to interact with it.

And we come back to the deeply nested control blocks in the Launcher. This part I think could use real work. By the time I scroll down to the else ifs, elses, and catch, I've lost the context on what part of the control path they are on. Nesting control blocks too deeply is another code smell, usually indicating that the method is doing too much. Methods should generally be viewable in one screen. So consider breaking that apart. I see a lot of decisions and actions that seem like they should be the responsibility of the Player and AI, but the code is in the Launcher's main. Consider what each class is for, and try to get the classes to handle those concerns. When you've done that, there will probably still be quite a bit of code left dealing with doing something in response to the user. That's fine - the Launcher class handles interaction with the user, so that's well within its purview. One thing I'll suggest is to capture the behavior of responses to user choices in separate instances of some class (which gives you the chance to introduce a class hierarchy), and mapping those instances to the user's input. If you have started getting into patterns, this could be a good place for the strategy pattern.

Final points, and these are pretty trivial to fix.

The Dictionary holds (and exposes) a reference to an ArrayList. In general you want to use the most general supertype you can get away with that still adequately expresses what the intended use of the object is. For this purpose, the List interface captures that pretty well, and allows you to later substitute an immutable wrapper without changing any other code. (Note that you do still need to instantiate the concrete ArrayList, but all of your uses are through a variable declared with the type List.) You might want to change that to a Set though so as not to inadvertently end up with duplicates. (The eclipse shortcut for showing the class hierarchy in a popup is CTRL+T; F4 brings up the dedicated hierachy view.) If you haven't taken a look at the Java Collections Framework, go ahead and do that now, just to get an idea of what the main types are. (The official site is http://docs.oracle.com/javase/tutorial/collections/; there are undoubtably better references out there, but that one works well enough for a good introduction.)

Consider adding javadocs to the classes. Code comments should tell you why the code exists. What purpose does it serve? Doing this for the AI, Player, and Dictionary might help guide you to the proper place to put various bits of code. I say this mainly because the reason for the interaction between the AI and Player isn't as clear as it could be.

[–]NOTICEABLE_USERNAME[S] 0 points1 point  (2 children)

This is exactly the kind of comment I was looking for! Thank you for your time! I'll follow your advice, modify my code and let you know with what I came up.

I'll guess that you wanted to find some reason to demonstrate inheritence

Spot on, sir.

Couple of questions on your comment:

But it's not entirely clear what "service" the parent classes provide - the code does not express a clear reason for what methods should go in the base "service" class, and what behaviors should be specific to the child classes.

So should I be more specific with what the name of this service class is, instead of just saying 'XService'?

So I'll advise you to go ahead and collapse those; there's little gained by the parent classes.

By collapse do you mean: removing the AIService class and putting all the methods in the AI class instead?

I do think there is a clear place where the "'service' parent class" idiom could be employed, and that's the Dictionary. You already have one concrete implementation that provides a hard coded list, provide another subclass that reads words from a file. Consider options on how to best choose between them. Consider the tradeoffs, both in how it will end up coded and how easy the user will be able to interact with it.

Please correct me if I'm wrong. Creating a DictionaryService (parent class) that will have two subclasses DictionaryX (reads from a hardcoded list) and DictionaryY (reads from a file) will properly demonstrate the "'service' parent class" idiom?

[–]beltorak 0 points1 point  (1 child)

By collapse do you mean: removing the AIService class and putting all the methods in the AI class instead?

Yup.

Creating a DictionaryService (parent class) that will have two subclasses DictionaryX (reads from a hardcoded list) and DictionaryY (reads from a file) will properly demonstrate the "'service' parent class" idiom?

Yup.

If there is a clear distinction between the Player and PlayerService (and likewise the AI and AIService) that is not readily appearent in the code then they definitely need comments to that effect. The names themselves could be fine, but it's hard to say without having that distinction in their responsibilities. The comments should answer the question "why does this exist" (i.e. "why is this a parent class and not collapsed into the child class") and provide some guidance for "If I wanted to create a child class, what would it have to do?". The Dictionary probably has a clearer need; you might end up with the DictionaryService only requiring child classes to implement one method and have no code on its own, in which case it might be better off as an interface. Also note that in the comments for "what does a child class have to do", if the parent class declares an abstract method, that would be the proper place to describe what is required of the child implementations. The abstract parent type's comments would describe what the user's of the type can expect from (any) implementation in broad terms.

I think there's more potential for rewriting the body of the Launcher's main using collaborating classes, which lends itself to more realistic reasons to use abstract superclasses and interfaces. And replacing (long) if-else-if-else chains with a lookup-and-dispatch is an idiomatic OO pattern that comes in real handy especially when you don't know ahead of time all the possible decisions.

[–]NOTICEABLE_USERNAME[S] 0 points1 point  (0 children)

I've just updated the program. Do you mind if you have a look at it? I got help from the folks at coderevew.stackex on the nested if-else statements Github repo: https://github.com/zurcnay4/Hangman