Most of the retrospectives I’ve kept or participated in were about agile approaches (for example communication with the Product Owner) and organisation-related changes, but not everybody is into these. Most software engineers and craftsmen aren’t that interested in how to deliver faster, or how to communicate better, they are interested in how to be better at their profession: programming.
Usually, software craftsmen are interested in new technologies and improving their programming skills. The easiest way to gain knowledge and improve skills is to learn from each other, see how others are programming, what kind of tricks they are using, what problems they have, and how they solve them. Last but not least, when you read other people’s code, you will have more information about what is going on in the project(s) you are working in.
My proposal is to do code review during your retrospective meetings.
Usually, this kind of *retrospective *has the following goals:
-
learn new programming techniques
-
find quality improvement ideas
-
find coding behavioural patterns which need to be kept
-
find coding behavioural patterns which need to be changed
It is really important to keep these goals in mind, because the retrospective is not a real code review session where the goal is to find mistakes and correct them. It is about finding those things which can make you a better craftsman in the long run.
As a preparation, find some code snippets which were developed during the last sprint or in the last two weeks. Even if the participants find out who wrote the piece of code under review, try not to show the original author. Remember the goals and the long term effects; this meeting isn’t about blaming or improving an individual.
I’ll use an old project of mine for the examples. The project was a very, very simple version control system written in Java. * *I decided to review my methods which work with file versions and content copying.
I’ll present specific methods from this project for the sake of the readability of this post, but when you do code review during your meeting, remember to use code snippets from the last sprint or iteration.
The first step is to examine the snippets individually:
/**
* Returns the list of possible versions.
*
* @return the list of possible versions
* @throws IOException if error occurs while getting the versions
* from the metadata file
*/
public List getVersions() throws IOException {
List versions = new ArrayList();
metadata.load();
// Get the version from the file (3rd element in an entry -> index 2)
for (int i = 0; i < metadata.size(); i++) {
versions.add(metadata.get(i, 2));
}
return versions;
}
/**
* Returns the currently selected version.
*
* @return the currently selected version,
* ``null`` if nothing is selected
* @throws IOException
*/
public String getCheckedoutVersion() throws IOException {
metadata.load();
if (trackedFile.exists()) {
// There is a real selected version
if (metadata.size() > 0) {
return metadata.get(0, 2);
} else {
return null;
}
} else {
// The selected version was deleted, but no new version was selected
return null;
}
}My findings:
-
+ short, focused methods
-
+ meaningful method names
-
+ javaDoc comments are present and accurate
-
- do not return null, *otherwise you have to “null check”* values before calling a method or accessing a member variable, use null object pattern or special case pattern instead
-
- use constants instead of magic numbers
-
- mind the explanations in comments (in getCheckedoutVersion() the explanation of the second argument of metadata.get(0, 2) is missing)
With this approach, the team can find and improve existing coding habits. However, this will not present additional good practices - for that, make sure you also check** code snippets written by somebody else outside the team**.
There are tons of open source projects out there, and it is a really good practice to check them from time to time. There is a psychological reason why the code quality of open source projects is usually high and the contributors have the tendency to use the latest techniques and algorithms.
So, the next step is to compare one of our snippets to the code of an open source project. There are some things worth keeping in mind:
-
look for code written in the same language
-
look for code which does quite the same as your candidate
-
limit your efforts on finding the right candidate (maximum 1 hour)
I was very lucky, because I found a copy method in jgit, which is the Java implementation of the git version control system.
The source of jgit’s LockFile#copyCurrentContent() method:
public void copyCurrentContent() throws IOException {
requireLock();
try {
final FileInputStream fis = new FileInputStream(ref);
try {
final byte[] buf = new byte[2048];
int r;
while ((r = fis.read(buf)) >= 0)
os.write(buf, 0, r);
} finally {
fis.close();
}
} catch (FileNotFoundException fnfe) {
// Don't worry about a file that doesn't exist yet, it
// conceptually has no current content to copy.
} catch (IOException ioe) {
unlock();
throw ioe;
} catch (RuntimeException ioe) {
unlock();
throw ioe;
} catch (Error ioe) {
unlock();
throw ioe;
}
}My legacy version:
public static void copyFile(File source, File destination)
throws IOException {
InputStream in = new FileInputStream(source);
OutputStream out = new FileOutputStream(destination);
byte[] buf = new byte[1024];
int len;
while ((len = in.read(buf)) > 0) {
out.write(buf, 0, len);
}
in.close();
out.close();
}My findings:
-
+ my version looks cleaner, and the exception handling is better
-
- I don’t have a locking mechanism
-
- in case of failure I won’t close the streams properly
-
**- **both methods could be more readable
-
# does the buffer size make any difference?
As a last step, review projects written by experts like Robert C. Martin. He wrote fitnesse, so that code must really be clean. Here is a snippet from the FileSystemPage class:
private void loadContent(final PageData data) throws Exception {
String content = "";
final String name = getFileSystemPath() + contentFilename;
final File input = new File(name);
if (input.exists()) {
final byte[] bytes = readContentBytes(input);
content = new String(bytes, "UTF-8");
}
data.setContent(content);
}
private byte[] readContentBytes(final File input) throws IOException {
FileInputStream inputStream = null;
try {
final byte[] bytes = new byte[(int) input.length()];
inputStream = new FileInputStream(input);
inputStream.read(bytes);
return bytes;
} finally {
if (inputStream != null) {
inputStream.close();
}
}
}My findings:
-
+ small, readable methods
-
+ proper method names
-
+ good exception handling
Now I have several findings, let’s see what I should continue to do and what I should improve when I do programming next time:
Keep:
-
short methods and clean code (from self-check and expert compare)
-
updated javaDoc (from self-check)
Change:
-
use more finally blocks in error handling (from open source and expert compare)
-
locking mechanism (from open source compare)
-
introduce null object pattern (from self-check)
Think about:
- what will happen when I increase the buffer during copy? (from open source and expert compare)
For a code review based retrospective, I recommend the following activities
-
look for some code snippets from the last sprint or from the last two weeks
-
review those code snippets together as a team
-
compare those snippets to snippets from open source projects
-
see how experts do it
-
find a manageable list of what needs to be kept and what needs to be changed
Code review based retrospective can help you get better as a craftsman, but in order to be more agile you’ll also need the agile approaches I mentioned at the beginning. So find a healthy mixture of agile and craftsmanship related activities for your retrospectives: fifty-fifty usually works. Keep me posted how it turned out for you.
comments powered by Disqus