Overview

The module we decided to code review was MusicianDirectory Old. Note that this is the file before review.

The module post review: MusicianDirectory New

This module is responsible for dynamically loading the Musician classes, their icons, and other metadata on start up.

It's worthy of code review because it executes non-trivial introspective, pattern-matching and directory walking operations, and makes 2 goals of the project possible:

• Robust, easily replaceable front end (communicates with this meta-data service to extract).
• Plug-in style of musician addition (just create a new folder with the class, icon, metadata).

Feedback

• Currently all the functionality is in one method. Separating out operations into functions is necessary.
• docstring is necessary to describe each method.
• Mostly trivial rearrangment and cleanup to better reflect flow of control and readability in several places.
• We can update the icon discovery code to be stricter.
• Instead of using tuples, refactor to create a MusicianMetadata class.
• Practice Asking Forgiveness instead of Look Before You Leap in several places.
• Exception handling errors may be misleading for several occasion. e.g. "musicians directory not found" will be printed for lack of directory permissions
• Files with certain extensions are explicitly excluded-- why not implicitly exclude any non-desired file?
• The methods could have more appropriate names: filterMusicianList implies mutability; validTags isn't a verb-noun
• With the forethought that the returned lists may be very long, the methods that return lists could become generators.

Changes

• The constructor method was significantly broken down into much smaller, more cohesive pieces to improve readability, and to help isolate any problems that may occur to one method.
• Each musician directory is handled by its own method call, which extracts all the necessary information from the files in that directory.
• Each type of file in a musician directory is also handled by its own method (getTags, getConstructor, etc.) in order to further improve readability.
• MusicianMetadata class was created to store information about each musician, such as the display name, tags set, constructor, and file path to its icon image
• Removed some exception catching in cases where the program would break in other places after the exception was caught (i.e. can't find musicians directory)
• Changed the way the constructor method is detected to a much more concise, readable statement
• validTags returns a set of tags instead of a list because it makes more sense semantically and provides easier use by the GUI code
• Icon files must now be named icon.png,icon.gif, etc. to enforce a more consistent naming convention for images in the musician folders
• Docstrings were added to all of the methods to better describe their functionality