Hello and welcome to our community! Is this your first visit?
Register
Enjoy an ad free experience by logging in. Not a member yet? Register.
Results 1 to 7 of 7
  1. #1
    Super Moderator sage45's Avatar
    Join Date
    May 2002
    Posts
    1,060
    Thanks
    0
    Thanked 13 Times in 13 Posts

    My Own Coding Horrors: Goto Loops and variable obscurity; a scary combination.

    Recently, I was assigned the task of converting an in-house application to a service. Little did I realize that the original developer was fond of goto's and variable obscurity.

    Submitted for your perusal:
    Code:
    	Public Function FixImagePaths(ByVal tc As String) As String
    		Dim imgStr As String = ""
    		Dim srcStr As String = ""
    		Dim imgPre As String = ""
    		Dim fName As String = ""
    		Dim pre As String = ""
    		Dim post As String = ""
    		Dim m As Integer = 0
    
    		Dim k As Integer = 0
    		Dim i As Integer = InStr(tc, "<BASE href=")
    		If i < 1 Then Return tc
    		Dim j As Integer = InStr(i, tc, ">")
    		If j < i Then Return tc
    		Dim base As String = Mid(tc, i + 12, j - (i + 12))
    		base = Trim(Replace(base, Chr(34), ""))
    
    LoopThru:
    		'use j as new starting point each time
    		i = InStr(j, tc, "<IMG")
    		If i < j Then GoTo CleanUp
    		k = InStr(i, tc, ">")
    
    		pre = Mid(tc, 1, i - 1)
    		post = Mid(tc, k)  'was k +1
    		imgStr = Mid(tc, i, k - i)
    
    		imgStr = Replace(imgStr, "%20", " ")
    
    		i = InStr(imgStr, "src=")
    		If i < 1 Then GoTo LoopThru
    
    		If InStr(imgStr, "http://") > 0 Then GoTo LoopThru 'leave internet url's alone
    
    		j = InStr(i + 5, imgStr, Chr(34))
    		imgPre = Mid(imgStr, 1, i + 4)
    		srcStr = Mid(imgStr, i + 5, j - (i + 5))
    
    		srcStr = Replace(srcStr, "\", "/")
    
    		m = InStrRev(srcStr, "/")
    		fName = Mid(srcStr, m + 1)
    
    		tc = pre & imgPre & base & fName & Chr(34) & post
    
    		j = k + 1
    		GoTo LoopThru
    
    CleanUp:
    		Return tc
    	End Function
    Doesn't this piece of code, this veritable work of art, just make you say, "Now there is a developer that truly understand's his/her tools. I mean, it's just a model of efficiency. It is so elegant; simple and easy to understand."

    No? You don't want to say that? So, I am not the only one that thinks this quite possibly one of the worst pieces of code I have ever laid eyes on...

    So what is wrong with this code? Well, the developer has decided that the standard looping mechanisms within the language just are not good enough. They have decided that using a For( ; ; ) or For Each or While...Loop or even a Do...Loop, just are not good enough. Nope, let's use goto's. Not only that but we will use the most obscure names for our variables (a single char); lovely.

    And think of the joy I expressed when I found that his application was littered with such gems. Oh, boy, I was overjoyed.

    Well as it turns out, I ended up basically rewriting a good portion [translated as most] of the code within this application. Some portions I had to leave as is because the method to their madness eluded me.

    Ultimately my point is this, before writing anything:
    1. Understand the basic concepts of the language.
    2. Understand fully what it is your function is trying to accomplish.
    3. If your code requires documentation to help you keep track of what it is doing, then you have done something wrong.
    4. If your variables are obscure, you are bound to make a mistake (we have 5. integers, all using just one character, which one is my loop controller? i? j?)

    That all being said, here is my rewrite of the function in question:
    Code:
    	Friend Function FixImagePaths(ByVal Code As String) As String
    		If String.IsNullOrEmpty(Code) Then Return String.Empty
    		If Code.IndexOf("<BASE href=") = -1 Then Return Code
    		If Code.IndexOf(">", Code.IndexOf("<BASE href=")) = -1 Then Return Code
    
    		Dim base As String = Code.Substring(Code.IndexOf("<BASE href="), Code.IndexOf(">", Code.IndexOf("<BASE href=")) - Code.IndexOf("<BASE href=")).Replace("<BASE href=", "").Replace(Chr(34), "")
    		Dim index = Code.IndexOf("<IMG")
    		Dim preTag As String = String.Empty
    		Dim postTag As String = String.Empty
    		Dim imageTag As String = String.Empty
    		Dim fileName As String = String.Empty
    
    		While (index <> -1)
    			If Code.IndexOf(">", index) <> -1 Then
    				preTag = Code.Substring(0, index)
    				postTag = Code.Substring(Code.IndexOf(">", index))
    				imageTag = Code.Substring(index, Code.IndexOf(">", index) - index).Replace("%20", Chr(32))
    				If imageTag.IndexOf("src=") <> -1 AndAlso Not imageTag.Contains("http://") Then
    					fileName = imageTag.Substring(imageTag.IndexOf("src=")).Replace("src=", "").Replace("\", "/").Replace(Chr(34), "")
    					fileName = fileName.Substring(If(fileName.LastIndexOf("/") <> -1, fileName.LastIndexOf("/"), 0))
    					imageTag = imageTag.Substring(0, imageTag.IndexOf("src=") + (imageTag.IndexOf("=", imageTag.IndexOf("src=")) - imageTag.IndexOf("src=") + 1))
    					Code = String.Format("{0}{1}""{2}{3}""{4}", preTag, imageTag, base, fileName, postTag)
    				End If
    			End If
    			index = Code.IndexOf("<IMG", index + 1)
    		End While
    		Return Code
    	End Function
    -sage-
    Last edited by sage45; 12-04-2013 at 03:33 PM.
    HTML & CSS Forum Moderator

    "If you don't know what you think you know, then what do you know."
    R.I.P. Derrick Thomas #58
    1/1/1967 - 2/8/2000

  • #2
    Supreme Overlord Spookster's Avatar
    Join Date
    May 2002
    Location
    Marion, IA USA
    Posts
    6,278
    Thanks
    4
    Thanked 83 Times in 82 Posts
    Yikes. I feel your pain. Spent the last several months trying to understand a C code based library here at work. The person seemed to be fond of using only single letter variables, loved pointer arithmetic, didn't believe in writing any comments and couldn't seem to decide what coding style they wanted to use even though we have 1 standard here so he ends up with all manner of camelcased function names like myfunction(), MyFunction(), myFunction(), my_function(), My_Function().
    Spookster
    CodingForums Supreme Overlord
    All Hail Spookster

  • #3
    Super Moderator sage45's Avatar
    Join Date
    May 2002
    Posts
    1,060
    Thanks
    0
    Thanked 13 Times in 13 Posts
    Those are just awesome. I wanna use myFunction() for this, and then MyFunction() for that and then... Yeah I just love finding those as well. By far one of the worst ones I found was where they used multiple goto's in order to form an inner...inner...outer loop... Oh joy of joys...

    Here is another example:
    Code:
    	Public Function InsertDataValues(ByVal html As String) As String
    		Dim addSpace As String = ""
    		Dim selText As String = ""   ' frmEmail.winEd.GetSelectedText
    		If selText > "" Then
    			If selText.Substring(Len(selText) - 1) = " " Then addSpace = " "
    		End If
    		'frmEmail.winEd.InsertText(getDataValue(insTable & insField & "|" & lbxFormat.SelectedItem) & addSpace)
    
    		Dim i As Integer = 0
    		Dim j As Integer = 0
    		Dim fld As String = ""
    		Dim val As String = ""
    ReStart:
    		Dim jj As Integer = Len(html) ' <-- Completely unused declaration
    		i = InStr(html, "[[")
    		If i > 0 Then
    			j = InStr(i, html, "]]")
    			If j > 0 Then
    				fld = Mid(html, i + 2, j - i - 2)
    				val = getDataValue(fld) & addSpace
    				html = Replace(html, "[[" & fld & "]]", val)
    			Else
    				GoTo FinishUp  'this else clause is to prevent an endless loop from an occurrence of [[ with no closing ]]
    			End If
    			GoTo restart
    		Else
    			GoTo FinishUp
    		End If
    FinishUp:
    		Return html
    	End Function
    Re-write:
    Code:
    		Friend Function GetInsertFieldData(ByVal Code As String, Optional ByVal IsBirthdayTemplate As Boolean = False) As String
    			Dim index = Code.IndexOf("[[")
    			Dim preField As String = String.Empty
    			Dim postField As String = String.Empty
    			Dim doxInsertField As String = String.Empty
    			Dim value As String = String.Empty
    
    			While (index <> -1)
    				preField = Code.Substring(0, index)
    				If Code.IndexOf("]]", index) <> -1 Then
    					postField = Code.Substring(Code.IndexOf("]]", index) + 2)
    					doxInsertField = Code.Substring(index, Code.IndexOf("]]", index) - index)
    					value = GetInsertFieldValue(New InsertFieldHelper(doxInsertField), IsBirthdayTemplate)
    					Code = String.Format("{0}{1}{2}", preField, value, postField)
    				End If
    				index = Code.IndexOf("[[", index + 1)
    			End While
    			Return Code
    		End Function
    -sage-
    Last edited by sage45; 12-04-2013 at 04:05 PM.
    HTML & CSS Forum Moderator

    "If you don't know what you think you know, then what do you know."
    R.I.P. Derrick Thomas #58
    1/1/1967 - 2/8/2000

  • #4
    Super Moderator sage45's Avatar
    Join Date
    May 2002
    Posts
    1,060
    Thanks
    0
    Thanked 13 Times in 13 Posts
    This one makes me chuckle:
    Code:
    	Private Function ParseListToArray(ByVal s As String) As Array
    		Dim retList(99) As Integer
    		Dim rlCounter As Integer = -1
    		Dim i As Integer = 0
    		Dim num As String = ""
    Looper:
    		i = InStr(s, ",")
    		If i > 0 Then
    			num = Mid(s, 1, i - 1)
    			rlCounter += 1
    			retList(rlCounter) = num
    			s = Mid(s, i + 1)
    			GoTo Looper
    		Else
    			num = s
    			If Val(num) > 0 Then
    				rlCounter += 1
    				retList(rlCounter) = num
    			End If
    		End If
    		Return retList
    	End Function
    Example usage:
    Code:
    	Dim splitMe = "1,2,3,4,5,6,7,8,the,brown,fox"
    	Dim list() = ParseListToArray(splitMe)
    Can anyone tell me a simpler method? Perhaps, oh I don't know:
    Code:
    	Dim splitMe = "1,2,3,4,5,6,7,8,the,brown,fox"
    	Dim list() = splitMe.Split(",")
    -sage-
    HTML & CSS Forum Moderator

    "If you don't know what you think you know, then what do you know."
    R.I.P. Derrick Thomas #58
    1/1/1967 - 2/8/2000

  • #5
    Rockstar Coder
    Join Date
    Jun 2002
    Location
    USA
    Posts
    9,074
    Thanks
    1
    Thanked 328 Times in 324 Posts
    I once saw a bunch of C source for a AVR microcontroller and that programmer had the single letter variables going and then they also named all their functions like Func1, Func2, Func3, Func4, etc. Naturally with no code documentation.

    While at times it can be annoying I really like that at my work all code changes must be code reviewed before they can be checked in. Helps keep most of the horrors out of the code base.
    OracleGuy

  • #6
    Supreme Overlord Spookster's Avatar
    Join Date
    May 2002
    Location
    Marion, IA USA
    Posts
    6,278
    Thanks
    4
    Thanked 83 Times in 82 Posts
    Quote Originally Posted by oracleguy View Post
    I once saw a bunch of C source for a AVR microcontroller and that programmer had the single letter variables going and then they also named all their functions like Func1, Func2, Func3, Func4, etc. Naturally with no code documentation.

    While at times it can be annoying I really like that at my work all code changes must be code reviewed before they can be checked in. Helps keep most of the horrors out of the code base.
    Where I work we have a very strict peer review process and Quality checks but some of these idiots manage to fall through the cracks or find loopholes because there is no way code like this was properly peer reviewed for compliance with our coding standards.

    Code like this makes me want to find the person and smash their keyboard and fingers so they can't write any more code. They are either too ignorant to write readable code or think they are being "l33t" and writing cryptic code that they think makes themselves look smart. I was actually surprised I didn't find any gotos in this.

    Code:
    static int data_available ( hclib_ring_t *r )
    {
      int retval ;
    
      if ( r->t >= r->h )
      {
        retval = r->t - r->h;
      }
      else
      {
        retval = r->t + r->size - r->h ;
      }
    
      return retval;
    }
    Code:
    static int ring_find_crlf ( hclib_ring_t *r )
    {
      int j;
      int found;
      int avail;
      int offset;
    
      j = 0;
      avail = data_available( r );
      if ( avail )
      {
        found = 0;
    
        for ( j = 0 ; j < avail ; j++)
        {
          offset = (r->h + j) % r->size;
          if (r->buf[offset] == '\r')
          {
            if ((j+1) < avail)
            {
              j++;
              offset = (r->h + j) % r->size;
              if (r->buf[offset] == '\n')
              {
                found = 1;
                j++;
                break;
              }
            }
          }
        }
    
        if ( !found )
        {
          j = 0;
        }
      }
    
      return j;
    
    } /* End ring_find_crlf() */
    Code:
        /* Ok, there's data to be had */
        if ( r->h < r->t )
        {
          /* All the data (or all that we need) is in one chunk */
          memcpy( b, &(r->buf[r->h]), n );
          r->h = r->h + n;
          retval = n;
        }
    Code:
      for (m = 0 ; m < tablesize ; m++ )
      {
        /* Determine shortest string length */
        if ( bufsize < table[m].size )
        {
          limit = bufsize ;
        }
        else
        {
          limit = table[m].size;
        }
    
        #ifdef USING_DBUG
        dbgprintf( "parse_header_line() checking for: '%.*s' ...\n",
                   table[m].size, table[m].name );
        #endif
    
        if ( 0 == hclib_string_case_compare( (char *)*p, table[m].name, limit ) )
        {
          /* Skip until  colon */
          while ( (*n < len) && (**p != ':') )
          {
            if ( **p == '\r' )
            {
              break;
            }
            *n = *n + 1 ;
            *p = *p + 1 ;
          }
    
          /* Skip colon */
          if ( **p == ':' )
          {
            *n = *n + 1 ;
            *p = *p + 1 ;
          }
    
          /* Skip white space */
          while ( (*n < len) && ( isspace(**p) ) )
          {
            if ( **p == '\r' )
            {
              break;
            }
            *n = *n + 1 ;
            *p = *p + 1 ;
          }
    
          #ifdef USING_DBUG
          dbgprintf( "parse_header_line() found match : '%s', calling its parse function\n",
                     table[m].name );
          dbgprintf( "%d chars of string to parse are : '%.*s'\n",
                    (len - table[m].size - 3), (int)(len - table[m].size - 3), *p );
          #endif
    
          /* Call the appropriate routine */
          table[m].func(p, n, len, chan);
          break;
        }
    
      } /* for (n < RESPONSE_ARRAY_SIZE) */
    Spookster
    CodingForums Supreme Overlord
    All Hail Spookster

  • #7
    Regular Coder Nerevarine's Avatar
    Join Date
    Jan 2013
    Location
    Phendrana Drifts, Tallon IV, W-Class
    Posts
    285
    Thanks
    0
    Thanked 17 Times in 17 Posts
    Blog Entries
    3
    This thread! It hurts me inside.
    Time kills us in our sleep and we watch it happen in our dreams. -K.K.
    THE END-ALL PROGRAMMING REFERENCE: CLICK HERE (Courtesy of Major Payne)
    My username was previously L0adOpt1c. :: Please read this before posting in the HTML/CSS forum.
    Validate your HTML here and your CSS here. :: Need basic HTML/CSS tutorials? Click here, don't post about it.


  •  

    Posting Permissions

    • You may not post new threads
    • You may not post replies
    • You may not post attachments
    • You may not edit your posts
    •