Sign in to follow this  
Followers 0
Xenobiologist

Is that good code?

42 posts in this topic

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


Scripts & functions Organize Includes Let Scite organize the include files

Yahtzee The game "Yahtzee" (Kniffel, DiceLion)

LoginWrapper Secure scripts by adding a query (authentication)

_RunOnlyOnThis UDF Make sure that a script can only be executed on ... (Windows / HD / ...)

Internet-Café Server/Client Application Open CD, Start Browser, Lock remote client, etc.

MultipleFuncsWithOneHotkey Start different funcs by hitting one hotkey different times

Share this post


Link to post
Share on other sites



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.

Share this post


Link to post
Share on other sites

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.


Broken link? PM me and I'll send you the file!

Share this post


Link to post
Share on other sites

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.

Share this post


Link to post
Share on other sites

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


Scripts & functions Organize Includes Let Scite organize the include files

Yahtzee The game "Yahtzee" (Kniffel, DiceLion)

LoginWrapper Secure scripts by adding a query (authentication)

_RunOnlyOnThis UDF Make sure that a script can only be executed on ... (Windows / HD / ...)

Internet-Café Server/Client Application Open CD, Start Browser, Lock remote client, etc.

MultipleFuncsWithOneHotkey Start different funcs by hitting one hotkey different times

Share this post


Link to post
Share on other sites

I think it's rather ridiculous

If $iDimension < 0 Then

Ah, but you are wrong.

If $iDimension <= 0 Then

♡♡♡

.

eMyvnE

Share this post


Link to post
Share on other sites

I think it's rather ridiculous

If $iDimension < 0 Then

What do you want to tell me by this post? ;)

Scripts & functions Organize Includes Let Scite organize the include files

Yahtzee The game "Yahtzee" (Kniffel, DiceLion)

LoginWrapper Secure scripts by adding a query (authentication)

_RunOnlyOnThis UDF Make sure that a script can only be executed on ... (Windows / HD / ...)

Internet-Café Server/Client Application Open CD, Start Browser, Lock remote client, etc.

MultipleFuncsWithOneHotkey Start different funcs by hitting one hotkey different times

Share this post


Link to post
Share on other sites

#9 ·  Posted (edited)

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 by trancexx

♡♡♡

.

eMyvnE

Share this post


Link to post
Share on other sites

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

Fine :evil:

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


Scripts & functions Organize Includes Let Scite organize the include files

Yahtzee The game "Yahtzee" (Kniffel, DiceLion)

LoginWrapper Secure scripts by adding a query (authentication)

_RunOnlyOnThis UDF Make sure that a script can only be executed on ... (Windows / HD / ...)

Internet-Café Server/Client Application Open CD, Start Browser, Lock remote client, etc.

MultipleFuncsWithOneHotkey Start different funcs by hitting one hotkey different times

Share this post


Link to post
Share on other sites

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

Fine Posted Image

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

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).

Share this post


Link to post
Share on other sites

Okay, sorry once again this function. :mellow:

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


Scripts & functions Organize Includes Let Scite organize the include files

Yahtzee The game "Yahtzee" (Kniffel, DiceLion)

LoginWrapper Secure scripts by adding a query (authentication)

_RunOnlyOnThis UDF Make sure that a script can only be executed on ... (Windows / HD / ...)

Internet-Café Server/Client Application Open CD, Start Browser, Lock remote client, etc.

MultipleFuncsWithOneHotkey Start different funcs by hitting one hotkey different times

Share this post


Link to post
Share on other sites

#13 ·  Posted (edited)

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
 

:mellow: 

Edited by Bowmore

"Programming today is a race between software engineers striving to build bigger and better idiot-proof programs, and the universe trying to build bigger and better idiots. So far, the universe is winning."- Rick Cook

Share this post


Link to post
Share on other sites

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
 

:mellow: 

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

George

Question about decompiling code? Read the decompiling FAQ and don't bother posting the question in the forums.

Be sure to read and follow the forum rules. -AKA the AutoIt Reading and Comprehension Skills test.***

The PCRE (Regular Expression) ToolKit for AutoIT - (Updated Oct 20, 2011 ver:3.0.1.13) - Please update your current version before filing any bug reports. The installer now includes both 32 and 64 bit versions. No change in version number.

Visit my Blog .. currently not active but it will soon be resplendent with news and views. Also please remove any links you may have to my website. it is soon to be closed and replaced with something else.

"Old age and treachery will always overcome youth and skill!"

Share this post


Link to post
Share on other sites

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


Scripts & functions Organize Includes Let Scite organize the include files

Yahtzee The game "Yahtzee" (Kniffel, DiceLion)

LoginWrapper Secure scripts by adding a query (authentication)

_RunOnlyOnThis UDF Make sure that a script can only be executed on ... (Windows / HD / ...)

Internet-Café Server/Client Application Open CD, Start Browser, Lock remote client, etc.

MultipleFuncsWithOneHotkey Start different funcs by hitting one hotkey different times

Share this post


Link to post
Share on other sites

#16 ·  Posted (edited)

@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.

#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 by MHz

Share this post


Link to post
Share on other sites

#17 ·  Posted (edited)

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

#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 by Xenobiologist

Scripts & functions Organize Includes Let Scite organize the include files

Yahtzee The game "Yahtzee" (Kniffel, DiceLion)

LoginWrapper Secure scripts by adding a query (authentication)

_RunOnlyOnThis UDF Make sure that a script can only be executed on ... (Windows / HD / ...)

Internet-Café Server/Client Application Open CD, Start Browser, Lock remote client, etc.

MultipleFuncsWithOneHotkey Start different funcs by hitting one hotkey different times

Share this post


Link to post
Share on other sites

@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.


"Programming today is a race between software engineers striving to build bigger and better idiot-proof programs, and the universe trying to build bigger and better idiots. So far, the universe is winning."- Rick Cook

Share this post


Link to post
Share on other sites

#19 ·  Posted (edited)

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 by MHz

Share this post


Link to post
Share on other sites

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.

:mellow:


Valuater's AutoIt 1-2-3, Class... Is now in Session!For those who want somebody to write the script for them: RentACoder"Any technology distinguishable from magic is insufficiently advanced." -- Geek's corollary to Clarke's law

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!


Register a new account

Sign in

Already have an account? Sign in here.


Sign In Now
Sign in to follow this  
Followers 0