Vbscript ported to vb.net, any chance of better code?



  • well it finally happened, i've been charged with porting an old vbscript "application" to VB.net. part of what this mess had to do was deal with some massive arrays of about 30,000 elements. i made a set of functions to sort the data of the arrays and keep them relative to eachother. in all theres about a half dozen of these and the original code ran suprisingly fast in vbscript, given the scope of the task and it being written in vbscript of all things. anyhow, whats done is done and for the sake of my own sanity i changed the original function names all to xsort with overloading because i just know once i'm done they're gonna have me do something else to the program. [:'(] but anyhow here it is, ported to VB.net, this sample rearranges the data in 2 arrays based on the first. somebody tell me there's a faster way to sort a set of arrays side-by-side in excess of 30k elements. commenting added for clarity. bear in mind that vbscript is fairly limited in it's collection of functions/methods/etc and this was originally coded for that back in the days of win98.

     

    Public Overloads Function xsort(ByRef p As String(), ByRef d As String()) As Boolean
        On Error GoTo errorline
        Dim n As Long = UBound(p, 1)
        Dim y As Long = 0
        Dim s As Long = n - 1
        Dim a As Long = 0
        Dim b As Long = 0
        Dim q As Byte = 0
        Dim t As Long = 0
        Dim x As String = "" 'innitialize to empty
        For a = 0 To n - 1 'theoretical maximum possible number of passes required to sort
            q = 1 'this variable tests that a swap was made in this pass
            For b = y To (s) 'ascending pass, carries the highest value all the way to the highest element
                If (StrComp(p(b), p(b + 1), CompareMethod.Binary) > 0) Then
                    x = p(b)
                    p(b) = p(b + 1)
                    p(b + 1) = x
                    x = d(b)
                    d(b) = d(b + 1)
                    d(b + 1) = x
                    q = 0 'a swap was made
                    s = b 'highest point of swap, everything after this point doesnt need checking
                End If
            Next
            For t = n - s To n - y 'descending pass, carries lowest value all the way to the lowest element
                b = n - t
                If (StrComp(p(b), p(b + 1), CompareMethod.Binary) > 0) Then
                    x = p(b)
                    p(b) = p(b + 1)
                    p(b + 1) = x
                    x = d(b)
                    d(b) = d(b + 1)
                    d(b + 1) = x
                    q = 0 'a swap was made
                    y = b 'lowest point of swap, everything before blah blah
                End If
            Next
            If (q = 1) Then 'if q is still 1 then no swaps were made forewards or back in this itteration. sorting is complete.
                Return True
                Exit Function
            End If
        Next
        Return True
        Exit Function
    errorline:
        Console.WriteLine("error code: {0} ", Err.Number)
        Return False
        Exit Function
    End Function
     
    i just hope this doesnt end up being a new wtf.


  • OK first off, it isnt too bad just needs a bit of a clean up.

    DONT use varible names like 'p', 'q', 'x' and so on. The only place where i see it fit to name things like this are a) simple 'for' loops b) simple function paramaters.

    Also, try to use the boolean type for varibles that are logic, e.g.  'q'
    It makes a lot more sense to see :

    if(!swapsMade) ' no swaps made; we are finished sorting
        exit function
        jadaadada
    end if

    Other than that the function isnt too bad.

    However it would make a bit more sense to do this ( the entire app ) "some other way"(tm) that would allow you to use a built-in sort function.





  • @paranoidgeek said:

    if(!swapsMade) ' no swaps made; we are finished sorting

    Ohh and another thing dont ever use 'we' in comments.



  • @paranoidgeek said:

    @paranoidgeek said:
    if(!swapsMade) ' no swaps made; we are finished sorting

    Ohh and another thing dont ever use 'we' in comments.


    Correct.

    We don't like that.



  • @paranoidgeek said:

    DONT use varible names like 'p', 'q', 'x' and so on. The only place where i see it fit to name things like this are a) simple 'for' loops b) simple function paramaters.


    I whole-heartedly agree, thank you.  Most people just say NEVER EVER USE THEM!@#!@#

    Well, I tell you that sometimes a one letter variable is 5 times as readable as a descriptive one.



  • well sure, i can change it now. but you guys gotta remember, in vbscript, length of everything hits performance. the original had no formatting and every possible space was removed for performance. anyhow, i know if i was doing in it C/C++ i'd probably be swapping pointers instead of the data in the arrays, but then again i dont like C/C++. just my preference. anyhow, i'm pretty new to VB.net and still dont even know if thats possible to do so if anybody can tell me otherwise that'd be great, but seeing as how you guys are more conscerned with the variable names than answering my request i'm guessing the answer is no. come on guys, you can do better [:P]



  • BTW is this a cocktail sort http://en.wikisource.org/wiki/Cocktail_Sort ?



  • hmmm... it is very similar. the only thing is, mine reduces the section of the array scanned by far more than the wiki examples do per itteration of the outer loop. i had no idea that it was basicly the same as some other seemingly common enough piece of code, oh well. i guess thats as good as it gets then. i'm pretty sure if i used one of the examples it'd take the same number of loops of the outer loop but the majority of the loops would take longer with the wiki code to complete the sorting. thanks anyway guys.[:^)]



  • @Otac0n said:

    @paranoidgeek said:
    DONT use varible names like 'p', 'q', 'x' and so on. The only place where i see it fit to name things like this are a) simple 'for' loops b) simple function paramaters.


    I whole-heartedly agree, thank you.  Most people just say NEVER EVER USE THEM!@#!@#

    Well, I tell you that sometimes a one letter variable is 5 times as readable as a descriptive one.


    I only use singles for counters. Even for a property loop over a map (for..in) I use 'key' instead of 'i'.

    The rest is descriptive ie. semantic. They [i]mean[/i] something.

    Verbose if need be.



  • @paenis said:

    well sure, i can change it now. but you guys gotta remember, in vbscript, length of everything hits performance. the original had no formatting and every possible space was removed for performance....



    That's absolutely rediculous.  On a modern processor, removing whitespace will NEVER give you a notable performance boost.

    Try writing efficient loops and queries instead.



  • A few small suggestions:

    Don't pass p and d by reference.  You don't need to and it could only cause problems (unless you actually want to make the variables reference different arrays by the time the function is complete)

    Learn Exceptions.  Try...Catch is much more powerful than On Error...  Simple routines are a good place to practice.

    Don't return a Boolean to indicate failure.  It's too easy to miss an error when calling it.  Throw an Exception (or in good old VBScript, Err.Raise).  Returning a value also limits your options.  An InvalidArgumentException is much clearer than False and although an Integer would re-open those options, it would also be more cumbersome to document and debug.  I like to return something useful, like p to open up the possibility of chaining.

    Please learn how to spell better.  I know it's a nitpick, but it makes the code look like it was written by a 15 year old.  Out of 9 comments, there are 4 spelling errors.  I'm a little jaded on this subject as I'm working on a project where about 10% of the words are spelled wrong (yes, really, that's not an exaggeration).



  • Please God Please remove the On Error GoTo!

     

    Don't make the baby Jesus cry [:'(]



  • @paenis said:

    somebody tell me there's a faster way to sort a set of arrays side-by-side in excess of 30k elements.

    Can't you create a type containing all the items represented in the arrays as properties of the types?  Then change your code to have a single array of these types & sort that?  Don't know much about VBS though so that might not be an option....

     



  • @paranoidgeek said:

    @paranoidgeek said:
    if(!swapsMade) ' no swaps made; we are finished sorting

    Ohh and another thing dont ever use 'we' in comments.


    Even if you are pair programming?



  • 1. Don't use Long. Long is 64-bit now. You want 32-bit Integer use "Integer" <=> "Int32" (alias)
    2. p is an array. Arrays in vb.et ar object. Ubound is ok but it isn't portable. You will find p.Lenght (or p.Count in collections)
    3. "" <=> String.Empty
    4. Use For a As Integer . Declaring type in loop may give better performance
    5. StrComp is the same as "=" and btw: you will find p(b).Compare(p(b+1))
    6. You don't have to use brackets in If.
    7. Using on error is  like surrounding each line with Try Catch
    8. Array.Sort like p.Sort can do the think for you.
    9. Doing

        Return True 
        Exit Function

    have no sense cos exit function will never be evaluated. Fortunately the compiler knows that and erases that line in compliation.


Log in to reply