Results 1 to 23 of 23

Thread: [RESOLVED] Advise on Function's Intent

  1. #1

    Thread Starter
    Super Moderator dday9's Avatar
    Join Date
    Mar 2011
    Location
    South Louisiana
    Posts
    11,711

    Resolved [RESOLVED] Advise on Function's Intent

    I am still working on a legacy conversion (and I likely will for a while).

    The legacy code has the following function defined:
    Code:
    Function FormatField(ByVal field)
        Dim i%, s%
    
        s = ""
        For i = 1 To Len(field)
            If InStr(" -", Mid$(field, i, 1)) = 0 Then
                s = s + Mid$(field, i, 1)
            End If
        Next
        FormatField = s
    
    End Function
    Following the code it appears that it does the following:
    1. Declare a new integer and string variable named i and s respectively
    2. Set the default value of s to an empty string
    3. Loop from 1 to the length of the parameter
    4. Check if the string literal " -" is not in the String
      1. If it is not, then append the currently iterated character to the variable s
    5. Return the variable s


    To me, functionally this looks like all it is doing is replace the string literal " -" with an empty string. Would this function be the same as:
    Code:
    field = Replace(field, " -", "")
    Or am I missing something?
    "Code is like humor. When you have to explain it, it is bad." - Cory House
    VbLessons | Code Tags | Sword of Fury - Jameram

  2. #2
    Smooth Moperator techgnome's Avatar
    Join Date
    May 2002
    Posts
    34,531

    Re: Advise on Function's Intent

    *facepalm* That's in fact what it appears to be doing... that means at some point, some user entered something like "1-2-3-4" into a field expected to be "1234" ... and it caused an error. So of course the solution was to "check each character in the input and make sure it isn't '-'" ..... of course now, that would cause someone like me to try "1+2*3/4?5#%6" and see what happens... Jsut looks like a bad solution....

    OR.... unless it's for de-formatting, say a phone number where user are always entering 800-555-1212 .... that would reduce it to "8005551212" for storing... either way, yeah, it looks like it's doing a replace on the "-" char to empty string.


    -tg
    * I don't respond to private (PM) requests for help. It's not conducive to the general learning of others.*
    * I also don't respond to friend requests. Save a few bits and don't bother. I'll just end up rejecting anyways.*
    * How to get EFFECTIVE help: The Hitchhiker's Guide to Getting Help at VBF - Removing eels from your hovercraft *
    * How to Use Parameters * Create Disconnected ADO Recordset Clones * Set your VB6 ActiveX Compatibility * Get rid of those pesky VB Line Numbers * I swear I saved my data, where'd it run off to??? *

  3. #3
    PowerPoster
    Join Date
    Feb 2012
    Location
    West Virginia
    Posts
    14,205

    Re: Advise on Function's Intent

    Actually it looks like it is removing removing spaces and dashes if they exist. It is not looking for both together as it is only looking at 1 character at a time but if that character is in the string " -" then it is effectively removed. So if the string a "abc-efg 123" the result would be "abcefg123"

  4. #4
    PowerPoster
    Join Date
    Aug 2010
    Location
    Canada
    Posts
    2,412

    Re: Advise on Function's Intent

    I think you are missing something - looks like it is removing all " " and "-" characters from the string, so:

    Code:
    Function FormatField(ByVal RawField As String) As String
        FormatField = Replace$(RawField, " ", "")
        FormatField = Replace$(FormatField, "-", "")
    End Function
    But there is a lot wrong with the initial function, so it's hard to know with 100% certainty. For example "s" is declared as Integer, not String so an error will be raised at the s = "" line. Other problems include using Variants, using + for string concatenation instead of &, and using a loop where the Replace$ would be appropriate.

    That said, it looks like what it is doing is looping through each character of the passed "field" variable, and see if the character is found in the " -" string (so either a space or a minus). If so, skip that character. If not, concatenate the character to the result string. When done, return the result string.

  5. #5

    Thread Starter
    Super Moderator dday9's Avatar
    Join Date
    Mar 2011
    Location
    South Louisiana
    Posts
    11,711

    Re: Advise on Function's Intent

    That's my fault for not being familiar with the InStr function because y'all are right with it removing both hyphens and spaces. I just tested it in this fiddle (slightly modified for .NET syntax): https://dotnetfiddle.net/onmZwL

    It still seems like the same thing could be done using multiple replaces like in jpbro's example.

    I just wanted to make sure that there wasn't anything else going on here that I was missing. Thanks y'all.
    "Code is like humor. When you have to explain it, it is bad." - Cory House
    VbLessons | Code Tags | Sword of Fury - Jameram

  6. #6
    PowerPoster
    Join Date
    Feb 2006
    Posts
    24,482

    Re: Advise on Function's Intent

    Sounds like somebody was trying to "cave man around" using the MaskEdit control for input. Probably one of those Mort coders from the VBA world who didn't know it exists.

  7. #7
    PowerPoster
    Join Date
    Feb 2017
    Posts
    4,995

    Re: Advise on Function's Intent

    Yes, it is removing spaces and "-", but the s% should be s$ to be declared as String.

  8. #8
    The Idiot
    Join Date
    Dec 2014
    Posts
    2,721

    Re: Advise on Function's Intent

    looking at the code, I would change it to:

    Code:
    Function FormatField(ByVal field As String) As String
        Dim i As Integer, c As String
    
        For i = 1 To Len(field)
            c = Mid$(field, i, 1)
            If c <> " " And c <> "-" Then FormatField = FormatField & c
        Next i
    End Function
    if you want to use something else instead of Replace and as close possible to the original, but a bit easier to understand.
    Last edited by baka; Aug 24th, 2020 at 05:19 PM.

  9. #9
    PowerPoster Zvoni's Avatar
    Join Date
    Sep 2012
    Location
    To the moon and then left
    Posts
    4,418

    Re: Advise on Function's Intent

    Could someone please explain, how this:
    Code:
    If InStr(" -", Mid$(field, i, 1)) = 0 Then
    is ever supposed to be true?
    I'm looking for a 2-character-long Substring in a 1-character-long returnvalue from mid$?
    I was always under the impression, that InStr searches for the Substring "as is", in that case a SPACE followed by a DASH, not either/or

    EDIT: Forget what i said. i just embarassed myself
    For the life of me, i can never remember with InStr, that String1 is the string to search in, and String2 is the string i'm looking for in String1

    EDIT2: Since everyone is pitching in, here is my solution
    Code:
    Public Function FormatField(ByVal Field As String, Optional ByVal IllegalChars As String = " -") As String
    Dim i As Long
        FormatField=Field
        For i=1 To Len(IllegalChars)
            FormatField=Replace(FormatField, Mid$(IllegalChars, i, 1), vbNullString)
        Next
    End Function
    Last edited by Zvoni; Aug 25th, 2020 at 05:11 AM.
    Last edited by Zvoni; Tomorrow at 31:69 PM.
    ----------------------------------------------------------------------------------------

    One System to rule them all, One Code to find them,
    One IDE to bring them all, and to the Framework bind them,
    in the Land of Redmond, where the Windows lie
    ---------------------------------------------------------------------------------
    People call me crazy because i'm jumping out of perfectly fine airplanes.
    ---------------------------------------------------------------------------------
    Code is like a joke: If you have to explain it, it's bad

  10. #10
    Frenzied Member
    Join Date
    Feb 2003
    Posts
    1,807

    Re: Advise on Function's Intent

    The original code in the op does not look like it should work. The s variable is declared as an integer ("%") but treated like a string: ("s = s + Mid$(field, i, 1)").

    And I could be wrong but some users appear to be under the impression that ("If InStr(" -", Mid$(field, i, 1)) = 0 Then") is checking for the presence of " -". But how can that be if ("Mid$(field, i, 1)") extacts and returns only one single character from the "field" variable? As far as I can tell it appears to be checking whether either " " or "-" are present. And what was the original progamming language? It look like Quick Basic or some old version of Visual Basic...

  11. #11
    The Idiot
    Join Date
    Dec 2014
    Posts
    2,721

    Re: Advise on Function's Intent

    InStr(" -", Mid$(field, i, 1)) = 0

    is actually working,

    " -" is the string1
    and
    Mid$(field, i, 1) is the search string, so example:
    if field is "12-4"
    it will do:

    " -", "1" = 0 true
    " -", "2" = 0 true
    " -", "-" = 2 false
    " -", "4" = 0 true

    and should be: "124" or 7 depends if we want s as string or integer
    if integer, s = s + Mid$(field, i, 1) should be s = s + Val(Mid$(field, i, 1)) if string, s = s & Mid$(field, i, 1)

    (I use Val because we don't know for sure its a number, it could be something else, if we use CInt it would return as an error if it tries to convert a letter)
    Last edited by baka; Aug 25th, 2020 at 07:36 AM.

  12. #12
    PowerPoster Zvoni's Avatar
    Join Date
    Sep 2012
    Location
    To the moon and then left
    Posts
    4,418

    Re: Advise on Function's Intent

    Peter, you fell into the same trap as me. See my first edit above.
    InStr([Start],String1,String2) --> InStr searches for String2 in String1

    EDIT: Nevermind, that the way the original code was working is to take the long route
    Field="Annoying long text-with crap-in between"
    The code runs through the whole length of the string, instead of turning it around, and run through the Illegal characters (which would/should be much shorter).
    Nevermind Replace vs. concatenating the result.

    EDIT2: btw: It would be a perfect candidate for my ReplaceAny-Function in the codebank
    Last edited by Zvoni; Aug 25th, 2020 at 07:37 AM.
    Last edited by Zvoni; Tomorrow at 31:69 PM.
    ----------------------------------------------------------------------------------------

    One System to rule them all, One Code to find them,
    One IDE to bring them all, and to the Framework bind them,
    in the Land of Redmond, where the Windows lie
    ---------------------------------------------------------------------------------
    People call me crazy because i'm jumping out of perfectly fine airplanes.
    ---------------------------------------------------------------------------------
    Code is like a joke: If you have to explain it, it's bad

  13. #13
    Frenzied Member
    Join Date
    Feb 2003
    Posts
    1,807

    Re: Advise on Function's Intent

    Instead of fixating on the details of how that code supposedly works, can someone provide a description of what it actually is supposed to do in plain english? Looks like it would have been nice if whoever wrote it had added a proper comment.

  14. #14

    Thread Starter
    Super Moderator dday9's Avatar
    Join Date
    Mar 2011
    Location
    South Louisiana
    Posts
    11,711

    Re: Advise on Function's Intent

    Quote Originally Posted by Peter Swinkels View Post
    Instead of fixating on the details of how that code supposedly works, can someone provide a description of what it actually is supposed to do in plain english? Looks like it would have been nice if whoever wrote it had added a proper comment.
    That's part of the fun with this legacy conversion. The language is VB6, but the project started in '94 so what I think happened was that it was originally written VB3, then upgraded to VB5, and then ultimately wound up in VB6. So the documentation of the features isn't what it should be, when we approach SME's they just say "well it works when I do this".

    Now it's my job to convert this VB6 project into a modern web app using Angular 9 on the front-end and C# MVC on the back-end.

    Part of my struggle with this conversion is that not only does it appear that approximately 50 million developers worked on this in the past 30 years (say goodbye to naming conventions), but there is code that appears more complex than it needs to be (like this function for example).

    Plus while I have a very strong Visual Basic .NET background, I always focused on a .NET approach and not a Visual Basic specific approach. This had the benefit of allowing me to pick up C# and F# very quickly and I also got a solid understanding of OOP paradigms, but the downfall is that I'm not familiar with Visual Basic specific functions like InStr.

    Still nonetheless, I feel like I have a clearer understanding of the Function now and what its intent was so I am going to mark this thread as resolved.
    "Code is like humor. When you have to explain it, it is bad." - Cory House
    VbLessons | Code Tags | Sword of Fury - Jameram

  15. #15
    Smooth Moperator techgnome's Avatar
    Join Date
    May 2002
    Posts
    34,531

    Re: [RESOLVED] Advise on Function's Intent

    Long story short...It's the piece that's been maintained in Nebraska all this time...


    -tg
    * I don't respond to private (PM) requests for help. It's not conducive to the general learning of others.*
    * I also don't respond to friend requests. Save a few bits and don't bother. I'll just end up rejecting anyways.*
    * How to get EFFECTIVE help: The Hitchhiker's Guide to Getting Help at VBF - Removing eels from your hovercraft *
    * How to Use Parameters * Create Disconnected ADO Recordset Clones * Set your VB6 ActiveX Compatibility * Get rid of those pesky VB Line Numbers * I swear I saved my data, where'd it run off to??? *

  16. #16
    PowerPoster
    Join Date
    Aug 2010
    Location
    Canada
    Posts
    2,412

    Re: [RESOLVED] Advise on Function's Intent

    There is a bit of a puzzle in terms of the code's intent that can maybe only be answered by looking at the larger application. What data is typically being passed to FormatField?

    First, the code as posted can't run - there will always be a type mismatch error raised at the s = "" line since s is declared as an integer.

    Assuming the code as originally posted was copied verbatim, then FormatField is either never called (in which case we can abandon it completely), or it is always wrapped with On Error Resume Next - in which case it effectively does nothing (other than changing the Err object, so perhaps there's code that relies on that to do something else).

    Let's assume one of two other possibilities though:

    1) The s = "" shouldn't be there. In that case, assuming the value of Field is always numeric with the possibility of " " or "-" characters, then the code is trying to add the digits of the field string into an integer. E.g. FormatField("123-123") will result in 12 (or, 1+2+3+1+2+3). An error would be raised if any characters are not 0-9, "-", or " ".

    2) The s variable is actually declared as a string. In this case, the code is simply trying to remove " " and "-" from the passed field, so FormatField("123-123") would return "123123" (e.g. "1" & "2" & "3" & "1" & "2" & "3").

    The trick will be determining which of the above possibilities is correct - assuming there's it's not trying to do something else that I've missed!
    Last edited by jpbro; Aug 25th, 2020 at 09:17 AM.

  17. #17
    Frenzied Member
    Join Date
    Feb 2003
    Posts
    1,807

    Re: [RESOLVED] Advise on Function's Intent

    @dday9 - perhaps you should look into how older BASICs work even if they are long since obsolete. It may help you understand antiquated code such as what you had to deal with here. @tg: LOL! Sounds familiar. @jpbro: true enough, sometimes it helps to look at the bigger picture.

  18. #18
    PowerPoster
    Join Date
    Feb 2017
    Posts
    4,995

    Re: [RESOLVED] Advise on Function's Intent

    Quote Originally Posted by jpbro View Post
    First, the code as posted can't run - there will always be a type mismatch error raised at the s = "" line since s is declared as an integer.
    Exactly. The code as it is can't run.

  19. #19
    PowerPoster
    Join Date
    Aug 2010
    Location
    Canada
    Posts
    2,412

    Re: [RESOLVED] Advise on Function's Intent

    If you're going to be calling this function a lot and the goal is to strip " " and "-" characters from any string, then the following code when compiled with optimizations enabled is about 5x faster than the original code (fixed with "s" as string) and about 3.5x faster than calling Replace$:

    Code:
    Function FormatField(ByVal RawField As String) As String
       Dim ii As Long
       Dim l_LenField As Long
       Dim l_LenChunk As Long
       Dim l_Start As Long
       Dim l_Offset As Long
       
       RawField = Trim$(RawField)
       l_LenField = Len(RawField)
       If l_LenField = 0 Then Exit Function   ' Nothing to process, shortcircuit
       
       FormatField = String$(l_LenField, 0)
       l_Offset = 1
       
       For ii = 1 To l_LenField
          Select Case AscW(Mid$(RawField, ii, 1))
          Case 32, 45   ' Space or Minus
             If l_Start > 0 Then
                l_LenChunk = ii - l_Start
                
                Mid$(FormatField, l_Offset, l_LenChunk) = Mid$(RawField, l_Start, l_LenChunk)
             
                l_Start = 0
                l_Offset = l_Offset + l_LenChunk
             End If
             
          Case Else
             '  Any other character
             If l_Start = 0 Then l_Start = ii
          
          End Select
       Next ii
       
       If l_Start > 0 Then
          Mid$(FormatField, l_Offset) = Mid$(RawField, l_Start)
          
          l_Offset = l_Offset + l_LenField - l_Start + 1
       End If
       
       FormatField = Left$(FormatField, l_Offset - 1)
    End Function
    Note that the above is lightly tested, but I've generated a few million random strings and the results have always matched the string corrected original function.
    Last edited by jpbro; Aug 25th, 2020 at 10:52 AM.

  20. #20
    PowerPoster
    Join Date
    Aug 2010
    Location
    Canada
    Posts
    2,412

    Re: [RESOLVED] Advise on Function's Intent

    Below is an even faster version (55-60x faster than the original function!) if you don't mind using APIs:

    Code:
    Option Explicit
    
    Private Const FADF_AUTO      As Integer = &H1   'An array that is allocated on the stack.
    Private Const FADF_FIXEDSIZE As Integer = &H10  'An array that may not be resized or reallocated.
    
    Private Type SAFEARRAY1D    'Represents a safe array. (One Dimensional)
        cDims      As Integer   'The count of dimensions.
        fFeatures  As Integer   'Flags used by the SafeArray.
        cbElements As Long      'The size of an array element.
        cLocks     As Long      'The number of times the array has been locked without a corresponding unlock.
        pvData     As Long      'Pointer to the data.
        cElements  As Long      'The number of elements in the dimension.
        lLbound    As Long      'The lower bound of the dimension.
    End Type                    'https://msdn.microsoft.com/en-us/library/ms221482(v=vs.85).aspx
    
    Private Declare Function VarPtrArray Lib "msvbvm60.dll" Alias "VarPtr" (ByRef ArrayVar() As Any) As Long
    Private Declare Sub PutMem4 Lib "msvbvm60.dll" (ByVal addr As Long, ByVal NewVal As Long)
    
    Function FormatField(ByVal RawField As String) As String
       Dim ii As Long
       Dim l_LenField As Long
       Dim l_LenChunk As Long
       Dim l_Start As Long
       Dim l_Offset As Long
       
       RawField = Trim$(RawField)
       l_LenField = Len(RawField)
       If l_LenField = 0 Then Exit Function   ' Nothing to process, shortcircuit
       
       FormatField = RawField
       
       Dim lt_Buff As SAFEARRAY1D
       Dim la_Buff() As Integer
       
       With lt_Buff
          .cDims = 1
          .fFeatures = FADF_AUTO Or FADF_FIXEDSIZE
          .cbElements = 2&
          .cLocks = 1&
          .lLbound = 1&
          .pvData = StrPtr(FormatField)
          .cElements = l_LenField
       End With
       
       PutMem4 VarPtrArray(la_Buff), VarPtr(lt_Buff)
       
       For ii = 1 To l_LenField
          Select Case la_Buff(ii)
          Case 32, 45   ' Space or Minus
             l_Offset = l_Offset + 1
             
          Case Else
             '  Any other character
             If l_Offset > 0 Then la_Buff(ii - l_Offset) = la_Buff(ii)
          
          End Select
       Next ii
          
       PutMem4 VarPtrArray(la_Buff), 0&
    
       If l_Offset > 0 Then FormatField = Left$(FormatField, l_LenField - l_Offset)
    End Function
    NOTE: I borrowed the string<>array technique from Bonnie West's post here: https://www.vbforums.com/showthread....-Integer-Array

    NOTE #2: These performance improvements are nice, but probably only useful if you are really hitting this function hard in a loop (e.g. processing a lot of fields from a database or something along those lines).

  21. #21
    PowerPoster
    Join Date
    Feb 2017
    Posts
    4,995

    Re: [RESOLVED] Advise on Function's Intent

    And here is a one-liner version:

    Code:
    Function FormatField(ByVal field As String) As String
        FormatField = Replace$(Replace$(field, " ", ""), "-", "")
    End Function

  22. #22
    PowerPoster
    Join Date
    Aug 2010
    Location
    Canada
    Posts
    2,412

    Re: [RESOLVED] Advise on Function's Intent

    Yes, I'd go with Replace$ if speed isn't absolutely critical. About ~1.5x faster than the original function, and infinitely more readable. If speed is critical, then the SafeArray approach is dramatically better (50-60x faster than the original function, or ~35x faster than Replace$).

    Of course, if the intent of the original function is to actually add digits rather than concatenate them, then neither of the above will do.

  23. #23
    PowerPoster
    Join Date
    Feb 2017
    Posts
    4,995

    Re: [RESOLVED] Advise on Function's Intent

    If the unwanted characters are not frequent to be found in the String, a way to improve the speed is to call InStr befere replaing:

    Code:
    Function FormatField(ByRef field As String) As String
        FormatField = field
        If InStr(FormatField, " ") Then FormatField = Replace$(FormatField, " ", "")
        If InStr(FormatField, "-") Then FormatField = Replace$(FormatField, "-", "")
    End Function
    Also, the imput string is passed ByRef.

    Of course that won't compare to your super optimized version.

Posting Permissions

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



Click Here to Expand Forum to Full Width