ColdFusion Contest Entry Examined
The ColdFusion contest is now closed, and I am beginning to look at the entries. As I mentioned, I was going to pick the code apart and point out mistakes. This is not to make anyone feel bad or to make people think I don't make mistakes (anyone remember the supreme whopper?), but as readers of my blog know, I love to point out mistakes and issues because I think it helps people learn. Also, I like to point out alternative ways of coding functionality, just in case you don't know such methods exist. So, enough gab, let's get on with it.
The first entry can be viewed here. Play with it a few times before going on to see if you find the same issues I did.
Now that you have tried it out, let's take a look at the code:
<cfset VARIABLES.thisPage = GetFileFromPath(GetTemplatePath())>
<cfset VARIABLES.lownumber = 1>
<cfset VARIABLES.topnumber = 100>
<cfparam name="FORM.guess" default="">
<cfparam name="FORM.guesses" default="">
<cfparam name="FORM.thenumber" default="#RandRange(lownumber,topnumber)#">
<cfparam name="VARIABLES.message" default="">
<cfif FORM.guess IS NOT "">
<cfset FORM.guesses = FORM.guesses & FORM.guess & ",">
<cfif FORM.guess IS FORM.thenumber>
<cfset VARIABLES.message = "That's it, good job! It took you #ListLen(FORM.guesses)# tries">
<cfelse>
<!--- let's see where the guess is in relation to the number --->
<cfif FORM.guess GT FORM.thenumber>
<cfset VARIABLES.message = "Your guess is too high">
<cfelseif FORM.guess LT FORM.thenumber>
<cfset VARIABLES.message = "Your guess is too low">
</cfif>
</cfif>
</cfif>
<cfoutput>
I'm thinking of a number between #VARIABLES.lownumber# and #VARIABLES.topnumber#.<br>
Care to guess what it is?<br>
<cfif FORM.guess IS NOT "">
<b>You guessed #FORM.guess#. #VARIABLES.message#</b><br>
So far you've guessed: #FORM.guesses#
</cfif>
<form action="#thispage#" method="post" style="width: 200px; text-align: right; ">
<input type="hidden" name="thenumber" value="#FORM.thenumber#">
<input type="hidden" name="guesses" value="#FORM.guesses#">
I think it's: <input type="text" name="guess" size="3" value="#FORM.guess#"><br>
<input type="submit" value="take a guess"><br>
<input type="button" value="try again" onClick="location.href='#thispage#';">
</form>
</cfoutput>
There are some interesting techniques in here that beginners should be aware of. First off, note how the author created a variable, thispage, to stand for the current page. He uses that variable later on in the action attribute of the form. This is a great idea, as it means you can rename the file without having to worry about the form action. I will point out one thing I'd change, though. Instead of:
<cfset VARIABLES.thisPage = GetFileFromPath(GetTemplatePath())>
I'd use:
<cfset VARIABLES.thisPage = cgi.script_name>
This will return a slightly different value. Instead of just "foo.cfm", you will get the full path, "/moo/goo/foo.cfm". Both work of course, but the cgi variable involves a bit less code.
Edited: Well, it figures that if I wrote a post pointing out mistakes in code, I'd make a mistake myself. Barney points out in the comments that cgi.script_name will have issues on a server with a context root that isn't /. Turns out, the authors solution was better than my solution.
On a style note (not that I'm a style nazi), I noticed the author used the variables scope prefix, but forgot to use it again in both the form tag, and the "Try Again" button. In general, I do not use specify "Variables" when using the Variables scope, but if you do, you should try to use it consistently. Again, this isn't a huge deal, but something to keep in mind. Let's move on.
Notice what happens when you make a few mistakes? The author tracks your guesses, which is a nice touch, but the string is a bit messed up. Instead of seeing a list like "50, 75", you have "50,75,". Notice the trailing comma? This comes from this line:
<cfset FORM.guesses = FORM.guesses & FORM.guess & ",">
What should he have used? The listAppend function:
<cfset form.guesses = listAppend(form.guesses, form.guess)>
ListAppend will automatically add the comma (or whatever delimiter) before adding the new entry.
Another mistake - when the user finally picks the winning number, the "take a guess" button still shows up. This could be fixed by simply checking to see if the user has "won", and if so, hide the button.
So - now lets tackle the bad mistake. Have you guessed it yet? The code the author wrote does not make use of session or other RAM based variables. Instead, it keeps the state of the game in the form itself. That by itself isn't so bad, but the actual winning number is actually in the form. I discussed stuff like this in my last Macrochat. Basically, you can't trust hidden form fields since they really aren't hidden. I don't mean to beat up on the author because security wasn't really important to this contest, but I want to use it as a warning to my readers in general. Always think about security. Never stop. Period! So, with that yelling out of the way (grin), what are some ways he could have gotten around it? Well, the easiest solution would be to move to RAM based variables and store the right answer (along with the guesses) in the Session scope. But let's say, for whatever reason, that isn't an option. There are a couple of things you could do. You could encrypt the value. It would still be visible to the user, but in encrypted form. Encryption is not perfect, but you need to weight the costs and benefits. If this game were for real money, then maybe that solution wouldn't work. But if it was just for fun, than it would be acceptable. Another option - create a UUID, which is a unique number, and store the right answer in a database table. The form would have the UUID, and you would simply look up the answer based on the UUID from the form. If the user changed the value for some reason (yes, Virginia, users can change form values if they want to be nasty), then the lookup would fail. You could log it, or simply generate them a new UUID. Do folks have other ideas?
So, thats it for the first entry. I have 5 or 6 more to go through before I pick a winner. Hopefully they will all be as interesting, and the author should feel proud. These are all "normal" mistakes, not "Oh my god, we hired a muppet" mistakes.
Comments
[cfset req = getPageContext().getRequest() /]
[cfset thispage = "#req.getContextPath()##req.getServletPath()#" /]
I _knew_ if I wrote a post pointing out mistakes I'd make my own. ;)
I'm a beginner myself but I'd change a few things anyway :-).
- You should always initialize the random engine whenever using a random function
- why have
<cfif>
<cfelse>
<cfif>
<cfelseif>
</cfif>
</cfif>
instead of a standard <cfif><cfelseif><cfelse></cfif> ?
-instead of FORM.guess IS NOT "", use len(form.guess), it just looks nicer :-)
-why is there a submit and a button, instead of 2 submits ?
And that's all, correct me if I'm wrong :-)
Thank you for examining my code. You noticed some things that I didn't think about, but also commented on some things that I left out purposefully.
I intentionally didn't store the information in session variables because in this case, I didn't want to have to enable them via an application.cfm file, and frankly, I liked the elegance of just one page.
Also, i never use the VARIABLES scope either. I only did it this time because I thought you'd comment on it if I DIDN'T. So that's funny. I will pay more attention to consistency in scoping though so thank you for pointing that out.
About the #thispage# variable, I spent quite a bit of time playing with functions and cgi variables to get what I wanted. I only wanted that variable to return the actual filename. Then I would never have to fiddle with trimming out other stuff from it and it will always work as is. That goes in every application file I use, it's so handy.
Thanks for pointing out the ListAppend function. There are LOTS of cases where I use my manual method so ListAppend will be much better solution!
Some of the people in my Nashville CFUG pointed out the form security issue as well. Note that I would never store important information like this in a form field in a real life situation. They also pointed out that I should have hidden the form once the person won.
All in all, I appreciate you looking over my code. I learned a few things, and maybe other's will to.
Shout outs to the Nashville CFUG (www.ncfug.com).
The reason there is a submit and a button is that the "take a guess" submits the form, which triggers some code, whereas the "try again" button simply restarts the game.
After you've reviewed all of the entries I think you should have another contest for advanced CF programmers. Tell them to write the code as if someone's life depended on it. I'd love to see those programs picked apart. DepressedPress.com used to have contests to see who could write the fastest algothims for things like sorting a query. It was a lot of fun and a great learning experience.