On Friday 27 February 2004 7:27 pm, James Edward Gray II wrote:
> On Feb 27, 2004, at 8:32 AM, Gary Stainburn wrote:
> > 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. ;)
On the contrary, without your comments I wouldn't have have got this far.
>
> > 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().
Okay, I can do that simply enough, but could you elaborate on the reason
please.
>
> 2. I prefer $self->{_BLOCKS}= { }; to %{$self->{_BLOCKS}}=();. It's a
> little less noisy.
I'm sure I tried that, along with a miriad of alternatives. I'll change the
code and see what happens. I agree about the noise, and also David's comment
about it being 'more correct'.
>
> 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.
Interesting. I never new that. But then again I only speak Yorkshire.
>
> 4. Don't nest subs. They don't work like that, so don't show it like
> that.
See my reply to John's post for an explanation of this point.
>
> 5. Whitespace, whitespace, whitespace. It's a free resource. Use it
> wisely. (Sound's like this might become my new nit pick.)
A fair comment, and one I should have thought about before releasing the code.
As the code's been changed a few times as my perceptions change and therefore
my methods, it's got a bit untidy.
>
> 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().
Wouldn't that confuse things regarding the %_DEBUG? I don't see the problem
with using subroutines within the class itself, although I could understand
it if the subs were used outside the class it was in.
Having said that, putting debug and validate into a seperate class would
eliminate the duplication of code - assuming I can sort out the hash access
problem, i.e. one class containing the code, but accessing the global
variable from the package it's called from.
>
> Why is Tk_available() implemented in two files? Another good class
> method here.
>
This was simply a bit of sloppy housekeeping. The sub should only have been
in Trainset::Tk, with the one in Trainset.pm being simply a stub to make it
easily available to the calling prog. This may eventually be changed to
return an array of available interfaces, including possibly Tk, curses,
console.
> 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?
This is one thing I intend to do at some point, including the use of default
values, mandatory parameters, and the use of the %_DEBUG hash to control
verbosity.
>
> 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
--
Gary Stainburn
This email does not contain private or confidential material as it
may be snooped on by interested government parties for unknown
and undisclosed purposes - Regulation of Investigatory Powers Act, 2000
--
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
<http://learn.perl.org/> <http://learn.perl.org/first-response>
