CodeSOD: Rename This

Visual Basic .NET is actually not a terrible language. I don't think it'd ever be anyone's favorite language, and these days it's basically deceased, but it's essentially C# with verbose syntax. Even the APIs are identical...

... almost.

Because, you see, when Microsoft released VB .NET, their core plan was that it should be "easy" to port your existing VB6 code into VB .NET. This meant recreating a lot of static functions in the global namespace, like Mid (substring), or Kill (file delete). There were better, cleaner ways to do all of those things. For example, Kill throws an exception if you try and delete a file that doesn't exist, while File.Delete doesn't (but provides a status code).

So while you should use the newer APIs, you could still do things the old fashioned way. It was like GoTo- you shouldn't use it, but you could.

Russell F inherited some code which has all of these fun tricks.

 Private Sub Rename_OutputFile() Dim strWrkName As String Dim Counter1 As Integer 'Bob Flemp 'Set output file variable... strWrkName = Mid(strInput, 1, InStr(1, strInput, ".")) & "tmp" strOutput = Mid(strInput, 1, InStr(1, strInput, ".")) & "out" ' If Dir$(strOutput) = "" Then GoTo RenameOutput1 'tempremove Bob Flemp 'Delete Existing output file... Try Kill(strOutput) Catch End Try Counter1 = 0RenameOutput1: 'Rename temp file to output file... Try Rename(strWrkName, strOutput) Catch ex As Exception Counter1 = Counter1 + 1 If Counter1 > 10 Then If ActionCode.ToLower <> "t" AndAlso Not File.Exists(strWrkName) Then Exit Sub 'Bob Flemp - 112613 MsgBox("Cannot rename temporary output file from" & strWrkName & " to " & strOutput & "." & ControlChars.CrLf & "Reason is: " & ex.ToString) Else System.Threading.Thread.Sleep(100) GoTo RenameOutput1 End If End Try Log("leaving Rename_OutputFile") End Sub

"Bob Flemp" helpfully commented the lines he changed. Sometimes with just his name, sometimes with the date. The team was using source control, so none of that was necessary.

The pattern here is that they first do some string munging to get the paths arranged correctly. Then they try and delete the file at the destination, if it exists, catching any exceptions in that process. It's worth noting that there are many possible errors arising from deleting a file, not just that it didn't exist, but we just ignore them all. Plus, as deployed, the file being deleted should almost never exist, so almost all of the exceptions being thrown would be real exceptions that we need to handle properly.

Note also the commented out Dir$ call, which enumerates all the files matching the input pattern- this is a vestigal check to see if the output file exists before deleting it. This was probably a better choice than the catch-all exception block, which adds the frustration of seeing that they had a better thought at one point, but changed their minds. Then again, it also uses GoTo for control flow, so let's not credit them too much.

We get our block to actually do the renaming, complete with a retry counter and a loop implemented in GoTos. If, after ten attempts, we fail to rename, we either exist the subroutine without any hint of what just happened, or we pop up a message box explaining the error. I'm sure this never confuses any users.

It's worth noting that retrying the move every tenth of a second for one whole second is probably not going to resolve whatever locking issue prevented the rename in the first place. In the end, it's just a nice convenient way to waste some time before popping up an error- but time that other parts of the program might be depending on.

Russell writes:

I was allowed to refactor it, but I was too afraid to really change the basic structure (this code is such a mess that it may break a lot of other stuff) so my refactored version isn't really much better.

 Private Sub RenameOutputFile() Dim basePath As String = BaseAuth() Dim srcPath As String = basePath & "tmp" Dim destPath As String = basePath & "out" For i As Integer = 1 To 10 Try File.Delete(destPath) File.Move(srcPath, destPath) Return Catch If i = 10 Then Throw Else Threading.Thread.Sleep(100) End If End Try Next End Sub

It's easier to read, at least, but the basic stupidity is still there.

[Advertisement] Otter - Provision your servers automatically without ever needing to log-in to a command prompt. Get started today!