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. :-) )
In reply to Re: Advice on best practices
by Anonymous Monk
in thread Advice on best practices
by goldcougar
| For: | Use: | ||
| & | & | ||
| < | < | ||
| > | > | ||
| [ | [ | ||
| ] | ] |