Dermot wrote:
> 2008/8/11 Rob Dixon <[EMAIL PROTECTED]>:
>
>> - The usual reason for using a variable name with a leading underscore is to
>> denote that it's a private variable, but lexical variables aren't visible
>> outside the package anyway so you may as well drop the underscore and make it
>> more readable. You should also call it something better than HASH because
>> the %
>> says that, so how about %TYPES?
>
> I think John K had a pop at me for doing this once before. What I've
> done is change the name,% _HASH in my code actually reads %_KNOWNTYPES
> but I take the point that it's scoped and I don't have to make it read
> any uglier.
An informed decision is all we expect :)
>> - It's very unusual to bless an array reference for use as an object. I think
>> you should use a hash reference, and if you have array-like core data just
>> make
>> one of the hash values an anonymous array.
>
> There is an OO book by Conway and I read an abridged version that
> suggested this array of hash structure because array access can be
> faster. One of the elements in the hash, during testing/development at
> least is an array of 300 items.
I feel very strongly that software should be written primarily for readability.
If a highly-legible and fully-functional program proves to run too slowly, that
it the time to make changes to improve the speed. One of the delights of object
orientation is encapsulation, which lets you switch between data representations
freely as long as the the interface remains constant. I suggest you base your
implementation on a hash reference for convenience and readability, and switch
to an array reference if you need to make things work faster.
In any case, I'm certain that Conway didn't mean that things would be faster if
you simply injected an extra level of indirection. $self->{type} will always be
quicker than $self->[0]{type}. I guess what was intended was that an array
access is faster than a hash reference (and will consume less memory) so you
could write
use constant TYPE => 0;
my $type = $self->[TYPE];
and so on.
> It doesn't look like my code with exceed 600 lines. Randal suggests
> that code should be > 1000 lines before you see any benefits in using
> OO.
That is a rule of thumb, and each application has its own values to define
'benefit'. Here, for example, you should count in the advantage of learning
object-oriented coding. Object orientation is usually a trade-off of clarity and
reusability of code over execution speed (an equivalent OO solution is genrally
slower). But you must factor in your own aims and values.
>> - In a similar vein to the last point, at present you have $self->[0] all
>> over
>> the place. Is there ever going to be a $self->[1]? It doesn't look like it
>> from
>> the way you've written your methods, and you seem to have a superfluous array
>> layer in the data that just causes noise in the code.
>
> Your right and i wondered the same thing so I assigned $self->[1] as
> an opened filehandle. I don't have the exact code to hand but
> basically there is a sub like this:
>
> sub append_log {
>
> my $fh;
> open $fh, ">>$self->[0]->{type}->{logfile}" or die "Cannot append: $!\n";
> $self->[1] = $fh;
> }
>
> and later
>
> select $self->[1]
Now you're beginning to use the array for speed as I think it was intended by
Conway. But look at the cost! Wouldn't it be much nice to be able to write
select $self->{filehandle}
for now?
If I was stuck with the data structure I would quickly replace the 'real' self,
something like this
sub get_type {
my $self = shift->[0];
return $self->{type}
}
>> - Remember that any pair of }{ or ][ or a mixture of the two doesn't need an
>> intervening ->
>
> Personally I find the arrow more readable.
Fine. Use what you feel best with.
>> - To answer your question, yes it's a good idea to write accessor methods
>> like
>> type so that they will both read and write a value. But I would guess that
>> you
>> shouldn't be able to modify an object's type once it has been created?
>
> Gosh if $self's type could change that would really screw things up!!!
OK. As I thought. So you need a get_type method that only reads the value.
>> If you were to drop the spare array indirection as I suggested and rename
>> your
>> hash to %TYPES you could write
>>
>> sub type {
>>
>> my $self = shift;
>>
>> if (@_) {
>> my $newtype = shift;
>> $self->{type} = $newtype;
>> $self->{logfile} = "$TYPES{$newtype}{root}/$newtype.log";
>> }
>>
>> return $self->{type};
>> }
>>
>> which changes the type if a new one is supplied and then returns the current
>> value.
>
> I was struggling with this before I left work. In a line like
> print "Starting with ", $self->[0]->{type},"\n";
>
> I got
>
> Starting with HASH(x0023408)
>
> Not what as I was hoping for. Perhaps I was calling it correctly.
My guess is that you assigned
$self->[0]{type} = $KNOWNTYPES{$type};
or something like that. You have stored a hash reference in a fields that should
be a simple string.
> wrt Shawn's point's. I read the getter/setter in the Conway article. I
> guess it's down to context really. I am not a fan of returning undef
> on sub-routines but I'm sure there are arguements for it. In the case
> of the script I am working on, I don't want anyone to be able to
> change it's type once set and other methods are being called. In that
> case I could call get_type before I set_type to ensure that was undef
> :-/
Assign the type field in your constructor - 'new'. Then supply a 'get_type'
method if you think you'll need it.
> Thanx for all the comments. Much appreciated. Peer review rocks!
Keep us informed :)
Rob
--
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
http://learn.perl.org/
