Is that good code?

[Xenobiologist]

Hi,

just a little help for a programming beginner.

Func _ArrayUnique($aArray, $iDimension = 1, $iBase = 0, $iCase = 0, $vDelim = "|")
    Local $iUboundDim
    ;$aArray used to be ByRef, but litlmike altered it to allow for the choosing of 1 Array Dimension, without altering the original array
    If $vDelim = "|" Then $vDelim = Chr(01) ; by SmOke_N, modified by litlmike
    If Not IsArray($aArray) Then Return SetError(1, 0, 0) ;Check to see if it is valid array

    ;Checks that the given Dimension is Valid
    If Not $iDimension > 0 Then
        Return SetError(3, 0, 0) ;Check to see if it is valid array dimension, Should be greater than 0
    Else

If've got a question whether this ... If Not $iDimension > 0 Then ...is really good code?

What does Autoit do?

Assuming that $Dimension is 0

Does Autoit compare Not 0 > 0 or first transform not 0 to true and then compare true > 0 ?

Does it use brackets, so the code internally looks like ... If Not (0 > 0) ?

Shouldn't the code-construct Not $variable only be used with booleans?

Thanks for clarification.

Mega

[jvanegmond]

Nice little trick I picked up at testing class in school.

Local $iDimension, $Test

$iDimension = -1
$Test = Not $iDimension > 0
ConsoleWrite($Test & @CRLF) ; False

$iDimension = 0
$Test = Not $iDimension > 0
ConsoleWrite($Test & @CRLF) ; True

$iDimension = 1
$Test = Not $iDimension > 0
ConsoleWrite($Test & @CRLF) ; False

Draw conclusion yourself.

[monoceres]

In many languages the NOT (or !) operator usually have a lower precedence than ==,< and >, but in autoit NOT is the king of the hill therefore the code you demonstrated:

If Not $iDimension > 0

Is the same as

If (Not $iDimension) > 0

Which is probably not what the author wanted, but it still works since (((((NOT 1) == FALSE))==0)>0)==FALSE

Parenthesis should be added still.

[Jon]

If there is an easier to understand way of writing it that likely does the same "under the hood" then I'd use that. Personally.

Edit: For example, off the top of my head without checking the docs I'm not sure what the result is = bad.

[Xenobiologist]

Yes, you both are right with what you mentioned.

To make it more clear what I think

First of all, I can read the code very easily and of course I do understand what Autoit does with the code.

But FMPOV, it is bad code, cause Autoit doesn't do what is expected by beginners reading the code.

I would not use Not with a non boolean variable.

Tell me, if I'm totally wrong.

Thanks!

Mega

[jvanegmond]

I think it's rather ridiculous

If $iDimension < 0 Then

[trancexx]

I think it's rather ridiculous

If $iDimension < 0 Then

Ah, but you are wrong.

If $iDimension <= 0 Then

[Xenobiologist]

I think it's rather ridiculous

If $iDimension < 0 Then

What do you want to tell me by this post?

[trancexx]

What do you want to tell me by this post?

May I?

Manadar is saying it's rather stupid either of these:

If Not $iDimension > 0 Then

If (Not $iDimension) > 0 Then

or

If Not $iDimension Then

when it should be what it should be:

If $iDimension <= 0 Then

or

If Not ($iDimension > 0) Then

Edited January 8, 2010 by trancexx

[Xenobiologist]

So, you both agree that the code is programmed badly , right?!

Fine

(I don't want to discuss which possibility would be the fastest, although it could be interesting. )

[Jon]

So, you both agree that the code is programmed badly , right?!

Fine

(I don't want to discuss which possibility would be the fastest, although it could be interesting. )

Hard to understand (I wasn't sure you meant <= 0) and in AutoIt probably slower...

In AutoIt, it's usually the code with the least amount of tokens (not neccesarily the case for C++ or other languages).

[Xenobiologist]

Okay, sorry once again this function.

This is a more like a feature request. The function has got a huge time-problem with big arrays. Maybe somebody can change/improve it.

Here is a demo script to show what I mean.

#include <Array.au3>
Global $v = 5000, $a[$v +1], $s

For $i = 0 To $v
    $a[$i] = Random(0, 50, 1)
Next

$s = TimerInit()
$a = _ArrayUnique($a)
ConsoleWrite(Round(TimerDiff($s), 2) & ' sec' & @CRLF)

$s = TimerInit()
$a = __ArrayUnique($a)
ConsoleWrite(Round(TimerDiff($s), 2) & ' sec' & @CRLF)

Func __ArrayUnique(ByRef $aArray, $vDelim = '', $iBase = 1, $iUnique = 1)
    If $vDelim = '' Then $vDelim = Chr(01)
    Local $sHold
    For $iCC = $iBase To UBound($aArray) - 1
        If Not StringInStr($vDelim & $sHold, $vDelim & $aArray[$iCC] & $vDelim, $iUnique) Then _
                $sHold &= $aArray[$iCC] & $vDelim
    Next
    Return StringSplit(StringTrimRight($sHold, StringLen($vDelim)), $vDelim)
EndFunc ;==>_ArrayUnique

Mega

[Bowmore]

That is a big improvement in speed. I wrote a script this week at work that needs to filter an array of about 1,100,000 values down to about 250,000 unique values and it's taking longer than I would like. I was going to have a look at using a method other than using _arrayUnique(), but it looks like you have saved me the trouble. My initial test show that your version will be about 12 times faster. Thanks for sharing.

BTW Seeing the original topic of this thread, should not 

If Not StringInStr(....) Then

 

be

If StringInStr(....) > 0 Then

 

 

Edited March 6, 2010 by Bowmore

[GEOSoft]

That is a big improvement in speed. I wrote a script this week at work that needs to filter an array of about 1,100,000 values down to about 250,000 unique values and it's taking longer than I would like. I was going to have a look at using a method other than using _arrayUnique(), but it looks like you have saved me the trouble. My initial test show that your version will be about 12 times faster. Thanks for sharing.

BTW Seeing the original topic of this thread, should not 

If Not StringInStr(....) Then

 

be

If StringInStr(....) > 0 Then

 

 

Either of those will work fine. I usually use the NOT method myself but many prefer the > 0 method.

[Xenobiologist]

Okay, that discussion should be done. Should I make a feature request for the rewrite/improvement on _ArrayUnique?

[MHz]

@Bowmore

You do realize that those function conditional tests are opposites? One handles a true condition while the other handles a false condition. The > 0 usage is not required for StringInStr() as your testing for a true or false condition. Using "If StringInString() Then" does what is required to test. Omitting > 0 would have allowed seeing the function conditional test as clearly opposite.

The test is the same as the below test

; Not 1 equals 0 (False)
If Not 1 Then
    MsgBox(0, 'Test 1', 'Not 1')
EndIf

; 1 > 0 is True
If 1 > 0 Then
    MsgBox(0, 'Test 2', '1 > 0')
EndIf

@mega

Your demo script is approximately 50 times quicker then it should be with your __ArrayUnique() timing. The array being passed to __ArrayUnique() is a smaller, filtered array (uniques only) from the previous use of _ArrayUnique().

This demo script seems correct with the first 2 attempts at removing uniques items and the later 2 attempts are to mirror your previous usage for comparison. I added a couple of UDFs to help with ensuring that the same code is being used for all array unique tests. The For loop is to test for same values and show differences if any as your original test was not the same.

expandcollapse popup

#include <Array.au3>
Global $v = 5000, $a[$v +1], $s

For $i = 0 To $v
    $a[$i] = Random(0, 50, 1)
Next

ConsoleWrite('$b =  _ArrayUnique($a)' & @TAB)
$s = TimerInit()
$b = _ArrayUnique($a)
_Items($b)
_TimerDiff($s)

ConsoleWrite('$c = __ArrayUnique($a)' & @TAB)
$s = TimerInit()
$c = __ArrayUnique($a)
_Items($c)
_TimerDiff($s)

; similar to megas' suspicious attempt with an array which reuses $a
ConsoleWrite('$a =  _ArrayUnique($a)' & @TAB)
$s = TimerInit()
$a = _ArrayUnique($a)
_Items($a)
_TimerDiff($s)

; similar to megas' suspicious attempt with a already filtered array
ConsoleWrite('$a = __ArrayUnique($a)' & @TAB)
$s = TimerInit()
$a = __ArrayUnique($a)
_Items($a)
_TimerDiff($s)

; compare items and show differences
For $chk = 0 To UBound($b) -1
    If $b[$chk] <> $c[$chk] Then
        ConsoleWrite('! ' & $b[$chk] & ' = ' & $c[$chk] & @CRLF)
    EndIf
Next

Func __ArrayUnique(ByRef $aArray, $vDelim = '', $iBase = 0, $iUnique = 1)
    If $vDelim = '' Then $vDelim = Chr(01)
    Local $sHold
    For $iCC = $iBase To UBound($aArray) - 1
        If Not StringInStr($vDelim & $sHold, $vDelim & $aArray[$iCC] & $vDelim, $iUnique) Then _
                $sHold &= $aArray[$iCC] & $vDelim
    Next
    Return StringSplit(StringTrimRight($sHold, StringLen($vDelim)), $vDelim)
EndFunc ;==>_ArrayUnique

Func _Items(ByRef $array)
    ; consolewrite item count
    ConsoleWrite(UBound($array) & ' items' & @TAB)
EndFunc

Func _TimerDiff(ByRef $diff)
    ; consolewrite time in milliseconds
    ConsoleWrite(Int(TimerDiff($diff)) & ' ms' & @CRLF)
EndFunc

__ArrayUnique() is faster as it looks to be _ArrayUnique() with a large block of code removed. Thorough testing would be required if it were a replacement but it does display how _ArrayUnique() is perhaps excessive with treating an array.

For your last question, I would consider a rewrite may bring some good results. Only one way to find out if it does and that is create and test.

Edit:

Added a sentence.

Edited March 8, 2010 by MHz

[Xenobiologist]

Oh well thanks. My fault not to mention that point. I tried it this way, too.

expandcollapse popup

#include <Array.au3>
Global $v = 500, $a1[$v + 1], $a2, $s1, $s2, $t1, $t2

_createTestArrays()

a1()
ConsoleWrite('Size of Array = ' & UBound($a2) - 1 & @CRLF)
a2()

_createTestArrays()

a2()
ConsoleWrite('Size of Array = ' & UBound($a1) - 1 & @CRLF)
a1()

ConsoleWrite(Round($t1 / $t2, 2) & ' times faster!' & @CRLF)

Func a1()
    $s1 = TimerInit()
    $a1 = _ArrayUnique($a1)
    $t1 = Round(TimerDiff($s1), 2)
    ConsoleWrite('Standard _ArrayUnique : ' & $t1 & ' Msec' & @CRLF)
EndFunc ;==>a1

Func a2()
    $s2 = TimerInit()
    $a2 = __ArrayUnique($a2)
    $t2 = Round(TimerDiff($s2), 2)
    ConsoleWrite('Forum(Smoke_N) _ArrayUnique : ' & $t2 & ' Msec' & @CRLF)
EndFunc ;==>a2

Func _createTestArrays()
    ReDim $a1[$v + 1]
    $a2 = $a1
    For $i = 0 To $v
        $a1[$i] = Random(0, 50, 1)
    Next
    $a2 = $a1
EndFunc ;==>_createTestArrays

Func __ArrayUnique(ByRef $aArray, $vDelim = '', $iBase = 1, $iUnique = 1)
    If $vDelim = '' Then $vDelim = Chr(01)
    Local $sHold
    For $iCC = $iBase To UBound($aArray) - 1
        If Not StringInStr($vDelim & $sHold, $vDelim & $aArray[$iCC] & $vDelim, $iUnique) Then _
                $sHold &= $aArray[$iCC] & $vDelim
    Next
    Return StringSplit(StringTrimRight($sHold, StringLen($vDelim)), $vDelim)
EndFunc ;==>__ArrayUnique

As for the results: With your script I get this :

$b = _ArrayUnique($a)   52 items    12014 ms
$c = __ArrayUnique($a)  52 items    41 ms
$a = _ArrayUnique($a)   52 items    12276 ms
$a = __ArrayUnique($a)  53 items    0 ms

My attempt shows this:

Standard _ArrayUnique : 109.92 Msec
Size of Array = 500
Forum(Smoke_N) _ArrayUnique : 3.64 Msec
Forum(Smoke_N) _ArrayUnique : 3.6 Msec
Size of Array = 500
Standard _ArrayUnique : 109.31 Msec
30.36 times faster!

Edit: I added the results and Smoke_N who seems to wrote the __ArrayUnique.

Edit2 : So, there are two situations.

Normal array and already filtered array!

Mega

Edited March 8, 2010 by Xenobiologist

[Bowmore]

@Bowmore

You do realize that those function conditional tests are opposites? One handles a true condition while the other handles a false condition.

Of course > should have been =

It looks as though my attempt at a little bit of humor has fallen flat on it's face. The orginal theme of this thread was whether it was good practice to use the Not operator on non boolean values and I was attempting in a light hearted way tne contradiction.

[MHz]

Of course > should have been =

That seems correct but are you going to use "= 0" to test for a False condition and "= 1" to test for a True condition. The "= 0" and "= 1" is unneeded extra which may confuse you. I prefer just to use Not.

The issue is "Not $iDimension > 0" which someone mentioned as working but I say only for testing for 0 or the keyword Default and everything else it passes as OK. The syntax is confusing and flawed.

How flawed is "Not $iDimension > 0"? Try the below code to find out. The 1st test is "Not $iDimension > 0" and the 2nd is "Not StringIsInt($iDimension) Or $iDimension <= 0". Only the latter test gives good results.

; If Not $iDimension > 0 then
ConsoleWrite('"Not $iDimension > 0" = Only 0 and the keyword Default is True. Everything else is False' & @CRLF)
ConsoleWrite('If True Then Return SetError()' & @CRLF)
$iDimension = 'junk'
ConsoleWrite($iDimension & ' = ' & (Not $iDimension > 0) & @CRLF)
$iDimension = True
ConsoleWrite($iDimension & ' = ' & (Not $iDimension > 0) & @CRLF)
$iDimension = Default
ConsoleWrite($iDimension & ' = ' & (Not $iDimension > 0) & @CRLF)
For $iDimension = -1 To 2
    ConsoleWrite($iDimension & ' = ' & (Not $iDimension > 0) & @CRLF)
Next
ConsoleWrite(@CRLF)

; If Not StringIsInt($iDimension) Or $iDimension <= 0 Then
ConsoleWrite('"Not StringIsInt($iDimension) Or $iDimension <= 0" = Everything except positive integers is True.' & @CRLF)
ConsoleWrite('If True Then Return SetError()' & @CRLF)
$iDimension = 'junk'
ConsoleWrite($iDimension & ' = ' & (Not StringIsInt($iDimension) Or $iDimension <= 0) & @CRLF)
$iDimension = True
ConsoleWrite($iDimension & ' = ' & (Not StringIsInt($iDimension) Or $iDimension <= 0) & @CRLF)
$iDimension = Default
ConsoleWrite($iDimension & ' = ' & (Not StringIsInt($iDimension) Or $iDimension <= 0) & @CRLF)
For $iDimension = -1 To 2
    ConsoleWrite($iDimension & ' = ' & (Not StringIsInt($iDimension) Or $iDimension <= 0) & @CRLF)
Next

Output

"Not $iDimension > 0" = Only 0 and the keyword Default is True. Everything else is False
If True Then Return SetError()
junk = False
True = False
Default = True
-1 = False
0 = True
1 = False
2 = False

"Not StringIsInt($iDimension) Or $iDimension <= 0" = Everything except positive integers is True.
If True Then Return SetError()
junk = True
True = True
Default = True
-1 = True
0 = True
1 = False
2 = False

Would you use "Not $iDimension > 0" after seeing the results?

Edited March 8, 2010 by MHz

[PsaltyDS]

Order of operations throws people off with NOT, because it is the first operation performed (highest precedence) before their compare operators. So if used in a simple context like "If NOT @error Then" everything works as expected. But "If Not $iDimension > 0 Then" is executed as "If (Not $iDimension) > 0 Then" due to order of precedence.