$ sudo apt-get install mercurial
$ hg clone http://rmhhomebase.hg.sourceforge.net:8000/hgroot/rmhhomebase/rmhhomebase
Find and fix the "Bad Smells"
Long Method: A method should do one thing and only one thing. addWeek.php violates this rule of thumb by accepting various parameters, such as publish, reset, and remove in order to change the main function's actions.
if($_GET['publish'] && $_SESSION['access_level']>=2) {
$id=$_GET['publish'];
$week=get_dbWeeks($id);
if ($week->get_status() == "unpublished")
$week->set_status("published");
else if ($week->get_status() == "published")
$week->set_status("unpublished");
update_dbWeeks($week);
add_log_entry(''.$_SESSION['f_name'].' '.$_SESSION['l_name'].' ' .
$week->get_status().' the week of get_id().'&edit=true\">'.$week->get_name().'.');
echo"Week \"".$week->get_name()."\" " .
$week->get_status() . ".
Back";
}
// resets a week if the user is a manager
else if($_GET['reset'] && $_SESSION['access_level']>=2) {
$id=$_GET['reset'];
$week=get_dbWeeks($id);
delete_dbWeeks($week);
add_log_entry(''.$_SESSION['f_name'].' '.$_SESSION['l_name'].' reset the week of get_id().'&edit=true\">'.$week->get_name().'.');
generate_populate_and_save_new_week(substr($id,0,2),substr($id,3,2),substr($id,6,2),
$week->get_weekday_group(),$week->get_weekend_group(), $week->get_family_room_group());
echo "Week \"".$week->get_name()."\" reset.
Back";
}
else if ($_GET['remove'] && $_SESSION['access_level']>=2) {
$id=$_GET['remove'];
$week=get_dbWeeks($id);
if ($week->get_status()=="unpublished" || $week->get_status()=="archived") {
delete_dbWeeks($week);
add_log_entry(''.$_SESSION['f_name'].' '.$_SESSION['l_name'].' removed the week of get_id().'&edit=true\">'.$week->get_name().'.');
echo "Week \"".$week->get_name()."\" removed.
Back";
}
else echo "
Week \"".$week->get_name()."\" is published, so it cannot be removed.
Back";
}
This code tries to do three different actions in one function. The easiest fix is to separate these actions into their own functions. The goal is to have high cohesion.
Too Few Comments: Having looked through most of the code, it is safe for me to say that this software has too few comments overall. Some files, such as personEdit.php, have a decent number of comments. The comments do a good job explaining "why" instead of "how" something is done. A pair of fresh eyes can look at personEdit.php and get a sense of what is happening and why is it important. In my Operating Systems class, students are encouraged to comment all but the most obvious lines of code. An instance variable "counter," when used with a loop, is obviously a counter. Commenting every line would classify as too many comments, and this is also a "bad smell." personEdit.php finds a point in the middle of both extremes.
Other files, such as editMasterSchedule.php, only have a few comments for hundreds of lines of code. Maintenance accounts for a large percentage of a software's life cycle; therefore, good documentation is necessary in order to reduce maintenance costs and avoid future hassles. The function get_day_names() in editMasterSchedule.php is a simple example of too few comments. Monday through Friday are determined through a very similar process via if statements, but Saturday introduces a substring method that is not used for any other day of the week. Why? If there is something intrinsically different about Saturday that requires it to be handled differently than a weekday, a comment should reflect that. Given a few months, it is difficult to forget even your own code. Comments increase readability and reduce maintenance costs. It might take a few extra seconds to type a comment, but the benefits are well worth this extra coding time.
Data Clumps: A data clump occurs when several variables consistently appear together. The only data clump that I can find in this code is related to the date. Month, day, and year are kept together in a date object, but the string name and three character abbreviation are also used to identify days. The function get_day_names in editMasterSchedule.php assigns these string identifiers manually, but this could easily be done in a custom date object.
Speculative Generality: Speculative generality is defined as "inserting features into the code for functionality that is not part of the current requirements." Having looked over chapter 5, appendix A, and RMH Homebase's source code, I have not noticed any included features that are not part of the requirements.
No comments:
Post a Comment