COMP229: Assignment 1 Feedback COMP229 Object Oriented Programming Practices Assignment 1 Feedback On the whole, the overall performance in assignment 1 was really good, with 62 out of 77 submissions getting more than 50% of the available marks, 43 out of 77 submissions getting more than 65% of the available marks and a pleasing 33 out of 77 submissions getting a distinction grade (75%) or better. This was a fairly challenging exercise in Java as well as getting to grips with design patterns, especially if you took it upon yourself to write your code from scratch, so you should all be proud of yourselves. On the whole, the vast majority of the solutions submitted were functional and well written. Where much improvement can be made however is in documenting your work in an accompanying report -- I've suggested ways that reports can be improved significantly please do have a look ready for Assignment 2! As a rule of thumb you should have spent about 30% of the available time (about 7 hours) on documentation and reflecting on what you have learned. In marks terms, it would have been better to leave the application in an incomplete state, losing a mark or so for that part of the assessment. To compensate you could then have gained many more marks by documenting what you had achieved to that point and by writing good tests of any functionality you had managed to implement. Now to the detail. In the following sections, we review some of the most common observations we made while marking your submissions - and a few things we thought we'd throw in for luck: Magic Numbers 2013 In programming the phrase "magic number" generally refers to the bad practice of using explicit numbers directly in your source code without an accompanying explanation of purpose. In general, the use of magic numbers is strongly discouraged. To avoid them it is usually suggested that all such numbers should be replaced by specially declared constants (static and final class variables) whose names should clearly indicate their purpose. Such a constant should then be accompanied by a description of its purpose, in the form of a (JavaDoc) comment, at the point where it is declared. So why is it bad practice to use magic numbers in your code? The answer here is that they generally tend to present us with a number of substantial, and potentially disastrous, code maintenance problems, some of which we mention here: When you originally wrote your code you knew what each magic number did, but a few months later you've forgotten and as a result you are no longer entirely clear what your code was intended to do... ...to fix this you have to spend many hours pouring through your code to work out what each of its magic numbers does. More importantly, in a commercial environment it is very likely that some other programmer will need to work with / maintain / update your code - they certainly won't know what your magic numbers do. Suppose you wanted to change your code in a way that requires updating your magic numbers, how can you be sure that you've updated every instance of a given magic number to implement your change... ...Say you wanted to make your Nim game have more than three piles. Now some instances of the number 3 will refer to the number of piles and others will refer to other quantities which just happen to be 3. So how can you be sure that you've changed all of the 3s which refer to the number of piles and haven't changed any of those that refer to other quantities? Overall, code containing magic numbers is about as easy to read, and thus to understand and debug, as a text written in ancient babylonian - we can certainly recognise individual words (or constant instances) but our understanding of their real meaning is limited by a hazy understanding of the context in which they were originally used. Using declared constants instead of magic numbers circumvents all of these problems. Firstly it accurately documents each use of a given constant value, allowing us to refer back from each point of use of that constant to the associated comment at its point of declaration. Secondly, it allows us to update each use of that constant by making a single change to its defined value, in a way that is guaranteed not to disrupt the values of constants used for other purposes. As an example, here is a piece of code from one of our samples written using magic numbers
static void reset() {
matchesOnPile= 9;
currentPlayer= 0;
}
The intention of the reset is fairly clear, but not very good for maintaining the code in the event that we want to vary the number of players, the number of piles or even the number of matches on the piles. We'd have to search through the whole code looking for places referring to initialisation kinds of code. Our original code, however, used carefully named constants instead of magic numbers and for it the answers to these questions are immediately clear, making this code much easier to understand and maintain:
static void reset() {
matchesOnPile= STARTING_CONFIGURATION;
currentPlayer= STARTING_PLAYER;
}
Indeed if we refer back to the comments at the declaration sites of these constants any remaining questions are immediately answered in their comments:
// Game state.
static int matchesOnPile; // The number of matches on the pile
static Player currentPlayer; // The current player in turn
public enum Player { Black, White } // The names of the players
static Player STARTING_PLAYER= Player.White; // The player who always starts the game
static int STARTING_CONFIGURATION= 9; // The initial number of matches on the pile
where we see in fact that the player is an enumerated type rather than a 0 or 1, making it much more readable as a player rather than some other constant. Equality (==) vs. equals() One problem that a few people seemed to run into was a little confusion about what expressions like
(vLetter == "A")
do. Indeed, in a couple of cases this misunderstanding prevented individuals from making any substantial progress on the assignment. Remember that in Java any variable that you use to access an object of some class is treated as a reference (a kind of pointer). So when we do something like
(vVariable1 == vVariable2)
this code actually checks to see that each one of these variables is literally pointing to exactly the same object. The same goes for the test of the letter variable above, in this case it checks to see if the String object pointed to by vLetter is exactly the same as the literal string object "A". Unfortunately, in many cases this is far too strong a notion of equality - especially if the string in vLetter was, for instance, read in as a line of input from the keyboard (as it was in some instances) using code like:
BufferedReader mBufferedIn =
new BufferedReader(new InputStreamReader(System.in));
String vLetter = mBufferedIn.readLine();
In this case, each call to the readLine() method creates an entirely new object of the String class whereas "A" is a constant string object that stays the same throughout the execution of your program. So it will never be the case that the object pointed to by vLetter and the constant "A" are literally the same object. What was really needed here was a check that the string pointed to by mLetter and the constant string "A" both contain the same sequence of letters, which can be done using the code:
vLetter.equals("A")
Commenting 2013 As discussed in the lecture on JavaDoc, it is generally best not to comment individual lines of code - instead you should generally provide more detailed comments only at the top of each method. Indeed, in the Extreme Programming community line-by-line commenting is referred to as "code deodorant", since their view is that such comments are used to cover up "code smells". In other words, in their view comments tend to be applied to poorly written code because it is hard to understand on its own. Their preference is to rewrite the original "smelly" code, thereby improving the application itself, in preference to covering it up with hastily applied comments. Furthermore, as discussed in the lecture on JavaDoc, a major problem with in code commenting is that such comments tend not to be updated when the code to which they are attached is changed. Over time we end up with code and comments which are out of sync, making the comments themselves worse than useless. Finally, it is quite often the case that line-by-line comments are completely redundant. For instance, it is usually possible to see that a loop counts from 1 to 15 without having to re-iterate that fact in an associated comment. Overall, it is best to limit your comments to a single JavaDoc comment at the top of each method. This should describe the purpose of the method, the nature of its inputs and outputs, any important pre-conditions or post-conditions which apply to it, any side effects it might have, a brief description of the algorithm it implements and links to any external documents which might help the reader understand what is going on. Tip: when using Javadoc comments it is worth noting that the Javadoc tool doesn't respect plain text formatting. So if you've carefully laid out the text of your comments in columns, to read well in your code, when you convert them to HTML help pages all of this lovely text will end up getting run into a single paragraph of unbroken gobbledegook. So to ensure that your text ends up nicely formatted in your HTML docs you should use HTML markup tags in your Javadoc comments - examples of which can be seen in the comments in the sample code. If you want to separate things into paragraphs surround them in
tags and if you want to display lists of information surround the whole list in
tags and surround each list item with
tags. You can find out more about how to use HTML tags to format your comments in any HTML tutorial such as http://www.w3schools.com/html/. Intentional Coding In the place of in code comments you should try instead to write simple clear "intentional" code whose meaning is easily determined without needing to consult line-by-line comments. If you absolutely feel you need to explain something within a method, then you might want to consider rewriting your code at that point to make it more intentional. If that isn't possible then it is probably an indication that the method you are working on is too complex and that some aspects of its implementation could reasonably be peeled off into a method of its own equipped with its own explanatory JavaDoc comment. Here is a brief list of tips you might like to research and apply: Use symbolic constants rather than magic numbers (see above). Keep your methods small, contained and restricted to a specific task - then give them a name which accurately reflects their purpose. Adopt a variable naming convention which allows you to easily determine one kind of variable from another - local variables, class variables, instance variables, parameters and so forth. Use longer variable names which describe the purpose that variable is being put to. They should consist of 2 or 3 descriptive words in "camel case" and starting with a lower case letter. Never cut corners and use names such as temp or stuff, regardless of how briefly a variable is being used. Carefully design the formatting of your code - splitting things like complex conditions across a number of lines in a way which makes their meaning easier to parse. Avoid excessive use of inheritance - it is a powerful tool but excessive use can make things complicated. In general, aggregation and association are more generally applicable tools. Take a "thin GUI" approach to your code. Your GUI classes should only setup the GUI and translate GUI actions to calls on methods of some business logic class. Never mix GUI and logic code in the same class. Look out for situations in which you had to resort to "shotgun surgery", peppering your code with loosely connected changes in order to fix a bug or add a new feature. This probably means that code which should be sitting together in a single class is actually scattered throughout your application. Avoid deeply nested if...then...else constructs. These are hard to understand and could often be replaced by early method returns. Keep an eye out for classes which are getting too large, code heavy and busy. Split such things up into a family of associated but more specific classes. On the other hand also keep an eye out for classes which are too lazy and implement very little useful functionality. Consider merging them with other classes whose functionality is closely related. Always avoid "inappropriate intimacy" where one class knows too much about the internals of another. To resolve such intimacy problems consider re-organising how your classes divide up the task at hand amongst themselves. Don't just introduce getters and setters and assume the job is done. Naming Conventions and Capitalisation In places the task of reading your code was complicated by strange or inconsistent naming conventions. We recommend the following conventions, and will insist on them when marking assignment 2: Variables should have descriptive names consisting of 2 or 3 words. The words within a variable name should be distinguished using the "camel case" convention - that is lower case except for the capitalisation of the first letter of each word within the variable name. Variables should always start with a lower case letter denoting the kind of variable it is - v for local variable, p for parameter, m for member (class or instance), e for exception object etc. Class names should always begin with a capital letter and should also have descriptive names consisting of 2 or 3 words distinguished by camel case. Method names should also be descriptive, but possibly with more words also distinguished by the camel case convention. Method names should all start with lower case letters. This distinguishes them from constructors, which have the same name as their classes and so, will start with a capital letter. It ensures that there is no way that method calls can get confused with constructor calls (using the new keyword). Constants (static final class members) should follow their own convention. Again these should have descriptive names, but each letter of a constant should be capitalised and its constituent words should be separated by underscores (as in the constant name BOARD_SIZE). Finally, all package names should be given in lower case. While on the topic of packages, you should make sure that every class you write is contained within a definite package. There is a thing called the default package, which contains those classes which are not housed in a specific package - but the Java designers (and the Eclipse IDE) STRONGLY discourages you from putting classes in the default package.... Repeated Code Chunks Duplicated code is a major hazard. If you duplicate a piece of code in many places, then each time you try to change the code in one of those places you'll end up having to make changes in all of them. It is hard to make sure that multiple changes are applied consistently and easy to miss a place where such a change should be applied. Applications that rely on lots of code duplication often require "shotgun surgery", wherein bugs can only be fixed (or new features added) by peppering your code with "buckshot" made up of many similar (but rarely identical) patches. This is a maintenance nightmare. So code copy-and-paste is not your friend in such instances. For example, many people made seven duplicates of the code for adding action listeners to their play buttons. This works, but what happens later on when we want to change how those buttons behave? We'll have to make seven almost identical, but separate, code changes and make sure that each copy of the code works properly. Instead it might have been better to write a single method which contained the common repeated code and then to call that method from each of the places that originally had a copy of that code. In the specific case of action listeners, it would have made sense to create a separate class that implemented the ActionListener interface and contained the common action listener implementation. Then we could create one object of this class for each play button on the GUI. In the case of the sample application, we've actually created a new class called ColumnButton which extends JButton and implements the ActionListener interface to act as its own action listener. Separation of Powers - Logic and GUI It is important to keep application logic and GUI code as separate as possible. By doing this we move the complex logic code into classes of its own, which can then be tested to death. It also means that we can re-use that logic code without alteration in a number of applications with quite different GUIs. In general, you should try to implement a "thin GUI" approach to application programming, making your GUI code as simple as possible. You'll notice, for instance, in our sample application that each button action listener does no more than make a single method call to the associated GameModel object - which then does all of the work. GUI code is very hard to test automatically, so making it as simple as possible means that it will need relatively little testing. However, if logic code is integrated into the GUI itself then it is almost impossible to test effectively - or at the very least the writing of that test code becomes very much more complicated. So have a look at your game logic classes and make sure that they do not contain any user interface code of any kind (GUI or console). Conversely make sure that you GUI does no more than make simple, single calls to your game logic objects on response to single user interactions. If you discover that either of these rules is violated then you should think about refactoring your code to disentangle these two aspects of your application. Our reason for asking you to write both a GUI and a text interface based version of this application is that we hoped this would encourage you to move all of your logic to the GameModel class. Then you can easily re-use that same class for both versions of the application. We also asked you to use the Observer pattern in your work for the same reason. A few people ended up making 2 copies of their GameModel class, one for the text application and one for the GUI. This is not a good idea, as it means that if you change the code in one of these classes you will probably have to change it in the other - twice the work and twice the chance that something will go wrong (see my comments on code duplication above). Don't Clobber Your Exceptions Just a quick point about exception handling. When catching exceptions, the types of the exceptions in your catch blocks should be as specific as possible. For example, if you are expecting that some kind of IOException might occur you can certainly catch it with the following try...catch code:
try {
// IO code
} catch (Exception eCaught) {
// handle any exception
}
However, this code will catch all exceptions, not just the IO ones, and will try to handle them all identically. This will probably not be possible, unless your intent is just to ignore (or "clobber") all exceptions. But clobbering all exceptions is a very bad idea - because then we will never find out when exceptions we didn't expect to happen actually do arise - probably as a result of a bug in our code. So we should never catch exceptions using an exception type as general as Exception, instead we should be more specific:
try {
// IO code
} catch (IOException eCaught) {
// handle any IO exception
}
Now we only catch IO exceptions, so we know more about the exception we are handling and will probably have more success in doing so. All other exceptions will propagate further out to be handled by handlers for other kinds of non-IO exception - so we don't need to worry about them here. Indeed it might be better to be even more specific still:
try {
// IO code
} catch (FileNotFoundException eCaught) {
// handle file not found exceptions...
} catch (IOException eCaught) {
// .. and handle all other IO ecxceptions
}
Using switch Statements In some cases the use of switch statements is discouraged, since they may substitute for a more appropriate use of the class and inheritance mechanisms of Java (more about this when we talk about design patterns). However, they are always to be preferred to nasty sequences of if...then...else statements. For example, the following code is hard to read, hard to debug and actually quite slow to execute:
private static int convertStringToInt(String pLetter)
{
int vNumberToReturn = 0;
if(pLetter.equals("A"))
{
vNumberToReturn = 1;
return vNumberToReturn;
}
if(pLetter.equals("B"))
{
vNumberToReturn = 2;
return vNumberToReturn;
}
if(pLetter.equals("C"))
{
vNumberToReturn = 3;
return vNumberToReturn;
}
//... more code follows
}
This code is a little inelegant and is also quite brittle, since it doesn't handle lower case input, leading spaces, trailing garbage and so forth. It probably would have been better to use a switch statement, although they only apply to characters and integers so we will need to pre-process our input a little:
private static int convertStringToInt(String pLetter)
{
switch (pLetter.trim().toUpperCase().charAt(0)) {
case 'A': return 1;
case 'B': return 2;
//.... etc etc.
default: return 0;
}
}
Indeed in this particular case we could have made things even simpler, using the fact that the char type is actually a kind of integer type on which we can do arithmetic and the fact that letters are numbered consecutively:
private static int convertStringToInt(String pLetter)
{
int vLetterAsInt = pLetter.trim().toUpperCase().charAt(0);
if (vLetterAsInt < 'A' || vLetterAsInt > 'G')
return 0;
else
return vLetterAsInt - 'A';
}
Testing Tips Actually, most of you who spent time on writing tests did a very good job. We will be looking at testing again in upcoming lectures, but in the meantime here are a few tips to help you in deciding what tests to write. Every method of the class you are testing should have at least one test method devoted to testing it. Each test method should test only one aspect of the behaviour of the method you are testing ... that way if the test fails we have a better chance of working out why and correcting the fault. Start by testing the simplest possible things you can think of - initialisation of new objects by various constructors, single update operations, very simple cases of calculations and so forth - then build from that to more complex tests. Think carefully about "boundary cases" - that is things that occur away from the "run-of-the-mill" operation of your code. For example, test to make sure that various actions, such as win detection, still work properly if they involve locations which occur at the edges of the board. Make sure that overflows (of columns or the whole board) are correctly dealt with. Test to make sure that you can't undo when no pieces have been played and that you cannot redo unless you have previously executed an undo. Discuss your testing with others, they will often come up with test ideas you would have never thought of because you are too intimately involved with your own code. Put about as much effort into your tests as into your code - they will repay you handsomely in the long term. Write your tests as you write your code. You don't have to write them first as I did, but certainly don't leave them all to the end as an afterthought. Report Comments Overall, most reports documented what was implemnted quite well. Where there could be improvements is on reflecting on what you have done, for example by dwelling on parts of the design that you think were difficult, or where you discovered a particularly cunning or tidy solution to a design or coding problem. Here are a few overall comments worth bearing in mind for Assignment 2: In such a report you should probably concentrate on a higher level description of the way that your application is structured. What we were looking for was a description of how your classes interacted to gain an overall effect - on the whole, detailed descriptions of individual methods are probably best placed in JavaDoc comments. A picture is worth a thousand words. For example, many people could have saved themselves time, and made their reports easier to understand, by describing the relationships between their game classes in a simple class diagram which clearly sketched their dependencies. We weren't really looking for a blow by blow account of what you did to come up with your solution. However a few comments about some of the particularly nifty solutions you came up with in response to specific problems are always handy, Try to avoid the simple listing of facts. Such reports should read as narratives, with a clear narrative flow from one point to the next. Just because this is a technical document doesn't mean that it can't be a pleasure to read. Reports of this kind should be clearly structured into headed sections - introduction, overall structure, specific technical sections, design conclusions, pointers for future work and so on. The design is of particular interest in Java assignments. What classes did you use and why? Did you use any Java features such as inheritance or composition? Most important was whether you used one of the Design Patterns you've been learning about. In this assignment dwelling a little on the MVC pattern and how it was used in your implementation was what we were looking for; and if you implemented the Observer Pattern we would have liked to hear about that too!
