Increasing code efficiency.

[I-F AB]

Hi all,
I wrote some code a couple of weeks ago which I now think may have
some runtime inefficiencies in it.
I'm not sure if I've written it the right way but now I think I might
be doing things in a potentially slow way, especially if I'm dealing
with a monstrous layout of >40 cells.
I'm thinking of using an array but somehow I'm stumped.
Could anyone help me on this?

Example of a typical input:
ListIn = '( "nor2" "res4" "or6" "res7" "aoi3" "aoi1" )
Mode = "Hilite"

****************************************************************************************
procedure( CBOnArr2Inst(ListIn Mode)
prog( (OpsList SizeList elemStr Ops)
OpsList = '()
SizeList = length(ListIn)
for( elem 1 SizeList
sprintf(elemStr "%n" elem) ;convert no. -> string
; evalstring( strcat( "\\" elem "=\"" elem "\"") )
evalstring( strcat("ElemCell_" elemStr
" = \"" nthelem(elem ListIn) "\"") )
;create list of operations
if( Mode == "Switch" then
Ops = strcat("CBButtons2(ElemCell_" elemStr ")")
else ;else if/case
Ops = strcat("CBButtons1(ElemCell_" elemStr ")")
) ;if
OpsList = append1(OpsList Ops)
);for
return(OpsList)
) ;prog
) ;proc CBOnArr2Inst
****************************************************************************************

This code is actually used for a GUI.
It works as I want it but may cause some problems in runtime when I
start putting everything together. The input would be from a GUI for
example:

1st buttonboxfield: ?callback CBOnArr2Inst(OriList
"Hilite")
2nd buttonboxfield: ?callback CBOnArr2Inst(NuList
"Switch")

Thanks.

Best Regards,
I-FAB

 

[Andrew Beckett]

I-F AB wrote, on 03/06/09 10:39:

Hi all,
I wrote some code a couple of weeks ago which I now think may have
some runtime inefficiencies in it.
I'm not sure if I've written it the right way but now I think I might
be doing things in a potentially slow way, especially if I'm dealing
with a monstrous layout of >40 cells.
I'm thinking of using an array but somehow I'm stumped.
Could anyone help me on this?

Example of a typical input:
ListIn = '( "nor2" "res4" "or6" "res7" "aoi3" "aoi1" )
Mode = "Hilite"

****************************************************************************************
procedure( CBOnArr2Inst(ListIn Mode)
prog( (OpsList SizeList elemStr Ops)
OpsList = '()
SizeList = length(ListIn)
for( elem 1 SizeList
sprintf(elemStr "%n" elem) ;convert no. -> string
; evalstring( strcat( "\\" elem "=\"" elem "\"") )
evalstring( strcat("ElemCell_" elemStr
" = \"" nthelem(elem ListIn) "\"") )
;create list of operations
if( Mode == "Switch" then
Ops = strcat("CBButtons2(ElemCell_" elemStr ")")
else ;else if/case
Ops = strcat("CBButtons1(ElemCell_" elemStr ")")
) ;if
OpsList = append1(OpsList Ops)
);for
return(OpsList)
) ;prog
) ;proc CBOnArr2Inst
****************************************************************************************

This code is actually used for a GUI.
It works as I want it but may cause some problems in runtime when I
start putting everything together. The input would be from a GUI for
example:

1st buttonboxfield: ?callback CBOnArr2Inst(OriList
"Hilite")
2nd buttonboxfield: ?callback CBOnArr2Inst(NuList
"Switch")

Thanks.

Best Regards,
I-FAB

Hmm. You're making 4 common mistakes:

1. Iterating over a list with a for loop, and then using nthelem to access the
entry. This is O(N^2) where N is the length of the list.
2. Building up the list by appending to the end of the list with append1(). This
is also rather inefficient, as you need to take a copy of the list being
appended to to avoid doing a destructive modification. This is also O(N^2)
3. Using evalstring - run-time evaluation you should avoid if possible.
4. Storing data in symbols

That said, you don't have an immense amount of data here.

Something like this (not tested, of course) would be cleaner:

ElemCell=makeTable('ElemCell nil)
procedure( CBOnArr2Inst(ListIn Mode)
foreach(mapcar elemStr ListIn
; I can't really work out why you even need to store the array.
ElemCell[elemStr]=elemStr
;create list of operations
if( Mode == "Switch" then
strcat("CBButtons2(ElemCell[\"" elemStr "\"])")
else ;else if/case
strcat("CBButtons1(ElemCell[\"" elemStr "\"])")
) ;if
);foreach
) ;proc CBOnArr2Inst

Part way through cleaning this up, I realised that since I don't know what
you're doing in CBButtons1/2, I can't really tell you precisely what to do. I
can't see why you need to create a bunch of variables containing the entries in
the list. Wouldn't it be simpler to just do:

procedure( CBOnArr2Inst(ListIn Mode)
foreach(mapcar elemStr ListIn
;create list of operations
if( Mode == "Switch" then
strcat("CBButtons2(\"" elemStr "\")")
else ;else if/case
strcat("CBButtons1(\"" elemStr "\")")
) ;if
);foreach
) ;proc CBOnArr2Inst

In both cases, there's no need for a prog() or let() because there are no local
variables. I'm using foreach mapcar to transform one list to another.

Regards,

Andrew.

 

[I-F AB]

Hi,
Thanks. I just realised I shifted most of the original functions in
this procedure to other procedures.
It looks like I've now made too much editing to the script an managed
to end up with a ridiculously complicated code doing something simple.
Thanks again.

Best Regards,
I-FAB

 

[Riad KACED]

Hi,

You could try the skill profiler from the skill development
environment. The skill Profiler tells you where your code is taking
the most time and memory. I'm using it quite often to optimize large
scripts even-though I don't find it very easy to understand some
times. That said, the Skill profiler does not give you precious
advises as Andrew did :-(

Regards,
Riad.