5 Replies - 1286 Views - Last Post: 18 April 2013 - 06:11 PM Rate Topic: -----

#1 lordofduct  Icon User is offline

  • I'm a cheeseburger
  • member icon


Reputation: 2533
  • View blog
  • Posts: 4,633
  • Joined: 24-September 10

I just can't tell what is going on here with some of this...

Posted 11 April 2013 - 02:05 PM

So I've been working on a a conversion of a HUGE piece of software from vb6 to vb.net. This software was converted to vb6 from previous code base running on AIX machines and dates back to like 1982 or so.

Over the years several different... people (I don't want to call them programmers), have written and maintained the code. This has resulted in dumb ass shit. Stuff I didn't know was even possible. Worse is that the first pass of the conversion was performed by an automated tool and a couple of intern level programmers who just blindly transposed the code from vb6 to vb.net. I'm not going to highlight oddities that were introduced by either of these though as that's just to be expected. No the code I'm going to show is legit code coded by supposed professionals.

This is one of my favs...

        Dim stemp As String = ""

        Dim key As Integer
        For i As Integer = 1 To Strings.Len(sval)
            key = Asc(Strings.Mid(sval, i, 1))
            If key >= 97 OrElse key <= 122 Then key = key - 32
            stemp = stemp & Chr(key)
        Next

        sval = stemp



This would be just copy and pasted into sections of code when it had to be called. Not say wrapped into its own function that could be called or anything... nope, just thrown mid block of 800 lines of rambling code.

If you don't notice what's going on... I'm not surprised... you probably don't have the ASCII code sheet memorized. I only recognized because I happened to have looked at one earlier that day and went "wait a second... 97, that's 'a'..."

Yes, that's a 'ToUpper'... a function that already exists in vb.



Another one I love seeing is the lack of consistency. This will happen one right after the other. Like maybe it was written by two different people, both of whom didn't bother to look around themselves and notice that the previous line did the same thing just differently.

        If Len(Trim(sTemp1)) >= 0 Then
            ''do stuff
        End If

        If Trim(sTemp2 & "") <> "" Then
            'do stuff
        End If






But today, today I struck gold. I found the line of code that made me have to post here. Now note there is a util class that this project has that contains functions to manipulate strings. One of them is 'PadWithZeros', it takes in a string, a length, and an alignment value. It then pads zeros on the front or back of the string until the string is the length passed in. Pretty simple function, with so much potential! Like this gem:

Dim sval = StringUtil.FormatValue(CDbl(aRowArray.GetValue(lRowNum)) / CInt("1" & StringUtil.PadWithZeros("", 2)), StringUtil.FRM_AMT2)



Let me just focus in on that, to really show you the glory of it.

CInt("1" & StringUtil.PadWithZeros("", 2))



...

A part of my brain just ran down my nose.

Is This A Good Question/Topic? 3
  • +

Replies To: I just can't tell what is going on here with some of this...

#2 jon.kiparsky  Icon User is online

  • Pancakes!
  • member icon


Reputation: 7744
  • View blog
  • Posts: 13,083
  • Joined: 19-March 11

Re: I just can't tell what is going on here with some of this...

Posted 11 April 2013 - 02:14 PM

The hilarious part is they always think they're going to write something that just runs rings around the library functions...
Was This Post Helpful? 0
  • +
  • -

#3 BobRodes  Icon User is offline

  • Your Friendly Local Curmudgeon
  • member icon

Reputation: 574
  • View blog
  • Posts: 2,989
  • Joined: 19-May 09

Re: I just can't tell what is going on here with some of this...

Posted 16 April 2013 - 09:59 PM

Quote

If key >= 97 OrElse key <= 122 Then key = key - 32

But...but...hmm. I believe I've hit on a simplification: key -= 32

Hope that helps...

This post has been edited by BobRodes: 16 April 2013 - 10:00 PM

Was This Post Helpful? 1
  • +
  • -

#4 AdamSpeight2008  Icon User is offline

  • MrCupOfT
  • member icon


Reputation: 2262
  • View blog
  • Posts: 9,464
  • Joined: 29-May 08

Re: I just can't tell what is going on here with some of this...

Posted 17 April 2013 - 07:38 AM

It also has an error if it is meant to be a bounds check.
If key >= 97 OrElse key <= 122 Then key = key - 32


because as written it is reducing the key's value by 32 for all value other than the key value that cover the lower case letters.

should really be

If (97 <= key) AndAlso (key <= 122) Then key -= 32


Was This Post Helpful? 0
  • +
  • -

#5 lordofduct  Icon User is offline

  • I'm a cheeseburger
  • member icon


Reputation: 2533
  • View blog
  • Posts: 4,633
  • Joined: 24-September 10

Re: I just can't tell what is going on here with some of this...

Posted 18 April 2013 - 06:58 AM

Thank you guys for cleaning up the bugs in that code.

It was a lot of help! Now I can go on happy knowing that this code isn't all hackish anymore.
Was This Post Helpful? 0
  • +
  • -

#6 BobRodes  Icon User is offline

  • Your Friendly Local Curmudgeon
  • member icon

Reputation: 574
  • View blog
  • Posts: 2,989
  • Joined: 19-May 09

Re: I just can't tell what is going on here with some of this...

Posted 18 April 2013 - 06:11 PM

View PostAdamSpeight2008, on 17 April 2013 - 09:38 AM, said:

It also has an error if it is meant to be a bounds check.
If key >= 97 OrElse key <= 122 Then key = key - 32


because as written it is reducing the key's value by 32 for all value other than the key value that cover the lower case letters.

should really be

If (97 <= key) AndAlso (key <= 122) Then key -= 32


My explanation was more concise. Funnier too. :P

Wait a minute. I only read the correction first time through. As written, it is reducing the key's value for ALL values, is it not? Any number is either greater than 97 or less than 122, right?

This post has been edited by BobRodes: 18 May 2013 - 10:16 AM

Was This Post Helpful? 0
  • +
  • -

Page 1 of 1