Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

Can this be done better

68 views
Skip to first unread message

Cecil Westerhof

unread,
Sep 17, 2019, 6:28:05 AM9/17/19
to
I am a proponent of DRY.

I first had this code:
if {${command} == "*"} {
incr totalFound
set totalCPU [expr {${totalCPU} + ${CPUUsage}}]
} elseif {[regexp "${command}" ${thisCommand}]} {
puts [format ${formatStr} ${CPUUsage} ${thisCommand}]
incr totalFound
set totalCPU [expr {${totalCPU} + ${CPUUsage}}]
}

But I changed it to this code:
if {${command} == "*"} {
set addLine True
} elseif {[regexp "${command}" ${thisCommand}]} {
puts [format ${formatStr} ${CPUUsage} ${thisCommand}]
set addLine True
}
if {${addLine}} {
incr totalFound
set totalCPU [expr {${totalCPU} + ${CPUUsage}}]
}

Just wondering if there is a better way to do this?

--
Cecil Westerhof
Senior Software Engineer
LinkedIn: http://www.linkedin.com/in/cecilwesterhof

heinrichmartin

unread,
Sep 17, 2019, 8:49:46 AM9/17/19
to
On Tuesday, September 17, 2019 at 12:28:05 PM UTC+2, Cecil Westerhof wrote:
> set totalCPU [expr {${totalCPU} + ${CPUUsage}}]

incr totalCPU ${CPUUsage}

> if {${command} == "*"} {
> set addLine True
> } elseif {[regexp "${command}" ${thisCommand}]} {

I'd not make "*" special when a regexp is expected: The empty regexp matches everything. If you must support "*", then

if {"*" eq $command} {set command ""} ;# empty RE matches everything
if {[regexp ...]} {# doit}

No need for building a string "${command}". I guess the bytecode compiler optimizes it, but just ${command} is enough.

I'd rename variable command to commandRE or commandFilter or commandPattern. (Actually, I was about to write that the order of parameters to regexp was wrong, [regexp $commandRE $command].)

Also, you probably know that you can omit the braces for your variable names, but either style is fine.

Rich

unread,
Sep 17, 2019, 12:11:46 PM9/17/19
to
heinrichmartin <martin....@frequentis.com> wrote:
> On Tuesday, September 17, 2019 at 12:28:05 PM UTC+2, Cecil Westerhof wrote:
>> set totalCPU [expr {${totalCPU} + ${CPUUsage}}]
>
> incr totalCPU ${CPUUsage}

Note that this only works when both totalCPU and CPUUsage are integers.

Fractional values will cause incr to error out.

Cecil Westerhof

unread,
Sep 17, 2019, 4:44:07 PM9/17/19
to
heinrichmartin <martin....@frequentis.com> writes:

> On Tuesday, September 17, 2019 at 12:28:05 PM UTC+2, Cecil Westerhof wrote:
>> set totalCPU [expr {${totalCPU} + ${CPUUsage}}]
>
> incr totalCPU ${CPUUsage}

CPUUsage is a float, so incr would not work.


>> if {${command} == "*"} {
>> set addLine True
>> } elseif {[regexp "${command}" ${thisCommand}]} {
>
> I'd not make "*" special when a regexp is expected: The empty regexp
> matches everything. If you must support "*", then

Good point: changed.


> if {[regexp ...]} {# doit}

That does not work. When the regexp is empty (in the new situation) I
do not print the commands, but only calculate the total. Otherwise I
print matching lines.


> No need for building a string "${command}". I guess the bytecode
> compiler optimizes it, but just ${command} is enough.

Changed.


> I'd rename variable command to commandRE or commandFilter or

Good point.


> Also, you probably know that you can omit the braces for your
> variable names, but either style is fine.

I know, but I prefer to use them for variable names that are longer as
three characters.


It has become:
if {${commandRE} == ""} {
set addLine True
} elseif {[regexp ${commandRE} ${command}]} {
puts [format ${formatStr} ${CPUUsage} ${command}]
set addLine True
}
if {${addLine}} {
incr totalFound
set totalCPU [expr {${totalCPU} + ${CPUUsage}}]
}


Rolf Ade

unread,
Sep 17, 2019, 8:19:06 PM9/17/19
to

heinrichmartin <martin....@frequentis.com> writes:
> On Tuesday, September 17, 2019 at 12:28:05 PM UTC+2, Cecil Westerhof wrote:
>> [...]
>> if {${command} == "*"} {
> [...]
> Also, you probably know that you can omit the braces for your variable
> names, but either style is fine.

I disagree with the last opinion. In the so much overwhelming majority
of cases that an almost always is justified it is better to write in
such a case

if {$command == "*"} {

instead of

if {${command} == "*"} {

While it works it's not just as fine.

heinrichmartin

unread,
Sep 18, 2019, 3:28:22 AM9/18/19
to
On Wednesday, September 18, 2019 at 2:19:06 AM UTC+2, Rolf Ade wrote:
> heinrichmartin writes:
> > Also, you probably know that you can omit the braces for your variable
> > names, but either style is fine.
>
> I disagree with the last opinion. In the so much overwhelming majority
> of cases that an almost always is justified it is better to write in
> such a case
>
> if {$command == "*"} {
>
> instead of
>
> if {${command} == "*"} {
>
> While it works it's not just as fine.

This sounds like an option rather than a fact. Would you mind to elaborate?

Rolf Ade

unread,
Sep 18, 2019, 9:36:36 AM9/18/19
to
Since we agree that both ways of writing work in the same way it surely
is an opinion (as I already wrote in my first sentence). It's also not a
big thing but just a matter of style or "best practice".

As obvious it is two characters more (in writing and reading) - for
which advantage? While Cecil, who is as he wrote used to it, may
disagree it doesn't help in understanding the line better or faster.
More the contrary - you've to check a pair of curly braces more.

Since it's an uncommon way of writing a novice may stumble over that,
asking himself what magic or hidden sense is behind this (there is none,
as we both know).

Cecil wrote he prefers this (in my eyes unnecessary complicated) way of
writing. He is of course free to do things in the way he prefers.

This personal freedom doesn't makes it as fine from the viewpoint of a
general "best practice".

(I don't argue lesser characters are always better or clearer but we
discussing a concret line of code. For which this seems pretty clear to
me.)



jda...@gmail.com

unread,
Sep 18, 2019, 11:14:05 AM9/18/19
to
This looks like it is in a loop over multiple commands.
The test of the pattern will be the same for all passes through the loop.

Why not create two separate commands:

generate a list of command information (the puts [format ...] part)
proc listCmds {cmdDataLists} {
foreach cmd $cmdDataLists {
puts [fmtCmdData $cmd]
}
}

fmtCmdData {command CPUUsage ... args} {

format "%16s %8f" $command $CPUUsage ...
}

calculate total usage and number of commands (sum of CPUUsage and list length)


Both would take an optional pattern.

It is likely a new proc to get a list of lists of command data would be required too.

This is not faster, there would be two passes through the data, but it creates modules to build other reports (like one sorted by usage, or showing only the 10 highest usage commands).

Dave B

jda...@gmail.com

unread,
Sep 18, 2019, 11:29:10 AM9/18/19
to
Sorry about that, firefox/google/tab and CR is a bad combination.
What I meant to say:

> The test of the pattern will be the same for all passes through the loop.
>
> Why not create separate commands:
>
> generate a list of command information (the puts [format ...] part)
> proc listCmds {cmdDataLists} {
> foreach cmd $cmdDataLists {
> puts [fmtCmdData {*}$cmd]
> }
> }
>
> proc fmtCmdData {command CPUUsage ... args} {
> # format spec is only used here, no need for variable
> format "%16s %8f" $command $CPUUsage ...
> }
>
> calculate total usage (sum of CPUUsage and list length)
> proc sumUse {cmdDataLists} {
set tot 0
foreach cmd $cmds {
lassign $cmd command CPUUsage ...
set tot [expr {$tot + $CPUUsage}]
# or just use lindex
}
return $tot
}

Number of commands would just be [llength $cmdDataLists]


It is likely a new proc to get a list of lists of command data would be required too. Something like:

proc getCmdData {pattern} {
# build a list of lists (or maybe dicts) of data for each command
set cmds ...
# then filter using pattern
lmap cmd $cmds {
if {[regexp $pattern [lindex $cmd 0]]} {
set $cmd
} else {
continue
0 new messages