CodeSOD: This Event is Quite the Do

Benjamin inherited some code from a fellow developer. The original author of this code wrote a lot of code for the company, and that code drives a lot of products which make the company piles of money. Tired of making money, that developer left to go open a restaurant.

Which means Benjamin is now responsible for maintaining code which lives in 15,000 line files where class-level variables are essentially treated as globals. There's also a lot of misunderstandings about how Windows Forms GUIs work.

For example:

' Delay to give the GUI CPU timeSystem.Threading.Thread.Sleep(DevSetup.SleepTimeMilliSeconds)

By default, Windows Forms applications are single-threaded. This can create problems: if your application logic is busy with an expensive calculation, or waiting on a network resource or whatever, you can easily block the GUI from updating while that process is running. The solution is to push the expensive operations into an asynchronous context, letting the GUI do what it needs to do.

Putting the current thread to sleep in a single-threaded application doesn't give the GUI any CPU time.

There's a different option, in Windows Forms, that's essentially a hold-over from the old VB6-style GUIs: DoEvents. If you're performing a long, complex operation, you can periodically call DoEvents() from within that operation, which pauses your code and hands control back to the GUI. This lets the GUI repaint, for example. In a modern context, there are cautions about actually using it.

Benjamin found a line which looks like this:

If Screen IsNot Nothing Then DisposeOf(Screen)

That, itself, doesn't look bad. If the Screen object isn't null, dispose of it. Fair.

Public Shared Sub DisposeOf(ByRef Obj As Object) If Obj IsNot Nothing Then Obj.dispose() Obj = Nothing Doevents() End IfEnd Sub

The implementation of DisposeOf starts to look a little worrisome. First, we're repeating the null check, which implies we didn't need to check before. We're also doing the Obj = Nothing which is definitely a holdover from VB6, where you couldn't rely on objects being released automatically. .NET has proper garbage collection, so that's unneccessary.

But then there's a call to Doevents. And that's worrying. First, this isn't getting called from inside a long operation where we might need to worry about letting the GUI repaint. Second, this makes a theoretically pretty generic "safely dispose even if things are null" method pretty specific to a GUI context (and yes, .NET has nullable types, so you don't need the null check at all).

And wait, Doevents all lowercase? That's not the real, .NET built in method? What are they doing?

Public Sub Doevents() Application.DoEvents() Application.DoEvents() Application.DoEvents()End Sub

Oh no.

[Advertisement] Forget logs. Next time you're struggling to replicate error, crash and performance issues in your apps - Think Raygun! Installs in minutes. Learn more.