I was thinking back over all the coding mistakes I see people make (and trust me, I see a lot of them -- even in my own code!) and I thought, "hey, this might make for an interesting blog posting." So here it is, some of the more common coding mistakes I see people making (in no particular order).
Off-by-one
This has got to be one of the most common mistakes I've seen people make. And it's not much of a surprise that it catches people since the framework isn't terribly good at being consistent with whether it's 1-based or 0-based. So here's my little list of gotchas. Assume that every API is 0-based unless it's on this list (which I hope is comprehensive, but if I miss something, please note it and I'll update the list):- Collection.Item
- Collection.Remove
- EditableMovie.SoundTrack
- EditableMovie.Track
- EditableMovie.VideoTrack
- FolderItem.Item
- FolderItem.TrueItem
- Graphics.DrawPolygon
- Graphics.FillPolygon
- InStr
- InStrB
- Mid
- MidB
- NthField
- NthFieldB
- OLEParameter.Position
- QTUserData.GetUserData
- QTUserData.GetUserDataText
- RecordSet.IdxField
- UBound (returns the last accessible item in an array -- not a count)
When you see an API that's 0-based, you loop from 0 to count - 1. When you see an API that's 1-based, you loop from 1 to count.
Inefficient loops
I see this one come up enough that I feel the need to comment on it. When you're doing looping operations, you need to be very careful about what you do in the loop. If you're doing expensive operations such as redim'ing an array, creating memory blocks, making new instances of a class, or accessing the disk then you're going to see greatly degraded performance.[rbcode] dim redValue,px,py as integer dim pgfx as new picture( 640, 480, 32 )pgfx.graphics.drawPicture( somePicture, 0, 0 )
do
redValue = pgfx.RGBSurface.pixel( px, py ).red
select case redValue
case 100
pgfx.RGBSurface.pixel( px, py) = RGB( cr, cg, cb )
case 95
.. .
end select
loop until done[/rbcode]
Did you spot the issue? The call to RGBSurface is a very expensive one, and it's being done twice on every iteration thru the while loop. Yeeks! This code should look like this instead:[rbcode]
dim redValue,px,py as integer
dim pgfx as new picture( 640, 480, 32 )
dim surface as RGBSurface = pgfx.RGBSurface
pgfx.graphics.drawPicture( somePicture, 0, 0 )
do
redValue = surface.pixel( px, py ).red
select case redValue
case 100
surface.pixel( px, py) = RGB( cr, cg, cb )
case 95
.. .
end select
loop until done[/rbcode]
In the latter example, we've pulled the call to RGBSurface out of the loop, and so this expensive operation doesn't happen over and over and over again. Note that this sort of optimization is one that's not limited to doing Graphics operations. Any time you have a loop, the more things you can pull out of the loop, the fast it will execute.
Improper use of the Random class
I see this one a lot, and it's mostly because the documentation isn't very clear on this. For 99.9% of people out there, you only want one globally accessible Random object. The other .1% of people out there are probably doing statistical simulations and need multiple Random streams. If you make multiple Random objects and fail to set the seed for them, then there's a good chance (depending on timing) that you'll get the same set of random numbers from the generator. I see code like this a lot:[rbcode] for i as Integer = 0 to 1000 Dim r as new Random DoSomething( r.InRange( 0, 100 ) ) next i[/rbcode] Which demonstrates exactly what not to do with the Random class. So here's a little droplet of code for you -- if you use the Random class, use this instead:[rbcode] Module MyRandom Private Dim mRand as RandomProtected Sub Seed( i as Integer )
if mRand = nil then mRand = new Random
mRand.Seed = seed
End Sub
Protected Function Seed() as Integer
if mRand = nil then mRand = new Random
return mRand.Seed
End Function
Protected Function Gaussian() as Double
if mRand = nil then mRand = new Random
return mRand.Gaussian
End Function
Protected Function InRange( lb as Integer, ub as Integer ) as Integer
if mRand = nil then mRand = new Random
return mRand.InRange( lb, ub )
End Function
Protected Function LessThan( ub as Integer ) as Integer
if mRand = nil then mRand = new Random
return mRand.LessThan( ub )
End Function
Protected Function Number() as Double
if mRand = nil then mRand = new Random
return mRand.Number
End Function
End Module[/rbcode]
Once you've got this module in your project, you should replace all calls to your Random class with MyRandom -- so the previous bad code will now look like this:[rbcode]
for i as Integer = 0 to 1000
DoSomething( MyRandom.InRange( 0, 100 ) )
next i[/rbcode]
which is perfectly good code since it's only operating on a single instance of the Random class which is globally accessible. Seriously, take this concept and run with it -- it'll save you a lot of headaches and hard to track down bugs if you happen to be misusing the Random class.
Improper endianness
When you're doing cross-platform applications, endianness becomes a big liability when you're working with certain operations in your code. If you are working with files, binary data (from things like serial devices) or sockets, then you need to be keenly aware of the fact that you can't make assumptions about what endian settings various APIs are using.You should go through your code and look for any place that you use BinaryStream or MemoryBlock and make sure you're explicitly setting the .LittleEndian property on those objects. Failing to do so could mean that you're getting the improper endian setting for the platform you're application is running on, and you've got a bug. This bug will manifest itself as "When I save the file on Windows but open it on the Mac, nothing works" or "When I transfer foo from Mac to Linux, it crashes."
Failing with Permissions
And the last topic I'd like to talk about today is permissions. Many people forget about the fact that not every user's OS installation is the same as your development box. Quite often (especially in corporate settings), the user is running on a limited access account, where they've only got access to write to certain areas of the file system. One of the most common mistakes is when programmers try to write files out to the same directory the application is installed in (by using GetFolderItem( "" )). Quite often, that directory is read-only for an account, and so the application fails to run properly. When you need to write files out, there are certain places you should write different types of files to. If you need to write a file out to some place different than this list, you should take a good, hard look at whether you need to rewrite some of your code. Note, this list is for writing files out after the application is already installed. During the install process, you can place files where your application needs them and read them in just fine once the app is running.If you need to write out any preferential data to the disk (let's just ignore the fact that this data should really be in the Registry on Windows), you should be using the PreferencesFolder API.
If you need to write out any files that the application needs only needs temporarily, you should be using the GetTemporaryFolderItem API -- note that this API will return you a properly named file in the proper temp directory for the OS. It doesn't return to you the folder (for that, you should use TemporaryFolder).
If you need to write out files that are not temporary in nature, but needed for the application to run, then you should be using the ApplicationSupportFolder API. You should never be using GetFolderItem("") for this.
If you need to write out user documents, you should always default to the DocumentsFolder API since that will bring the user to the place where most of their documents are typically saved.
If you need to write files to any directory that's not listed here, all bets are off as to whether the user will have access to it. The best thing for you to do is test early and often on a machine with limited access rights. This will usually show you where you forgot about permissions issues fairly quickly.
Well, that wraps up the top five coding mistakes I see users making. Hopefully you're not making any of these mistakes, but if you are -- now you know what to go fix in your code. Good luck, and happy coding!
Quick note: ApplicationSupportFolder returns the folder inside "./Library" on Mac OS X rather than "~/Library" so although it may be useful during an install, it's really not appropriate for user-specifc application data.
About loop optimisation, do you have any hint on speed diferrences between:
for i as Integer = 0 to UBound(myArray)
Dim p as myClass = myArray(i)
// do something
next
and
for each p as myClass in myArray
// do something
next
I suspect that the later form will be a bit faster, but I didn't quantify it.
Wait, what? Really?? Oh no kidding, so it does.
Then in that case, you should use SpecialFolder.ApplicationData on the Mac instead. You can't use this one if you're working on Linux though since it returns nil.
They should be the same speed because each does the same operation. Your second snippet is simply short-hand for the first one.
Thanks so much for this post! I must say that I've run into a few of those problems myself and had to discover the solutions by research or trial & error.
Maybe a similar post about common mistakes or under/mis-used features introduced in RB2005 would be a good post as well!
@BluJai -- you're welcome! And that's not a bad idea, though I'm not certain I've really seen many new features being mis- or under-used. Or perhaps it's just a little early for me to tell.
I still say that ANY programmer of ANY application for ANY platform should be forced at gunpoint to administer an ActiveDirectory of at least 500 users for a full year. I'm sick of seeing supposedly "professional-level" software that won't work with a corporate environment... the biggest offender for me right now is Firefox. Sure, I'd love to deploy it at work... but where's the .msi installer? Where's the policies I can edit? Why does it store the cache in the wrong place? (If you use roaming profiles and Firefox, Firefox's cache will 'roam' because they put it in the wrong folder. It doesn't sound too bad until you realize that Firefox's cache could be 100+ MB easily.)
And yes, this includes games. I like running as a limited account on my home machine to prevent spyware installs, and so many video games are coded by retards that they don't work. And I'm sitting there going, "why the hell does Civilization III save in Program Files by default? WTF?!"
Anyway, enough ranting. Permissions issues piss me off. ;)
SpecialFolder does the trick - unfortunately the docs are wrong ;)
http://realsoftware.com/feedback/viewreport.php?reportid=rndeeknr
In the list 1-based, shouldn't there be left and leftb?
Hmm... left and leftb take a character (or byte) count. So if you ask for the Left( foo, 0 ) you would get "" back, and Left( foo, 1 ), you would get back the first character. So I think Right isn't 1-based by the same logic since it's the right-most X characters.
Re: the Random class. I agree with the statement that 99.9% of all users should only have a single, global random object. Since that appears to be the case, why doesn't the framework offer one? It seems like the less-yummy old Rand() call was a little better this way. Couldn't the default Application class have a Randomizer as Random property that is automatically seeded? I know, it's easy enough to add it yourself, but it seems like having it already there will pre-emptively correct that error.
We've discussed this sort of an option, and it may appear in a future release of the product.
@Brady, yes I agree, and @Aaron, great to hear it's being considered. However, instead of an "App.Randomizer" property, how about just making Random a static class as well, so we can call it like this:
dim i as integer = Random.InRange(3, 5)
App already has enough stuff without a random number generator being thrown in.
@Adam -- :: coughs :: without giving away too many details about something that's not implemented -- we've been considering that exact syntax. Except the RB term for "static class" is "module" ;-)
Please explain, i don't see the difference: try this in the action of a button, i don't see any difference in the way it works, left, mid and right all accept a zero start parameter, but all count a number of characters from a certain point, left, right or somewhere in the middle.:
dim l,r As Integer
dim s As String ="idontgetthedifferencebetweenleft-mid-right-basedzeroorone"
dim t,tl as String
t=left(s,0)
tl=str(len(t))
msgbox "left-position = 0"+EndOfLine+t+EndOfLine+tl
t=right(s,0)
tl=str(len(t))
msgbox "right-position = 0"+EndOfLine+t+EndOfLine+tl
t=mid(s,20,0)
tl=str(len(t))
msgbox "mid(20)-position = 0"+EndOfLine+t+EndOfLine+tl
t=left(s,1)
tl=str(len(t))
msgbox "left-position = 1"+EndOfLine+t+EndOfLine+tl
t=right(s,1)
tl=str(len(t))
msgbox "right-position = 1"+EndOfLine+t+EndOfLine+tl
t=mid(s,20,1)
tl=str(len(t))
msgbox "mid(20)-position = 1"+EndOfLine+t+EndOfLine+tl
Correct, and the issue isn't the count, it's the starting position. With left and right, the base (starting position) is implied as either the first character or the last character; the specified parameter is a count. The 0- vs 1-based gotcha can only happen when you're given a base to work with. The only reason I tossed UBound in the list is because people use it as a count instead of as a last index.
The code you posted isn't doing anything wrong. The first three examples are all asking for a count of 0 characters (so you'll get "" back), and the last three are all asking for a count of 1 character (so you'll get back a single character). None of them are displaying the traditional off-by-one error. Here's the "bad" example:
t = mid( s, 0, 1 )
This will return unexpected results because the second parameter (the base) is 1-based, not 0-based.
Aha, now i get your point re the mid-function. But i still don't see the difference between left and right, i guess the Arab people would with your argument put left in your list and throw right out :-D.
Sorry, I removed Right from the list because it doesn't belong there (neither does left). The reason they don't belong is because the base index is implied in the name and you only specify a count (not a starting index). Mid belongs in there because you have to specify a starting index.
o.. and they would throw out mid too, because they would read it as dim :-D
Now i see it, as usual you are right again!
Shouldn't the compiler generate an exception if the starting point in the mid-function is zero? I tried it and with a zero and even with a negative starting point i didn't get an error.
The compiler doesn't know about the mid function, or any function for that matter. So it'll never give a compiler error since that means the compiler really needs to know way too much about the framework. However, it could generate a runtime exception when the code executes -- but there may be ramifications for code relying on the old behavior, so it needs research. You could certainly request a feature that we generate an exception though.
Maybe it's a possible new subject to explain where and how the functions of RB are defined, where the parameters and the outcome of a function are declared and how they are put together in a working application.
Thanks for the explanations.
Number 1 is the 1 that vexes me the most (or is it number 0 - I forget). Is there a good reason why there isn't a standard for this? I don't mind zero-based and one-based is intuitive but *both*!!! I probably flip up the LR to remind myself of this more than anything else.
This is one of those small annoyances that I would be willing to see fixed despite the short-term havoc that it would wreak upon my code.
Serenity NOW!
~joe
@Joe -- it's basically all historical cruft and VB-compatibility. The general rule of thumb is that newer APIs are 0-based, and ancient or VB-matching APIs are 1-based. Trying to change the behavior this late in the game would cause an absolutely insane amount of code to break. For example, I shudder to think of how much of the IDE code would break; do *you* want to go flipping thru that much code trying to find every place that broke? Yeeks!
I'm probably a minority but yeah I would. Just make a little "scrubber" app that reads the xml code and looks for nthfield (and the like) and puts a -1 after it. Done and done!
~joe
Common user mistake, or common documentation mistake? Both. Let's feed this egg some more chickens.
In looking up the object types and accessors, to see whether they're 0 or 1 based, and what kind of result to expect from a UBound, etc., I came up with the following:
* Array.IndexOf (0 or 1 based?)
* UBound (Do you *always* have to add one?)
* NthField
* (+1,er,good job on Mid)
* Split (cases of missing, compounded or trailing delimited are obscure)
..the examples would be so much easier to realize if you could copy/paste them into a test app.