This blog by MJD changed how I feel when I see what seems at first to me like crazy bad code. Give the people who wrote what's already in place the benefit of the doubt. They may simply have landed on an inelegant approach to solving a more difficult set of issues than you're considering. Early on, learn about what is in place, why it is there, what decisions led to it, and how it solves the problems that you may not be weighing yet.

Only after you've taken the time to fully understand the thought process that led to the code you now see will you be able to maximize your own contributions. And at that point hopefully your patience and open mindedness will have won the respect of your team.

I really really really like this post. This is one of those things where the person is pretty smart, but they're working in a vacuum. I think communication is the overall key here, and code reviews. I've been introduced to so many tools 3/4 of the way through a project that quite often replace almost all of what I was working on. Part of that is my fault for not communicating enough and part of it is serendipity, getting in touch with the right person at the right time.

Regardless, more communication lends itself to more chances for these encounters.

Three thousand years of beautiful tradition, from Moses to Sandy Koufax, you're god damn right I'm living in the fucking past

Most of the issues you raise are language agnostic and boil down to: "don't repeat yourself", "don't reinvent the wheel" and "use the power of the force" (well, that one I just invented to encapsulate "don't shell out if you don't have to" in a sexier package).

Perl and its environment provide much better ways of handling all the issues you mention and adopting the solutions you suggest would result in more robust and easier to maintain code. However this doesn't sound like a coding issue to me. It sounds like a culture or personality issue. I suspect the other consultant is not lazy enough to do the work up front that would ensure robust, reliable and maintainable code and it may be hard to reprogram that consultant appropriately. You need a way to convey the virtue of laziness, and that being lazy means doing the required work so you don't have to redo it.

Some of the required work may be learning to use the tools available to best advantage and that very likely requires learning new stuff. Many people will willing crawl over fields of broken glass to avoid having to learn new stuff.

Your choice is between just going with the flow and introducing minor clean ups as you can without disrupting the project, or finding a way to reprogram the consultant in a short enough time to be useful and still get the project done on time. Someone is going to suffer regardless!

The following is my pragmatic (I hope) viewpoint; there will be other ways to look at this. So instead of telling the consultant "these guys who I don't know said your code isn't good", you might want to treat possible issues in the same way one would treat a bug: if you think something's wrong, develop a test case that shows that it is wrong, then that same test case can be used to make sure the problem has been fixed. Anyway:

Reusing code- sub doXYZ exists in all 5 of the perl files, duplicated exactly. Shouldn't he be creating a module for the shared sub functions and including it, so the code for these is shared?

The only one of your questions with an obvious answer: Yes.

File IO- he builds a string of the windows command he wants to run, and runs it via System ... is this the wrong way?

It's only truly "wrong" if there is any doubt about the contents of the variables being interpolated into the system command. For example, if they contain user input, it may expose a security hole (!). If they contain certain special characters, the quoting might break. Also this solution is not really cross-platform compatible; using modules like File::Copy is, and avoids issues with shell metacharacters in filenames. On the other hand, if (and only if!) the variables being interpolated into the system command always follow a known pattern, this way of doing it may be less efficient compared to using a module, but not necessarily "wrong".

Personally, I'd always use the File::* modules, or Path::Class / Path::Tiny. And if I have to, instead of system, I'd use IPC::System::Simple, IPC::Run3, Capture::Tiny, or IPC::Run. So if you still have the chance to influence this code, I'd suggest you try to go with those.

Monitoring for new files- while(1==1){ ... sleep(60); }

If it's only one process polling the directory on the system every 60 seconds, then there is nothing wrong with polling (I'm assuming the process consumes negligible resources while sleeping). Sure, there are more "elegant" solutions (see e.g. here, which among other things suggests Win32::ChangeNotify), but polling isn't immediately "wrong".

Connecting to SQL Database- It does this by writing to a hard coded file with a list of file names, return delimited, that is has finished. Then there is a PostgreSQL scheduled job looking for that file, importing it into a table. It then has the filenames, and then goes out again and issues the PostgreSQL COPY command to copy that file into the appropriate final table.

I'd say it's only slightly Rube Goldberg; and again it depends on how well other aspects of the system are under control. What I mean by that is exactly what you've identified - there may be problems with concurrency, and it depends how often this insertion happens. So while certainly one solution is to use DBI and generated filenames (e.g. File::Temp), or INSERTing the data directly, the other approach is simply to make sure that there can't be multiple processes reading and writing to those files using some kind of centralized locking mechanism (also, what happens if one process hangs, how is that controlled?). That may be less scalable, but whether or not it needs to be, that again depends. (Personally, I'd try to be less Rube Goldberg. :-) )

I would suggest you simply describe what is wrong with the practices you disagree with, or if they aren't wrong per se, why your alternative is better. That would go much farther than appealing to community standards.

In your first example, programmers in every language learn about the value of code re-use, so your senior developer knows he should do it; perhaps he is just unfamiliar with Perl modules. Surely those well-known advantages are case enough to make the change. And a senior developer should know from experience that tracking down and modifying each and every one of those duplicated code sections will take longer than it would to read a tutorial on how to use modules. If not longer in time, greater in annoyance! And a new skill would be gained.

The more general argument would apply for learning how to search CPAN. Or at least to learn about what functionality the core modules provide.

I think your request is very, very problematic.

Just because you can't argue in a business environment like this: "I've been told on PM that your stuff is crap!"

I know, this is a bit exaggerated. But i think such issues can't be handled this way.

If i where in your shoes, i would consult another consultant for a review, AKA second opinion.

Best regards, Karl

Hi Karl, Thanks for your reply. I definitely wouldn't handle it that way. We are both consultants working for the same customer. So, I can't just suggest that the customer should hire another consultant for a second opinion without having some basis first. However I want to do the right thing for the customer. So, given that I"m no perl expert myself, I'm looking for some feedback from others to see if there is enough problems with what I've outlined in the post to justify bringing it up to the customer with the suggestion of further review by another consultant. If however the feedback from others on this forum is that what he is doing is standard/best practice, then there is no need for me to bring it up, as I don't want to offend him and cause unnecessary confrontation. My experience however, leads me to believe these are not standard/best practices.

"I definitely wouldn't handle it that way"

This is good.

"My experience however, leads me to believe these are not standard/best practices."

Why don't you trust your experience?

Some more thoughts about your request:

I feel competent to say some things about most of the examples you posted because most of this stuff is a good part of my daily work.

But what do you think? How should i (or some other monk) handle this kind of request?

Can you imagine what this implies?

I think PM isn't made for the things you request.

Edit: I changed my mind

Update: Just some thoughts about code reuse, for example:

From time to time i stumble over some code i wrote some years ago - in different languages.

It's always the same - i ask myself (or i am asked): "Why did i/you do it?"

Many reasons:

• I didn't know it better at that time...
• I didn't have the time to be more generic because...
• The admin department wanted that stuff standalone...
• I was in such a hurry...
• Holy shit, i forgot it...

I hope you got it ;-)

My best regards, Karl

«The Crux of the Biscuit is the Apostrophe»

I appreciate everyone taking the time to respond. I am really just looking for technical answers here, like what Anonymous Monk has provided. I provided 4 examples, and I'm just looking to know if those are standard/best practices. I provided some context to my question, just because I thought it generally helps to explain the context of a question. While I appreciate your advice, I'm comfortable with how to handle it with the customer. The only issue I am seeking advice on (and why I came to Perl Monks) is to know the if those 4 techniques are implemented in standard/best practices. For example, if you hired a developer and he wrote code like the examples I provided, would you find it acceptable? Is it the right way to do those things? Feedback, like from Anonymous Monk, is what I was looking for, and how I phrased my question. (Maybe I should have left out the context?)

To answer the other comments about worry if the other developer find this...That isn't a concern. I'd expect if he knew of Perl Monks, he would have seen some other code examples and utilized other perl modules and not written the code as he did. Even so, if he does find it, he will know I at least took the time to check before I brought it up to him. If someone took the time to first check with knowledgeable people to see if my code wasn't up-to-par before they questioned it, I would actually appreciate that. At least they cared enough to check first before making ignorant statements. I'd like to discuss it with him from a more knowledgeable position, and not being a perl developer for many years, I feel like some additional feedback from some perl pros would help with that. ex, I say "Wondering why you did XYZ that way?", he says "Because thats the best way!". "Best" imho means it was done in best practices, in a standard, most efficient way. What I'm getting so far is, its mostly in the "gray area". Some would do it differently, but its not "wrong". So, based on that, I would say its best for me to not even bring it up.

... if those 4 techniques are implemented in standard/best practices. For example, if you hired a developer and he wrote code like the examples I provided, would you find it acceptable?

Those are two different things, which is kind of what the earlier post was getting at. For example, I'd say copying/moving/deleting files via system is almost certainly not a best practice (the File::* modules are in the Perl core and therefore almost always available). Is it "acceptable"? Sometimes, if the appropriate care is taken and the strings being interpolated into the command are well-known. If it's fairly easy for you to find a test case that shows a failure, then it's a problem!

I think you have some valid concerns, especially if this is code you're working in tandem with.

I think you need to take the human approach here and just ask the person that wrote it or is maintaining it.

"Hey $author, I was taking a look at this code you wrote and I was curious why you did $thing this way?"

This way, it's not confrontational, and they don't really have an excuse to get defensive. You're there purely for educational purposes. I've found a lot of times people get kind of a sheepish look and go "yeaaaa it's really bad code, sorry, I was in an enormous rush etc." If they try to justify it with something stupid, well, you have your answer then.

If you are both “consultants working for the same company,” and therefore you are not the person for whom this consultant is working and/or someone whose ability to do your job is not directly and materially affected, then I would cordially suggest that you ... well ... “mind your own business.”

Although the methods being used by this person might “smell bad” to your own personal set of sensibilities, you don’t know what instructions this person might have been given by the person who issued his contract, and you also don’t know what is or isn’t important from the point of view of the person who is signing his checks.   None of the decisions he’s made are indefensible.

It’s not only bad form for one consultant to question another consultant’s methods or competence ... it can well be political therefore career suicide.   If he does anything at all with Perl, he knows about PerlMonks, and it won’t take him long to find out that he’s been pretty well body-slammed there.   Not the wisest move you’ve ever made ...

mind your own business

sundialsvc4, on your home node you advertise your services as "... spends most of his professional time with legacy software projects (in Perl and otherwise) that have wandered far off-course, playing a pivotal role in turning the projects around and restoring the software to reliable service". Advising goldcougar to leave potentially bad code unquestioned could be advantageous to your business. That's not just a conflict of interest, that's somewhere in the gray area of criminal business practice.

Then again, you're probably just trolling for Worst Nodes as usual.