Welcome to the fourth (getting bored yet?) entry in the ColdFusion contest I am running. If you haven't yet, take a look at entries one, two, and three.
This entry is a bit simpler so I won't have much to say with it. You can demo it here, and the source code follows:
Application.cfm: <cfapplication name="GuessaNumber" applicationtimeout="#CreateTimeSpan(1,0,0,0)#" sessionmanagement="true" sessiontimeout="#CreateTimeSpan(1,0,0,0)#" setclientcookies="true">
index.cfm <script> var xmlhttp=false; /@cc_on @/ /@if (@_jscript_version >= 5) // JScript gives us Conditional compilation, we can cope with old IE versions. // and security blocked creation of the objects. try { xmlhttp = new ActiveXObject("Msxml2.XMLHTTP"); } catch (e) { try { xmlhttp = new ActiveXObject("Microsoft.XMLHTTP"); } catch (E) { xmlhttp = false; } } @end @/ if (!xmlhttp && typeof XMLHttpRequest!='undefined') { xmlhttp = new XMLHttpRequest(); }
// function getfile will perform our request using our created xmlhttp object // <cfoutput> function guess(number) { var Current = "guess.cfm?guess="+number+"&RandomKey=" + Math.random() * Date.parse(new Date()); xmlhttp.open("GET", Current, true); xmlhttp.onreadystatechange=function() { if (xmlhttp.readyState==4) { document.getElementById('results').innerHTML = document.getElementById('results').innerHTML + xmlhttp.responseText; } } xmlhttp.send(null) } </cfoutput> </script>
<body>
<cfif NOT isNumeric("session.HiddenNumber")> <cfset session.HiddenNumber = RandRange(1,100)> </cfif>
<style> #results { position: absolute; width: 400px; border: 1px solid black; }
#guess { position: absolute; margin-left: 410px; width: 300px; height: auto; padding: 10px; border: 1px solid black; }
.low { color: blue; }
.high { color: red; } .win { color: purple; } </style>
<div id="results"> I am thinking of a number between 1 and 100! Guess what it is!<BR> </div>
<div id="guess"> <h1>Guess the Number!</h1> <i>Please key in the number you wish to guess, then click the link!</i> <form name="myform"> <input name="myguess"> <a href="javascript:void(0);" onclick="guess(myform.myguess.value);" onkeypress="checkenter();">Click to Guess!</a> </form> </div>
guess.cfm <cfif NOT IsNumeric(URL.Guess)> I am sorry, you must enter a number to guess. Please try again! <cfabort> </cfif> <cfset yourguess="#session.Hiddennumber - URL.guess#">
<cfoutput> <cfif yourguess eq 0> <p class="win">You got it! The number was indeed #Session.Hiddennumber#!<BR> <cfset Session.Hiddenmenu = 'done'> <a href="index.cfm">Click to Play Again!</a></p> <cfelseif yourguess LT 0> <p class="high">You guessed #URL.Guess# - that is too high!</p> <cfelseif yourguess GT 0> <p class="low">You guessed #URL.Guess# - that is too low!</p> </cfif> </cfoutput>
Here are my comments in no particular order.
He uses a simple form of AJAX, which means he loses points for applying a buzzword. (Kidding.) For a simple reload, it seems like overkill, but I kinda like it.
As I've mentioned in the previous entries, I did not expect great design, but I have been very pleased with the simple things these beginners are doing. In this demo, he uses two different colors for the messages for "Too High" and "Too Low". Simple - yet very effective.
He uses a timeout value for his application, and session, of one day. I'd say that is a bit of a security risk for his session data. Not a big huge deal, obviously, but I'd question why a session needs to stay around for a full day. I'd recommend changing it to the default 20 minute timeout unless he has a good reason.
He does validation for the guess number (yeah!), but only part way. He checks to ensure the guess is numeric, but does not check to see if it is an integer (3.14159 is numeric as well) and does not check to see that it is between one and a hundred. This brings me back to my last Macrochat about being sure to go the extra mile in validation.
Lastly, there is another bug he missed, and I missed it as well. I had noticed this block:
<cfif NOT isNumeric("session.HiddenNumber")> <cfset session.HiddenNumber = RandRange(1,100)> </cfif>And I had asked myself - why isn't this code throwing an error since he never defaulted session.HiddenNumber? It took me a minute or so before I noticed the quotes around session.hiddenNumber. He wasn't checking the variable, but the actual string. What he should have had was this:
<cfif NOT structKeyExists(session,"hiddenNumber") or not isNumeric(session.hiddenNumber)> <cfset session.HiddenNumber = RandRange(1,100)> </cfif>Bugs that don't throw errors are always the hardest to debug, luckily this one was a bit easier once my eyes actually focused. (Need...more...coffee...) I kept the isNumeric check in there since his "win" state sets the value to "done" as a way to flag index.cfm to reset the value. You could change that to simply remove the value as well as an alternative.
Archived Comments
Also - his guess.cfm should do an isdefined check on url.guess. Just noticed that.
One comment on the interface for this one. I entered a number, and hit enter. Nothing changed. I did that 3 or 4 times before clicking the "Click to guess" link and seeing something change.
From that perspective, the interface is non-intuitive. I see a form field which can be submitted by hitting the enter key. ( And hitting the enter key does submit, since the screen redraws).
If you start making guesses by clicking the link, then hit the enter key the game doesn't save state and you lose all your guesses (and I assume a new number is chosen).
Food for thought.
Oh, that's actually understandable. The original entry had text that mentioned that. It also had the persons email address. So I removed it all since I didn't think it mattered. So, pretend like it has nice text saying, "Don't just hit Enter". ;)
Yea, sorry. I did some research and found that I can put a javascript:void(0); then my ajax action, into the 'onSubmit' property of the form (duhhh) So, sorry about that, my new version can be submitted using enter and has proper field focus afterwards so you can just enter the number and hit enter over and over =)
The ajax part is nice; especially if you go there from this post, then use the demo, then want to hit your back button.. you don't have hit it 7 times.
However, the use of a link instead of a button has two problems to me.
1. the form field "infers" an http post; yet the link makes it a http get - I don't really like mixing the two up.
2. Even had the blurb been there making it so the enter key didn't work was pretty unintuitive. He should have caught the enter key to make it fire the link if nothing else. Accessibility is more than a buzzword.
He could have used a button to submit the form. Granted, in the background with the AJAX it still would have been an http get request, but at least the UI would be consistent.
Overall though I still prefer this one to the others from a user perspective simply becuase of the no refresh.
Some more nits:
1. on the text field he has a javascript function attached to the keypress event, checkenter(), either he didn't define this or you took it out - what did it do? It may negate my earlier comment
2. when I view source i see he needs to have a doctype declaration. The doctype really does matter in most modern browsers and how they render things so, even in a simple exercise, it should be there.
3. he should have his javascript block in the HEAD tag. (the head tag is missing entirely, as is the html tag and the closing body tag)
4. The style block should be moved out of the body and put in the aforementioned head tag
5. h1 tags should be used for structure on the document and not for emphasis. Emphasis is why we have CSS and the strong and em tag. Heck, em means "EMphasis"
6. since you mentioned buzzwords, and he is using AJAX I figure I can throw in another.. Unobtrusive Javscript. He should try to avoid having inline javascript thus further leaving the HTML markup as purely structural. He did a good job of using CSS classes (instead of inline) and the same could have been done for the javascript by attaching the event handlers during the body's onload event.
those are all pretty minor in the task of quickly solving your contest - but they are little things that are easy to do right and it is best to start doing them right so they become habit and thus basically no work in the future.
hope I don't come across as Anal here ;O) I just felt like doing a code review for some reason :O)
Bill:
Thank you, I do appreciate it. =) I came, over the past 3 years, from using front page, to Net Objects Fusion, to Dreamweaver, and now I use strictly CFEclipse.
I guess I take the doc declaration out because I never see that it makes much of a diff. I know I should keep the javascript seperate, that was an oops on my part for this contest =) At the point I wrote this, I did not know that the form object had an 'onSubmit' action I could tie too, I knew folks wouldnt like clicking the link (I didnt!)
I appreciate the feedback, as I really dont have anyone to 'mentor' from as far as making my code better. My friends consider my anal because I indent everything for readability, but I really want to take it to the next level and make my code poetry =)
Thanks all for the advice!