Hi, Kym, and thanks for the feedback. I will implement the suggestion to test "isImage". That's good advice.
Also, I like using a lot of variables, also. It makes my code easier to read and remember what it's doing over time. It could be more concise with fewer variables, but ultimately that would make it harder to work with for me. Thanks, again! Rick -----Original Message----- From: Kym Kovan [mailto:[email protected]] Sent: Sunday, August 21, 2011 9:35 PM To: cf-talk Subject: Re: Anyone care to review this image handling process? Hi Rick, not knowing what is in the rest of the CFC have you considered a malicious person uploading something other than an image beyond doing the upload to an web-unreachable temp folder? It might be worthwhile to either test the image for goodness before you copy it and/or put a try/catch round the image resize code so you pick up the bad image and can give the user some feedback or log the user as a nasty. <cfimage action="read" name="theImageObject"... <cfif isImage(theImageObject)> <cfset ImageResize("#theImageObject#",... <cfimage source="#theImageObject#" action="write"... ... <cfelse> bad person code </cfif> Regarding point 6 I like lots of variables if it makes the code easier to maintain rather than having one which changes what it is keeping depending on where you are. And verbose names as well. Kym K On 22/08/2011 10:44, Rick Faircloth wrote: > > 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:346909 Subscription: http://www.houseoffusion.com/groups/cf-talk/subscribe.cfm Unsubscribe: http://www.houseoffusion.com/groups/cf-talk/unsubscribe.cfm
