CodeSOD: Wait a Minute

Hanna's co-worker implemented a new service, got it deployed, and then left for vacation someplace where there's no phones or Internet. So, of course, Hanna gets a call from one of the operations folks: "That new service your team deployed keeps crashing on startup, but there's nothing in the log."

Hanna took it on herself to check into the VB.Net code.

Public Class Service Private mContinue As Boolean = True Private mServiceException As System.Exception = Nothing Private mAppSettings As AppSettings '// ... snip ... // Private Sub DoWork() Try Dim aboutNowOld As String = "" Dim starttime As String = DateTime.Now.AddSeconds(5).ToString("HH:mm") While mContinue Threading.Thread.Sleep(1000) Dim aboutnow As String = DateTime.Now.ToString("HH:mm") If starttime = aboutnow And aboutnow <> aboutNowOld Then '// ... snip ... // starttime = DateTime.Now.AddMinutes(mAppSettings.pollingInterval).ToString("HH:mm") End If aboutNowOld = aboutnow End While Catch ex As Exception mServiceException = ex End Try If mServiceException IsNot Nothing Then EventLog.WriteEntry(mServiceException.ToString, Diagnostics.EventLogEntryType.Error) Throw mServiceException End If End SubEnd Class

Presumably whatever causes the crash is behind one of those "snip"s, but Hanna didn't include that information. Instead, let's focus on our unique way to idle.

First, we pick our starttime to be the minute 5 seconds into the future. Then we enter our work loop. Sleep for one second, and then check which minute we're on. If that minute is our starttime and this loop hasn't run during this minute before, we can get into our actual work (snipped), and then calculate the nextstarttime, based on our app settings.

If there are any exceptions, we break the loop, log and re-throw it- but don't do that from the exception handler. No, we store the exception in a member variable and then if it IsNot Nothing we log it out.

Hanna writes: "After seeing this I gave up immediately before I caused a time paradox. Guess we'll have to wait till she's back from the future to fix it."

It's not quite a paradox, but it's certainly far more complex than it ever needs to be. First, we have the stringly-typed date handling. That's just awful. Then, we have the once-per-second polling, but we except pollingInterval to be in minutes. But AddMinutes takes doubles, so it could be seconds, expressed as fractional minutes. But wait, if we know how long we want to wait between executions, couldn't we just Sleep that long? Why poll every second? Does this job absolutely have to run in the first second of every minute? Even if it does, we could easily calculate that sleep time with reasonable accuracy if we actually looked at the seconds portion of the current time.

The developer who wrote this saw the problem of "execute this code once every polling interval" and just called it a day.

[Advertisement] ProGet's got you covered with security and access controls on your NuGet feeds. Learn more.