|
Re: Tact and the Monastery
by clintp (Curate) on Sep 14, 2002 at 15:01 UTC
|
A technique I've employed is holding a code review of my own code. There's an awful lot you can say about someone else's code and have it come off as self-criticism.
...at first I had it coded like this {show nasty code} and decided it'd be more efficient/pretty/maintainable like this {show good code}... and then go on to explain why it was more maintainable/pretty/efficient.
And you don't really have to beat it to death either, and certainly not drop any snide remarks about the bad stuff you've seen in their code. Just make sure that the bad stuff you *do* show them is your own: they may recognize their own code style.
| [reply] |
|
|
| [reply] |
|
|
()-()
\"/
`
| [reply] |
(jeffa) Re: Tact and the Monastery
by jeffa (Bishop) on Sep 14, 2002 at 12:57 UTC
|
| [reply] |
Re: Tact and the Monastery
by Ovid (Cardinal) on Sep 14, 2002 at 16:55 UTC
|
Interesting ... we just hired a new programmer and I recommended that he get an account here. His code seems good and solid and I definitely signed off on his code review. However, he has had no exposure to the Perl community. Personally, I was rather surprised by that as I've noticed that there tends to be a correlation between code quality and exposure to the community (and I don't just mean Perlmonks).
I have had the same experience as you regarding self-improvement since I've come to this site. I've heard some complain about the signal to noise ratio or they think that the Monks are too dogmatic, but while I would choose different labels, I wouldn't disagree with those statements: these qualities often foster spirited debate and when I get to read long threads of pros and cons of a particular thing, I actually have a chance to see a variety of viewpoints. When's the last time you saw that in some dusty ol' computer book? :)
Cheers,
Ovid
Join the Perlmonks Setiathome Group or just click on the the link and check out our stats.
| [reply] |
|
|
| [reply] |
|
|
| [reply] |
|
|
|
|
|
Re: Tact and the Monastery
by kschwab (Vicar) on Sep 14, 2002 at 15:02 UTC
|
OTOH....
I'm always leery of these "this other guys code was a mess"
comments. Certainly it's sometimes (mostly?) true, but "messy" is in the eye of the beholder.
For example, I tend to use lots of temporary variables where
they probably aren't needed. I've gotten negative feedback
on that. I think, however, that they make the code more
maintainable:
# crypt the password
my @chars=(a..z,A..Z,0..9,'.','/');
my $salt= $chars[rand(@chars)] . $chars[rand(@chars)];
my $crypted=crypt($password,$salt);
Now, the @chars and $salt variables aren't really needed, and I could
have done something like:
my $crypted = crypt( $password, join( '',
('.', '/', 0..9, 'A'..'Z', 'a'..'z')[rand 64, rand 64]));
which is more compact, but less readable, IMO.
I've also gotten negative feedback on code reviews for
overly defensive programming, which again, adds lines
of code and generally makes for more "mess". ( additional
eval blocks, lots of defined() calls, etc).
| [reply]
[d/l]
[select] |
|
|
| [reply] |
|
|
My rule is that a temporary variable is fine if it can be related to the application in some way, and not just an artifact of optimization.
One way to tell is if you can give it a truly meaningful name. If you can't, see if you can eliminate it.
There is sense & beauty in that. But it does require a high
degree of Perl knowledge.
Corollaries can be derived.
Temporary variables should be named so that it is obvious
that they are temporaries.
Temporary variables should
be scoped to advertise their ephemeral nature.
For those who can't readily parse dense Perl temporary variables are like the x modifier to m//.
| [reply]
[d/l]
[select] |
|
|
| [reply]
[d/l] |
|
|
|
|
|
|
I find your first example unnecessarily noisy, while the second is too golfish. Balance is key. Mine would look like so:
my @chars = ('a' .. 'z', 'A' .. 'Z', 0 .. 9, '.', '/');
my $crypted = crypt $password, $chars[rand @chars].$chars[rand @chars]
+;
That way I can't make a mistake in the size of the list vs the constant passed to rand. Nor will I have to figure out why I used 64 there if I read the source again in a year. It also lets me use concatenation vs the noisy and distracting join/slice method.
$salt on the other hand is superfluous - that value is trivially calculated on the fly.
Makeshifts last the longest. | [reply]
[d/l] |
|
|
OTOH... ;)
I personally think the second of your examples really is more readable. But maybe I'm just a bit nuts.
Myself, I use as few temporary (synthetic) variables as possible, I think they often make things harder to understand. Even if I immediately get the idea what @chars is, I would glance back on every occurance of it, to make sure what your idea of "characters" really is (with or without underscore, hypen, etc.). If the code spreads out, things become even worse...
In your second example, it's right there.
Which brings us back to the topic of this thread. While I am sure that there are some style ideas, any good Perlmonk would propagate, some other things really are a matter of taste.
| [reply]
[d/l]
[select] |
|
|
I don't like the comma-prepending style. You can easily add a new line at the bottom that way, but not at the top. It might be fine for languages that are inflexible about their commata, but Perl is not - you can append one to the last value with no harm. My formatting looks like this:
my %hash = (
one => 1,
two => 2,
three => 3,
);
Note the "errant" comma after the last value. Separating the assignment and closing bracket on their own lines adds more convenience. This way, I can add a hash key anywhere - even at the bottom or the top - without having to edit any other line than the one I'm on.
Makeshifts last the longest. | [reply]
[d/l] |
|
|
I'm with you on this one kschwab.
I certainly try not to use too many temporary, intermediate variables, but I do if they add to readability for a future maintainer of the code.
My basic criteria for using a temporary variable is this:
If I can think up a meaningful, transparent name for it, I can use it.
For example, in your case, @chars and $salt are both meaningfully named, thus allowed. What I would not allow would be @array and $foo for the same variables.
Regards,
Helgi Briem
| [reply] |
|
|
I would probably change that to look like
# crypt the password
my $crypted;
{
my @chars=(a..z,A..Z,0..9,'.','/');
my $salt= $chars[rand(@chars)] . $chars[rand(@chars)];
$crypted=crypt($password,$salt);
}
so that the temporary variables (@chars and $salt) disappear after the closure ends. And I agree with the suggestion lower down: you could lose $salt and incorporate the salting in the call to crypt.
--t. alex
but my friends call me T.
| [reply]
[d/l]
[select] |
|
|
| [reply] |
|
|
|
|
Re: Tact and the Monastery
by jryan (Vicar) on Sep 14, 2002 at 07:14 UTC
|
Of course, if he knows your username, he'll now know your true opinion on his code ;) | [reply] |
|
|
| [reply] |