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:346894
Subscription: http://www.houseoffusion.com/groups/cf-talk/subscribe.cfm
Unsubscribe: http://www.houseoffusion.com/groups/cf-talk/unsubscribe.cfm
