Hi folks,
Howdy.
as many of you who follow this list will know, a while back I decided to learn
OOP and set myself the task of simulating a railway system.
A number of people asked if I could make the code available for viewing in
order to help me with my questions.
Yeah, and I bet you dread my messages by now. ;)
I think that I have now done enough of the coding to show my intentions. I am
(not very) quietly chuffed with what I've achieved so far.
Okay, let's cover some basic syntax nit picks first.
1. Don't call subs with &debug. That has special meaning that could get you into trouble one of these days. Stick with debug().
2. I prefer $self->{_BLOCKS}= { }; to %{$self->{_BLOCKS}}=();. It's a little less noisy.
3. Just FYI, if you're trying to use a consistent case with something like $dir=uc($dir);, lc() is usually preferable to uc(). Uppercase isn't as consistent in some dialects, so it makes your code more portable.
4. Don't nest subs. They don't work like that, so don't show it like that.
5. Whitespace, whitespace, whitespace. It's a free resource. Use it wisely. (Sound's like this might become my new nit pick.)
Okay, more general structure comments.
I'm not too found of mixing subroutines and methods, especially in the same file. Why don't you turn them into class methods, instead. This especially makes sense for things like debug(), which the other classes can then call with Trainset->debug().
Why is Tk_available() implemented in two files? Another good class method here.
Okay, for my next section, let's look at a sub:
sub add_lever_occupy { # add list of checks (interlocks)
print "\nadd_lever_occupy started: @_\n" if (&debug);
my $self=shift;
my %params=validate(@_);
if (&debug) {
print "params:\n";
print "$_=$params{$_}\n" foreach (keys %params);
}
return 0 unless defined $params{name};
return 0 unless defined $params{dir};
return 0 unless defined $params{checks};
my $lever=$self->{_LEVERS}{$params{name}};
unless ($lever) {
warn "add_lever_check: unknown lever $params{name}\n";
return 0;
}
while (my $check=shift @{$params{checks}}) {
my ($block,$state)='';
if (ref $check eq 'ARRAY') {
($block)[EMAIL PROTECTED];
} else {
$block=$check;
}
print "calling add_occupy($params{dir},$block)\n" if (&debug);
$lever->add_occupy($params{dir},$block); # string containing name
}
return 1;
}First, this method starts off with 15 lines of debugging information and parameter maintenance, before it gets to the 11 lines of actual work. That ratio seems a little off to me. You're parameter system is nice, that's quite a lead in. Can we move ensuring the needed params are defined to the validate() method? Maybe parameter debugging there too?
Mostly, this is what I think your code needs, some simplification. It's a bit bulky and makes it hard for me to study. Someday, after you've forgotten a little, you'll want to read it too. ;)
I hope that gives you some general ideas. What's you've built so far is quite an accomplishment. I don't know thing one about it's subject, but it sure looks handy.
Good luck.
James
-- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] <http://learn.perl.org/> <http://learn.perl.org/first-response>
