Posted in ColdFusion | Posted on 06-13-2007 | 1,678 views
Welcome to the second to last entry in my ColdFusion Newbie contest. I should be hitting up the last one tonight and announcing a winner tomorrow or Friday. These last two took a while because I wasn't able to get them running locally. Now normally that would be a disqualification. But - as both of these submissions had online versions, I decided to give them a review and include them in.
The first entry is Big Fat Monster from Nick Sweeney. The link in the previous sentence goes to the online version of the game. As with some of the other entries, the design is really nice. I hope these guys aren't cheating and using other designers. (And I swear I'm not jealous. Really.) You will have to register first and then log in. Once you do, you can create a monster with a few basic skins. There is a "Hall of Fame" on the home page which is pretty darn cool. Another cool feature is that your monster apparently gets into battles by itself while you are out. Now my list of "while you were out" battles included battles I actually participated in, so I'm not sure how that feature works. Once working with your monster, you have a pet, feed, nap, and fight option. The fighting is rather simple. You select an enemy and cross your fingers. There is also a cute "mBay" button to buy items, but I never saw any items there. (So it turns out this is a browser issue. I did a view source and saw items, but I never saw them in Firefox. I bet it works for IE. Now Nick definitely can't win! ;)
When registering, I noticed that JavaScript was used to validate the form. What is the first thing we when we see a JS checked form, boys and girls? Turn off JS of course. This not only led to an error, it led to an error that revealed the code. This shows two problems here. First - no server side validation, and secondly, the 'Enable Robust Exception' information was turned on. Actually there are three problems - there was no general error handler defined for the application. The last two items could be fixed in about five minutes and regular readers of my blog know that I tend to harp on these subjects.
Digging into the code - I first noticed a full path in Application.cfm. This was a big problem in entry 7, so I've been keeping an eye out for it in other entries. As a reminder - anytime you hard code a path in your code your making it more difficult for you to move the code in the future. Keep that in mind.
Moving into index.cfm, I immediately see this:
2 Component="Battles"
3 Method="listBattles"
4 ResultsLimit="50"
5 Returnvariable="BattleList">
Which makes me wonder why an Application scoped CFC isn't being used instead. The CFC in question also lacks var scoping and output control. Although the var scope issue may be less of a problem since his CFC isn't cached, it is still something that can be fixed. I've been complaining about this in almost all the entries, but here is a quick example so you can see exactly what I mean:
2 name="TodaysBattles"
3 returntype="query"
4 hint="Returns Todays Battles">
5
6 <cfargument name="ForMonster" required="yes" default="">
7 <cfset BattleDay = DateAdd("d",-1,Now())>
8 <cfquery name="qryGetBattles" datasource="#REQUEST.DataSource#">
9 SELECT BigFatMonsterBattles.*
10 FROM BigFatMonsterBattles
11 Where BigFatMonsterBattles.AttackerID = #ARGUMENTS.ForMonster#
12 AND BigFatMonsterBattles.BattleTime >= #BattleDay#
13 ORDER By BigFatMonsterBattles.BattleTime DESC
14 </cfquery>
15
16 <cfreturn qryGetBattles>
17</cffunction>
In this case, all that is needed is to add two var statements. I'm also going to restrict the output and use cfqueryparam:
2 name="TodaysBattles"
3 returntype="query"
4 hint="Returns Todays Battles" output="false">
5
6 <cfargument name="ForMonster" required="yes" default="">
7 <cfset var qryGetBattles = "">
8 <cfset var BattleDay = DateAdd("d",-1,Now())>
9
10 <cfquery name="qryGetBattles" datasource="#REQUEST.DataSource#">
11 SELECT BigFatMonsterBattles.*
12 FROM BigFatMonsterBattles
13 Where BigFatMonsterBattles.AttackerID = <cfqueryparam cfsqltype="cf_sql_integer" value="#ARGUMENTS.ForMonster#">
14 AND BigFatMonsterBattles.BattleTime >=<cfqueryparam cfsqltype="cf_sql_timestamp" value="#BattleDay#">
15 ORDER By BigFatMonsterBattles.BattleTime DESC
16 </cfquery>
17
18 <cfreturn qryGetBattles>
19</cffunction>
Another problem I saw - and this really speaks to a common theme I'm seeing in this entry - is in monster_list.cfm. This is the main file run when working with your monster. In order to use this page you must be logged in. His code though does something like this:
2lots of code
3<cfelse>
4link to let the user log in
5</cfif>
This is a mistake to me. First off - the Application.cfm file really should be handling this logic. Secondly, from just a readability standpoint, if I see a file that is 95% CFIF block then I see a problem. In general I try to avoid files like that. If I can't use Application.cfm (the CFIF may not be security related), I'll swap my CFIF logic to something more like this:
2blah
3<cfabort>
4</cfif>
5rest of page
One cool thing - the cfchart usage. This was also done well in entry 7 and I think serves as a reminder about how nice this feature is. As far as I know it wasn't updated for ColdFusion 8, which is unfortunate, but it is pretty powerful as is.
I was rather surprised by the code in monster_battle.cfm. I encourage folks to take a look at it. Apparently Nick used formulas from Diablo II and the code is an interesting read. I find it odd that he mixes in queries and CFCs calls though. This should all be in a CFC - but check the file out. It is definitely worth the time.
One big issue in monster_battle.cfm - notice that the monster value is passed in via URL parameter. Guess what happens if you change the value? No validation. I was able to make the code throw an error by just changing the ID to 1.
The last thing I'll point out is monster_action.cfm. This is the file that processes your actions. One of the things I noticed was a status code for the monster. So for example, if the status of your monster was 90-99, it meant it was dead. This leads to code that looks like so:
2<cfif #ThisMonster.MonsterStatus# LTE 89>
Can folks see the problem here? It isn't so much that a number is being used - but rather a hard coded number. I would have rewritten this more like so:
2<cfif thisMonster.isAlive()>
Abstract away the "is alive" logic. This is especially important since he repeats the logic multiple times on the page, and I'm sure throughout the application.
In your example what if I somehow got around the 'not X' block, but tampered with the rest of the variables incoming? the rest of the page would run with those tampered variables whereas his version would not because it is locked down tighter to a point. I've personally seen this exact scenario in something I inherited =\
First of all – I am a designer learning to be a programmer – so I am glad you liked the design! But, Admittedly, I have a long ways to go on the coding side… (Basically, learning to program out of necessity.)
FYI for everyone – the dload file includes a lot of stuff that isn’t technically part of My Big Fat Monster. So, like the full path in the application.cfm – it isn’t used. (legacy stuff on my dev site) Sorry.
For the error handling:
I get the server side issue (I need to check for “empty” form variables, and return errors, right?) But what do you do if you can’t turn off 'Enable Robust Exception'? And what do you mean by general error handler? Incorporating cftry and cfcatch tags? (Want to make sure I understand this…)
Thanks for the example on the var scopes - - I have been struggling to get my head around it, and as someone has pointed out – I have been thinking too hard. It’s simpler than I think – I appreciate being able to see how I could/should have used it in my example.
For “Check Login”
If you place that in the application.cfm doesn’t that mean you have to be logged in to view every page? What if you want everyone to be able to see certain parts of the pages but not the “secure” portions?
Notes on Monster Status
I wanted to be able to give the monsters various states –presumably stored in a db or something– and not just alive and dead – but different forms of death and life. Unfortunately, I never got my head around how that could work without testing the numbers... (Is he alive, but sleeping?) Broken down, Status is on a scale of 1-100, with 90-99 being different deaths. So, for Example: Murdered in battle (92) is a different state than committed suicide (91) but both dead. And I wanted to do different actions and show different graphics based on that. (The image naming convention works like that (monster_#MonsterType#_MonsterStatus#.jpg) I just never finished thinking it out in time – and spent most of my efforts on the Monster Battle logic. (Which was tricky and yes – should probably be in a CFC – but as I stated – these are the first CFC’s I have ever written.)
Which brings me to the IE only… Yeah – I only had time to test in IE. The only defense I have there is – 90% of my clients websites have IE only visitors and so I have that as my default browser. I plan to make it browser compatible – someday. And yeah – I know everyone hates IE – but it’s what “the public” uses – bad or good. So, I usually start there and work backwords. Yes – backwords. :(
if(x)
lots of code
I'd do
if(!x) abort
To me it is a readability issue.
As for getting around a !x block... well, you have to be responsible for your code. If you can if(x), you darn well better know how to do !x. ;)
Justice: Good catch.
@Nick: If you can't turn off robust error handling, just add a general error handler with either onError, or cferror. You can block all errors from showing up in about 2 minutes with these functions/tags. Check the docs for more info (or my blog, I tend to harp on this subject).
check login: That is a good question. In the past I've done a few things. I've used a file naming scheme (all secure files are sec_), I've used sub folders (anything in /secured is secured), I've used just hard coded lists (ie, if request is for an item in this list, it isn't secure). As DK said, you do want to also keep the logic in the page itself. There is nothing wrong with being anal when it comes to security. My point had been that I just didn't like the big CFIF block around most of a whole template.
It's ok on the IE thing - I was just giving you a hard time. ;)