Organizing your program with subroutines (was Re: help with user selected hash operations?)

~~You've already got some good suggestions, so I'll make a different one.~~

TL;DR; OK, so I occasionally go off and write a little (ha!) more than I expected to. My coffee took longer than normal to wake me, so by the time I noticed, I had already written all of this stuff. It's not quite as polished as I'd like, but I've got stuff to do, and it's time to move on.

Anyway, the primary point I wanted to make was that you'd benefit by using some subroutines to organize your code. You can stop reading now, or if you want to see why, then read on...

Start with an outline

One of the most important things about programming is to break your big problems up and turn them into smaller problems. This makes 'em easier to write and easier to test. The key is to make some subroutines. Don't worry about them being "too small" or "too large" yet. Just use them to simplify your code a bit.

When I first started programming, I'd write an outline using comments:

# initialize variables

# Loop
  # Display menu and choose item
  # Process menu item:
    # a: add son/father pair
    # d: delete son/father pair
    # e: exit
    # g: get a father
    # o: output hash neatly
    # r: replace a father
    # x: get a grandfather
[download]

Comments can be important, but later I learned that good variable and subroutine naming makes most comments superfluous. So then I started making my outlines nearly executable by just going ahead and giving my subroutines good names. Let's look at the main body of your code but simplify it to outline form:

initialize_variables();

my $is_time_to_exit = 0;
while (! $is_time_to_exit) {

  my $choice = choose_menu_item();

  if ($choice eq 'a')    { add_son_father_pair(); }
  elsif ($choice eq 'd') { delete_son_father_pair(); } 
  elsif ($choice eq 'e') { $is_time_to_exit = 1; }
  elsif ($choice eq 'g') { get_a_father(); }
  elsif ($choice eq 'o') { output_hash_neatly(); }
  elsif ($choice eq 'r') { replace_a_father(); }
  elsif ($choice eq 'x') { get_a_grandfather(); }
  else {
   print "Invalid choice, TRY AGAIN!\n";
  }
}
print "Come back again -- goodbye!";
[download]

This way it's easy to read--it nearly reads like english. You're not cluttering up your main loop with lots of little bits of code.

Now you can create each of your subroutines independently. Let's try add_son_father_pair. Plugging in your original code (with a couple tweaks to accomodate it being in a subroutine) would be something like this:

sub add_son_father_pair {
  print "Enter a male name: ";
  chomp (my $name1 = lc <STDIN>);

  if (exists $son_father{$name1}) {
     print "Duplicate name -- try again!\n";
     return;
  }

  print "Add a father: ";
  chomp (my $add_dad = lc <STDIN>);
  $son_father{$name1} = $add_dad; 
}
[download]

Stub out your routines

During development, I rarely write the while program in a single pass. Instead, I'll frequently create a "Not Yet Implemented" function for each subroutine that I create so that I don't forget about it. You don't need to do that, as perl will give you an error message if you call a subroutine that doesn't exist. I find it useful, though, because I can easily see what functions I haven't created yet. For functions with particular argument lists, special cases or return values, I can add a little information while I'm thinking about it. To do so, I typically create them like:

# Nothing special about these:
sub add_son_father_pair { die "NYI" }
sub delete_son_father_pair { die "NYI" }
sub get_a_father { die "NYI" }
sub output_hash_neatly { die "NYI" }
sub replace_a_father { die "NYI" }
sub get_a_grandfather { die "NYI" }

# I wanted a bit more detail about this one, though.
sub add_daily_task { die "NYI";
   my ($task_name, $day_of_week, $task) = @_;
   # Dies on error, otherwise returns $time_of_day the task is schedul
+ed for
}
[download]

Putting the extra detail there when you're thinking about it lets you write it down and then forget it. You only have so much room in your head for active thoughts, so you don't want to clutter your head with details that you'll need later. Write 'em down, forget about it, and free up your brain to work on the next chunk.

Now, any time I need to figure out what is left to be written, I can just:

$ grep NYI my_perl_script.pl
sub add_son_father_pair { die "NYI" }
sub delete_son_father_pair { die "NYI" }
sub get_a_father { die "NYI" }
sub output_hash_neatly { die "NYI" }
sub replace_a_father { die "NYI" }
sub get_a_grandfather { die "NYI" }
sub add_daily_task { die "NYI";
[download]

and see what needs to be written at a glance.

Break out commonly used code into subroutines

That's not too bad, but I would simplify it a little further so it reads better. You've got a couple places (and will likely add more when you add options r and x) where you're printing a prompt, fetching a value from the user, changing it to lower case and chomping the line ending. While the code is short, it's just a bit noisy, and we can make it read more like english by making an ask_question subroutine:</c>:

sub add_son_father_pair {
  my $name1 = ask_question("Enter a male name: ");
  if (exists $son_father{$name1}) {
     print "Duplicate name -- try again!\n";
     return;
  }

  my $add_dad = ask_question("Add a father: ");
  $son_father{$name1} = $add_dad;
}

sub ask_question {
   my $question = shift;
   print $question;
   chomp(my $answer = lc <STDIN>);
   return $answer;
}
[download]

Keep your momentum and defer problems

As we just mentioned, we want to put repetitious code into subroutines to simplify and shorten your programs. You can also use subroutines to keep your coding speed up. When you're working at a high level of abstraction, you know the overall operations you want to accomplish. But diving into the details makes you shift perspectives. You can avoid that by simply deferring a particular problem into a subroutine: giving it its own little box for you to solve later, while you're still coding at your higher level of abstraction. Once you finish at your higher level, shift your perspective to another area, and do the same.

A subroutines value isn't only the ability to reuse code in multiple places: It lets you put boundaries around a problem and give it a name. So when you're debugging your program later, you don't get overwhelmed with details or have to suss out details of multiple levels of your program at once.It turns out that subroutines are often useful even if you use them only once: It lets you put a defer a problem--putting it in its own little box as it were.

Refactoring your code

Once you start breaking things up into subroutines, you'll sometimes find that you didn't choose the best place to split things up or use the best name for the subroutine to make it read well. Don't worry about it, just change it. For example, what if you had so many menu choices that you had upper case and lower case choices? Then you couldn't use ask_question for the menu choice.

One way we could adjust things would be to explicitly use your print "Make your choice: "; chomp(my $choice = <STDIN>); in your choose_menu_item subroutine, which wouldn't be so bad, since it would be isolated in only one location.

Another choice would be to remove the lc function from ask_question, but that would require that every time you're getting a name from the user, you'd be doing something like:

  my $name1 = lc ask_question("Enter a male name: ");
[download]

That doesn't feel nearly as good as the previous choice, because you'd be having to remember where to use lc and where not to. In your program, though, you're presumably mapping the names to lower case to ensure that you don't have difficulties with your hash keys (i.e. accidental duplicates, or not being able to find a name because it was capitalized in one place and not in another). Since subroutines are cheap, you could capture that requirement by creating a new subroutine to handle that case:

sub ask_question {
   my $question = shift;
   print $question;
   chomp(my $answer = lc <STDIN>);
   return $answer;
}

sub ask_for_name {
   my $question = shift;
   return lc ask_question($question);
}
[download]

This way, any time you're asking for a name related to your hash, you can use ask_for_name, and any time you don't need the lower-casing, use ask_question. These are the kinds of tradeoffs you'll find when programming. No-one starts out automatically knowing the best bits of code and subroutine names to use--that comes with practice. When you start out, you might reformulate subroutines many times, and later you'll gain experience and make much better guesses.

What subroutines to create first?

When you start fleshing out your program as an outline, it's easy to wind up with many functions that aren't implemented yet. How do you know which one(s) to implement first?

Frequently, it doesn't really matter. Just create them in any order that's comfortable for you. I generally implement whatever feels most comfortable to me at the time. However, I'll often start with the ones that provide useful information for debugging. In your case, I'd implement output_hash_neatly() first. Then I normally choose functions used to alter the data (such as add_son_father_pair or delete_son_father_pair). Since we've already got output_hash_neatly, I can use that in development to start building and testing my code, like this:

sub add_son_father_pair {
  # show the "before" data
  print "add_son_father_pair before\n";
  output_hash_neatly();

  my $name1 = ask_question("Enter a male name: ");
  if (exists $son_father{$name1}) {
     print "Duplicate name -- try again!\n";
     return;
  }

  my $add_dad = ask_question("Add a father: ");
  $son_father{$name1} = $add_dad;

  # Did we do it right?
  print "add_son_father_pair after:\n";
  output_hash_neatly();
}
[download]

I only put all that extra code in if I'm having trouble with a function. Normally, I'll just write it as I expect to be able to use it.

Another criteria I use to figure out what to write next is by running the script, and letting perl tell me what it wants next:

 
$ perl my_perl_script.pl
Undefined subroutine &main::choose_menu_item called at my_perl_script.
+pl line 7.
[download]

Now I know that to make progress running my program, I'd best implement choose_menu_item! ;^)

Uh...Conclusion

Subroutines are a powerful tool to help you organize and develop code. Sometimes they'll make your code longer, but that's not a problem. Breaking your code into logical chunks with good names makes it easier for you (or your colleagues) to understand the code and make successful changes with a mimimum of effort. Yes, subroutines often will pay off with a shorter program, but that's really not the best part, it's just an added bonus to the ability to make your programs clear and maintainable.

One final hint: When you have a large monolithic chunk of code, you don't need to start over and rewrite it. In fact, that's normally a bad idea--you're spending a lot of time on something that may be less important than your next task. Limit your refactoring to the part of code you need to modify. If your changing a hairy bit of code and can pull it out into a subroutine, that's great. If that bit of code has a chunk that should be turned into a subroutine, then do it. If other parts of the program should also be using that subroutine, then OK, make the change and update the call sites. Remember, however, you need to carefully test all the changes you made in addition to the overall operation of the program, so don't burden yourself with making gratuitous changes.

I'll frequently add # TODO: extract this into a subroutine comments while I'm working in a program, though, both to inform others of a refactoring opportunity and as a reminder list. After finishing your modification, you can mention to the owner of the program some improvements that can be made, and you may be tasked with making those improvements: either immediately if you have time on your schedule, or when you next have to modify the program.

Before making any modifications to a program, I'll frequently scan for TODO comments to see if there are any items that could/should be added to the currently specified change(s):

$ grep TODO my_perl_script.pl
# TODO pull these next few lines into a frobnicate() subroutine
# TODO Can solve world hunger .. but this line is too short to contain
+ the solution
# TODO Try to remember the solution to above TODO!
[download]

Scanning the TODO items is a good way to help refresh your memory on some internals, and can help you refine requirements a bit for your current change. Sometimes a change is intrusive enough to make many refactorings, so having a TODO list is a good way to make sure you don't miss something obvious.

Update: Fixed a code tag (<c> --> </c>)

...roboticus

When your only tool is a hammer, all problems look like your thumb.

Comment on Organizing your program with subroutines (was Re: help with user selected hash operations?) Select or Download Code


Come for the quick hacks, stay for the epiphanies.
	PerlMonks