Pathologically Eclectic Rubbish Lister | |
PerlMonks |
Re: Evaluate my news retriever... (pretty please!)by jarich (Curate) |
on Dec 15, 2003 at 05:54 UTC ( [id://314770]=note: print w/replies, xml ) | Need Help?? |
I've spotted a few things for you. 1. Drop diagnosticsBtw. This is production code that fills a database with news articles which are extracted from XML code. The script needs around 5 minutes to parse through 1200 articles/stories
It won't cut the code time down a lot, but it'll help a bit if you drop the use of diagnostics once this code goes into production. Using diagnostics will make the code take longer to compile and if your code is running warning free then you don't need this. 2. Consider standard modulesHave you considered using Getopt::Std for your script argument handling? What you're doing to find a single flag is fine, but if you want to add further switch handling you may want to take a look at the module. 3. Use the right quotes (and consider pulling out common strings)
I think you mean to use qq// or q// here rather than qw// I also think that these would be better written to use just a couple more variables: because this will allow you to change the site, id or password, if necessary, in one place rather than many. 4. Code order A general note of good programming practice. Usually subroutines appear right at the bottom of the main code rather than in the middle of them. This means that someone reading the code gets a better idea of code flow. 5. Don't escape your own SQL (use placeholders)
You're using DBI, there should be no need whatsoever for you to be self-escaping these values. And if you can think of a need then you really should be using the DBD's quote function not your own.
Yup, as I feared. If you use placeholders here and update your sqlStatement subroutine here you will get much better escaping of your data. Consider the following case: After your escape attempt this will now cause you problems. Rely on DBI/DBD to do your quoting correctly! 6. Verify your inputs It's also up to you to check that the data is meaningful before throwing it into your db. You're throwing away your articles before you've even verified that you can connect to server, yet alone checked if there were any articles to retreive. Maybe this is okay, but I think it's a bug. You do the same with weather. You should also consider making sure that the values you really want to have data in them aren't null, and maybe doing some other sanity checking.
I hope this helps.
In Section
Seekers of Perl Wisdom
|
|