When someone posts a "utility" on the company intraweb, and it's perl, and it's about 900 lines long, with no strict or warnings, has no indentation, needlessly builds strings and splits to an array when qw() would've been appropriate, needlessly uses ".*" in regular expressions, (mis)uses subroutine prototypes, has lots of repeated code that could be factored into subroutines, and lots of other things you'd expect to see in the sort of code that has all the previous issues, and you've already written a library to do the same thing and more (in fewer lines...though slightly more if you count the documentation), what do you do?
Are those the only two options? So maybe the code sucks, but maybe the guy is a novice coder too. Surely you can come up with something better than "you suck". Get your attitude right before ou deal with his.
Re:Try something else
runrig on 2008-08-09T07:51:55
Surely you can come up with something better than "you suck".
Surely I can, but "you suck" golfs better than "constructive criticism" in a blog title, and feels more satisfying in a rant
:-) Re:Try something else
runrig on 2008-08-09T08:40:13
Though my attitude does suck sometimes...
Re:Why do you need to do anything?
runrig on 2008-08-09T08:20:33
Why do you need to do anything?
Well, "doing nothing" would be pretty much the second option offered in the original subject...I was maybe considering something in between the two.
What do you want to achieve?
I'm not sure...I do feel like something should be done, but that maybe it's best if nothing is done. Maybe I'd like to just raise standards a bit, and do something about the proliferation of crap perl code that helps feed the notion of some that all perl code is crap...and I'd also like to achieve world peace while I'm at it
:-)
Don't fix if it isn't broken.
There's no reason to refactor the code if the net gain, i.e. working better, is zero.
Re:But... does it work?
runrig on 2008-08-09T09:15:40
It generates a report of problems with a certain type of file. There are a couple of major but easy to fix bugs. While it's ok at generating a report of possible/probable problems, it's not to be relied upon for absolute correctness/wrongness of the file...it's way too easy to trip up the parsing of the file and get false positives and negatives (e.g. comments are being parsed the same as commands). As long as there is no decree of "your file must pass this test", I won't yell too loudly.
Wait until MJD finishes his Program Repair Shop and Red Flags book, then order it and give it to the developer in question, so that in the future, they may write better code.
—————— • ——————
For the moment, I would ask: is this program still being edited? If so, it’s justifiable to take time to fix it, so give some code reviews; point out a handful of improvements that will have the largest effect. Do this iteratively, so the programmer can actually learn, instead of being overwhelmed by a slew of changes that would give him the impression that you are doing this just to show him how much better you are, not to teach.
If the code is currently “finished,” though, there is probably no justification for working on it. In that case I would wait until it needs bugfixes or features. “Refactor as you go” – which means when you’re not going, you don’t refactor, and when you are going, you always take the time to clean up any existing mess a little (to make your changes easier if nothing else). Do not clean up code just for the sake of cleaning it up; not only is this hard to justify to business types, it also does not actually add value. The next feature request is as likely as not to require you to undo some of the cleanup. (For management purposes, consider the concept of technical debt.)
"I saw XXXX and I thought you should know that I already have a library that does something very similar. If you need to extend it, make it more robust, etc, feel free to come and talk with me, because I may be able to save you a lot of work."
All the answers here make the assumption that someone, other than you, wants to improve the code. Going forward with only you wanting to change the code means that you'll be perceived as saying "This code sucks, and I think it should be done this way."
If nobody sees a problem in the code, then your first task is to help the person see that life could be improved by improving the code. Coming in and saying "This could code be better" doesn't mean anything, because "better" is a fluffy word. Why is it better to use qw instead of splits? Why is indentation better? Why is it better to use existing libraries? These are obvious questions to you, but to a novice who write code that has never answered these questions, they're not. You must answer those questions for the person.
Re:The author has to want to improve it
Aristotle on 2008-08-10T17:16:11
Which is what option #3 that I made up boils down to – wait until MJD finishes writing down good answers to these questions and then give them to the novice to read. I’m not entirely serious with that, but it’s not meant as as much of a joke as it might first seem, either.
(I am quite looking forward to that book. I am nearly (though of course never completely) certain that it will hold no big revelations for me, yet I am equally confident that I will enjoy it.)
Re:The author has to want to improve it
petdance on 2008-08-10T18:44:00
But "here, read this" is the same problem. The novice needs to want to change something, to get a benefit, in order to improve.
Because you'll (tactfully I hope) be retraining the other coder.
But why do that?
To stop. or at least reduce, the number of times
