Thanks, Peter! 1) Yes, code is indeed inside a larger function "add staff", etc.
2) Good catch! Duh! Now the counter increments as it loops. 3) I debugged my brain about the hashes. :o) 4) e:\tempImages now has a delete cffile.serverFile after processing is complete 5) I usually do this... just got lazy. Now e:\tempImages is application.tempImagesDirectory 6) I'll have to work on this. Thanks for the suggestion! 7) Same as No. 6... I assume you're talking about "extending" methods within a component. Haven't done that, yet, but it would be good to work on. On the "to-do" list! I appreciate your feedback and that of others! Thanks, all! Rick -----Original Message----- From: Peter Boughton [mailto:[email protected]] Sent: Sunday, August 21, 2011 9:49 AM To: cf-talk Subject: Re: Anyone care to review this image handling process? I'd raise four must-fix issues with that code. 1: You haven't var/local scoped any of these variables, despite being inside a function which is probably going to end up in a shared scope, so this code isn't thread-safe and thus can cause incorrect behaviour if two people upload images at the same time. (Or, possibly, you've not copied that part of code, since there's no cffunction/cfargument lines either, but still safer to highlight the problem.) 2: The variable "loopCounter" never changes, which is probably not what you want. The whole imageList looping is broken, because the list attribute accepts a string (which is actually what you're giving it, so no errors), but the imageList variable is a query. Either do list=#ValueList(imageList.name)# or use query="imageList" instead. (Also, don't forget the filter attribute. You can use it to do "file*" as well as "*.ext" to reduce the number of files to check against) 3: You've also got incorrect hash usage, e.g: <cfset newImageFileName = reReplace(#originalImageFileName#, '[^a-zA-Z0-9_\-.]', '', 'all') /> <cfif image is '#newFileNameComplete#'> <cfset renamedNewFileNameComplete = #newFileNameComplete# > (Yes, over-use of hashes is a must-fix issue, because it indicates a lack of understanding of what's going on, so it's a bug in the programmers brain that needs fixing.) 4: As Justin points out, tempImages is going to keep growing. Depending on where the images are uploaded from, you may simply be able to FileDelete the uploaded version after the cfimage/resize has been done, and that's that. Or you could schedule a task that runs outside working hours to clear the folder, or to remove anything over X days old (if there's a reason to keep recent ones). I wouldn't pass any code without those things being corrected, but a few others which aren't specific bugs but still worth being mentioned/fixed are: 5: You haven't use a variable for e:\tempImages, which means you'd need to search/replace if/when the drive or path changes. Better to have it defined in one place, so you can simply change a single line if it does, and using a variable means any typo will be caught (whereas a typo in a directory name might be silently accepted). 6: I'd also point out that you've got an awful lot of single-use variables there. Variables are variable. You can change what they contain and don't need to keep creating new ones - you only need one or two "ImageFileName" variables there, which you mutate as you go through the code. 7: Based on all this being inside if/len/trim, I'm thinking this is just a small part of a larger function, dealing with profile changing (or something like that). If so, I definitely suggest breaking down the function into smaller self-contained sub-functions. i.e. This logic should in be a processStaffPhoto function, with relevant arguments being passed in and the final filename being returned at the end, called by whatever the larger function is. Makes things significantly easier to follow and maintain. There we go... happy to expand on anything here if you want more details. ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~| Order the Adobe Coldfusion Anthology now! http://www.amazon.com/Adobe-Coldfusion-Anthology/dp/1430272155/?tag=houseoffusion Archive: http://www.houseoffusion.com/groups/cf-talk/message.cfm/messageid:346906 Subscription: http://www.houseoffusion.com/groups/cf-talk/subscribe.cfm Unsubscribe: http://www.houseoffusion.com/groups/cf-talk/unsubscribe.cfm
