Over Christmas, I read a book called "Clean Code". My employer lent it 
to me. No, I don't remember who the authors were.

While I don't agree with absolutely everything the book said, it does 
seem to speak a great deal of sense. The book says obvious things like 
DRP and SRP. It says things like "name stuff sensibly" and "don't write 
500-line methods". But it also says more insightful things.

For example, don't mix multiple levels of abstraction in a single block 
of code. Now I don't know about you, but I know for a fact that I have 
*totally* written code where I'm like "OK, so I process each item, and 
oh wait, I need to dump out some HTML here. Oh well, I'll just quickly 
hard-code it in here. It's only small anyway..." It turns out, even for 
small things, actually separating stuff out into a few tiny methods with 
descriptive names *really does* make it that much easier to read the 
code again later. Even if it seems pointless to implement a whole 
separate method just for a few lines of code.

Possibly the best insight the book has to offer is "if you need to read 
the comments to work out what the code does... the code sucks". The 
goal, basically, is to write your code in such a way that it's so 
utterly *obvious* what it does that there's really no need to write 
comments about it. The book goes into quite some detail on circumstances 
where it *is* valid to include comments - e.g., to document obscure 
quirks of external code you're interfacing to or something. But the main 
argument is that you should simplify your code to the point where 
comments become superfluous.

Reading through the book, and looking at the examples, and so on, the 
thing that impressed me is how widely applicable the advice is. The book 
is written in Java, but the advice is (almost) all equally applicable to 
any OO language. But actually, most of this stuff would apply just as 
well to Pascal or Bash scripts... or Haskell.

There are a few things I don't agree with. The book asserts that a 
method should never have a bool argument. Because "if it has a bool 
argument, it does two different things, and you should change it into 
two separate methods instead". Yeah, and are you going to split every 
single client of this method into two methods as well?? Similarly, you 
should never pass an enum to a method, because then it does *multiple* 
things. Again, this seems silly if all the method does is pass that enum 
on to somebody else. And even if it's dealt with locally, I don't see 
how this is "fundamentally bad" (although often you can replace a big 
switch-block with some nice OO polymorphism instead of using enums).

People often complain about Haskell's heavily abbreviated variable 
names. The book says that variable names should be descriptive, but it 
also states that the level of description should be proportional to the 
size of the variable's scope. So if you have a global variable (i.e., 
you are evil), it should have a *really* damned descriptive name. But if 
you have a loop counter that's only in scope for 3 lines of code, it's 
find to use something shorter.

Haskell of course *abounds* with 1-line function definitions. If a 
variable is only in scope for one single line of code, how much of a 
name do you really need? Similarly, when you have a function like "flip" 
that takes a 2-argument function and swaps its two arguments, you could 
name those arguments as "x" and "y", or you could call them 
"first_argument" and "second_argument". But how is that better? The 
shorter names are quicker to read, and it's arguably easier to see 
what's going on. And that's a common theme in Haskell: often you write 
code that's so utterly abstract that it would be difficult to come up 
with meaningfully descriptive names in the first place.

Heavily abbreviating function names, however, is rather less easy to 
justify. Was it *really* necessary to shorten "element" to "elem"? 
Similarly, the book demands that names *tell you* something about the 
items they refer to. The "max" and "maximum" functions are *clearly* 
different functions - but the names yield no clue has to *how* they are 
different. There _is_ some logic there, once you look it up, but it's 
hardly self-describing. ("max" finds the maximum of just two arguments, 
"maximum" finds the maximum element of an entire list of items. So one 
has a short name, and the other a long name. Logical, but not really 
self-describing.)

The same could maybe levelled at the standard class names. We have "Eq", 
"Ord" and "Num" rather than "Equal", "Ordered" and "Number". And 
"number" is a crappy name anyway; Num defines +, - and *, and also abs, 
signum and conversion from Integer. But "/" is defined in Fractional. 
(Not "Frac", thankfully.) Then again, designing a decent number 
hierarchy is *highly* non-trivial, so...

The "id" function could *easily* have been named "identity" instead. 
That would have been way, way more descriptive. But I think the biscuit 
has to go to the horrifyingly misleading "return" function, which is 
*nothing like* what any programmer experienced in C, C++, C#, Java, 
JavaScript, VB, Bash, Pascal, Smalltalk, Tcl, JavaScript... would 
expect. That name has such a ubiquitous pre-existing meaning that to 
define it to do something totally different seems like madness to me...

Post a reply to this message

On 11/01/14 18:13, Orchid Win7 v1 wrote:
> Over Christmas, I read a book called "Clean Code". My employer lent it
> to me. No, I don't remember who the authors were.

Robert Martin. I have a copy

John
-- 
Protect the Earth
It was not given to you by your parents
You hold it in trust for your children

Post a reply to this message

On 11/01/2014 07:09 PM, Doctor John wrote:
> On 11/01/14 18:13, Orchid Win7 v1 wrote:
>> Over Christmas, I read a book called "Clean Code". My employer lent it
>> to me. No, I don't remember who the authors were.
>
> Robert Martin. I have a copy

Good work.

Do you agree that it is righteous?

Post a reply to this message

Orchid Win7 v1 <voi### [at] devnull> wrote:
> Possibly the best insight the book has to offer is "if you need to read 
> the comments to work out what the code does... the code sucks".

There are many situations where comments are extremely helpful, not only
for others, but for the programmer himself.

For example, the implementation of a complex algorithm is often almost
indecipherable without knowing the algorithm in question, and how it
has been implemented in that particular case. Trying to understand a
complex algorithm by reading (uncommented) code only can be really
laborious and difficult.

Describing the algorithm, however, can make it a lot easier to understand
what's going on and save a lot of work.

-- 
                                                          - Warp

Post a reply to this message

On 11/01/2014 08:13 PM, Warp wrote:
> Orchid Win7 v1<voi### [at] devnull>  wrote:
>> Possibly the best insight the book has to offer is "if you need to read
>> the comments to work out what the code does... the code sucks".
>
> There are many situations where comments are extremely helpful, not only
> for others, but for the programmer himself.
>
> For example, the implementation of a complex algorithm is often almost
> indecipherable without knowing the algorithm in question, and how it
> has been implemented in that particular case. Trying to understand a
> complex algorithm by reading (uncommented) code only can be really
> laborious and difficult.
>
> Describing the algorithm, however, can make it a lot easier to understand
> what's going on and save a lot of work.

Indeed. If you're trying to implement the Bellman-Ford algorithm or 
something, some comments are probably merited. And the book says that 
non-obvious design choices are one of the few valid reasons to write 
comments.

I guess the vast majority of Java code (and probably C# and similar 
languages) is just yet-another-order-processing-application or similar. 
Hell, I work in data analysis, and > 80% of the codebase is just user 
management, loading and saving configuration data, and other such 
chores. If you're writing mile after mile of that, who needs comments? 
If you label everything clearly enough, it'll probably be fine.

(One could argue that more complex algorithms can be made readable by 
suitably suggestive labelling... but at some point that stops working, 
IMHO.)

Post a reply to this message

On 11/01/2014 19:51, Orchid Win7 v1 wrote:
> On 11/01/2014 07:09 PM, Doctor John wrote:
>> On 11/01/14 18:13, Orchid Win7 v1 wrote:
>>> Over Christmas, I read a book called "Clean Code". My employer lent it
>>> to me. No, I don't remember who the authors were.
>>
>> Robert Martin. I have a copy
>
> Good work.
>
> Do you agree that it is righteous?
It depends what you mean by righteous (the 60s/70s meaning may not 
accord with today's meaning). However, it makes a lot of sense. I see it 
more as a plea for non-obfuscated code; see it more as as a plea for the 
programming equivalent of the Campaign for Plain English.

John

Post a reply to this message

On Sat, 11 Jan 2014 18:13:44 +0000, Orchid Win7 v1 wrote:

> Possibly the best insight the book has to offer is "if you need to read
> the comments to work out what the code does... the code sucks".

That idea doesn't take into account others who may have to read and fix/
modify the code and their skills with the language.

Self-commenting code, of course, is a good goal - but writing code that 
is readable to you doesn't mean it's readable to everyone.

Comments help bridge that gap, IMHO.

Jim
-- 
"I learned long ago, never to wrestle with a pig. You get dirty, and 
besides, the pig likes it." - George Bernard Shaw

Post a reply to this message

On 12/01/2014 06:10 AM, Jim Henderson wrote:
> On Sat, 11 Jan 2014 18:13:44 +0000, Orchid Win7 v1 wrote:
>
>> Possibly the best insight the book has to offer is "if you need to read
>> the comments to work out what the code does... the code sucks".
>
> That idea doesn't take into account others who may have to read and fix/
> modify the code and their skills with the language.
>
> Self-commenting code, of course, is a good goal - but writing code that
> is readable to you doesn't mean it's readable to everyone.
>
> Comments help bridge that gap, IMHO.

Consider the following code:

   public SortedList<int> Primes(int max)
   {
     SortedList<int> x = new SortedList<int>();
     SortedList<int> y = new SortedList<int>();
     for (int n=2; n<max; n++) x.Add(n);
     while (x.Size() > 0)
     {
       int p = x[0];
       y.Add(p);
       for (int k=1; k*p < max; k++) x.Remove(k*p);
     }
     return y;
   }

Now it doesn't take a genius to work out what this does, but a few 
comments would certainly help make it a bit clearer. There's a couple of 
bits that aren't especially obvious, and by throwing in a comment or 
two, you could clear things up.

But now suppose we refactor it to look like this:

   public SortedList<int> FindPrimesBelow(int max)
   {
     SortedList<int> candidates = InitialiseCandidates(max);
     SortedList<int> primes = new SortedList<int>();

     while (candidates.Size() > 0)
     {
       int prime = GetNextPrime(candidates);
       primes.Add(prime);
       RemovePrimeAndItsMultiples(prime, candidates, max);
     }

     return primes;
   }

   private SortedList<int> InitialiseCandidates(int max)
   {
     SortedList<int> candidates = new SortedList<int>();

     for (int n=2; n<max; n++)
     {
       candidates.Add(n);
     }

     return candidates;
   }

   private int GetNextPrime(SortedList<int> candidates)
   {
     return candidates[0];
   }

   private int RemovePrimeAndItsMultiples(int prime, SortedList<int> 
candidates, int max)
   {
     for (int k=1; k*prime < max; k++)
     {
       candidates.Remove(k*prime);
     }
   }

This now makes it pretty much drop-dead obvious what the hell the 
algorithm does; it builds a list of the numbers from N to Max, and loops 
over them. At each step, it finds the next prime, adds it to the list of 
primes, and then removes it and all its multiples from the list of 
candidates. When the list of candidates becomes empty, it returns the 
list of primes. Simples.

There are few places here where a comment would add much of value. 
Notice how by replacing candidates[0] with GetNextPrime(), we've made it 
obvious what this particular bit of code does, without writing a 
comment. It may not be obvious *why* this works, but it is now clear 
*what* it does.

This seems to be the main argument of the book. Anything non-obvious, 
either put it into a subroutine with a descriptive name, or make it into 
a variable who's name says what it's for. Basically, anything that isn't 
clear, slap a name on it to clear things up.

Notice RemovePrimeAndItsMultiples(). That's a rather verbose name. Lots 
of people would just name this "ProcessPrime()" or something. But that 
doesn't tell you what this "processing" is without looking inside the 
method body. But by calling it RemovePrimeAndItsMultiples(), you can 
tell *exactly* what the method does without ever needing to read its 
contents. You may not know *how* it does its job, but you can tell what 
its job is.

In particular, the name makes it clear that the method removes multiples 
of the prime AND THE PRIME ITSELF. That small but crucial detail wasn't 
especially obvious in the original. Sure, you can work it out easily 
enough, but it's such an important fact that it merits being called out.

When I first started work at my new job, I was surprised at how long 
some of the variable names are. But now I understand what they're trying 
(not necessarily successfully) to do: to make it obvious to anyone 
reading WTF is going on. Now if only we could do that with our class 
names too.

[I'm still bitter that we have two classes both called ItemData, and one 
is a field of the other!! Because *that* couldn't possibly lead to 
confusion at all...]

I don't have the book in front of me now, but prime number sieving is 
actually one of the examples. They even go so far as to remove the "stop 
searching after sqrt(max)" optimisation to "make the code simpler". 
Because most of the time, being able to understand the code is far more 
important than getting maximum performance out of it. Like I said, most 
of the code that *I* write all day is just CRUD. It's all GUI stuff that 
only needs to be as fast as the human sitting at the keyboard.

Post a reply to this message

On Sun, 12 Jan 2014 10:38:16 +0000, Orchid Win7 v1 wrote:

> This seems to be the main argument of the book. Anything non-obvious,
> either put it into a subroutine with a descriptive name, or make it into
> a variable who's name says what it's for. Basically, anything that isn't
> clear, slap a name on it to clear things up.

That doesn't address the situation that it may be obvious to you, but not 
to someone else who's reading the code.

In order to be effective at this, you have to put yourself in the shoes 
of someone who isn't familiar with what you're doing.  I've worked with a 
fair number of developers, and they don't always have that ability.

Jim

-- 
"I learned long ago, never to wrestle with a pig. You get dirty, and 
besides, the pig likes it." - George Bernard Shaw

Post a reply to this message

On 11-1-2014 21:22, Orchid Win7 v1 wrote:
> On 11/01/2014 08:13 PM, Warp wrote:
>> Orchid Win7 v1<voi### [at] devnull>  wrote:
>>> Possibly the best insight the book has to offer is "if you need to read
>>> the comments to work out what the code does... the code sucks".
>>
>> There are many situations where comments are extremely helpful, not only
>> for others, but for the programmer himself.
>>
>> For example, the implementation of a complex algorithm is often almost
>> indecipherable without knowing the algorithm in question, and how it
>> has been implemented in that particular case. Trying to understand a
>> complex algorithm by reading (uncommented) code only can be really
>> laborious and difficult.
>>
>> Describing the algorithm, however, can make it a lot easier to understand
>> what's going on and save a lot of work.
>
> Indeed. If you're trying to implement the Bellman-Ford algorithm or
> something, some comments are probably merited. And the book says that
> non-obvious design choices are one of the few valid reasons to write
> comments.
>
> I guess the vast majority of Java code (and probably C# and similar
> languages) is just yet-another-order-processing-application or similar.
> Hell, I work in data analysis, and > 80% of the codebase is just user
> management, loading and saving configuration data, and other such
> chores. If you're writing mile after mile of that, who needs comments?
> If you label everything clearly enough, it'll probably be fine.
>
> (One could argue that more complex algorithms can be made readable by
> suitably suggestive labelling... but at some point that stops working,
> IMHO.)

I am still using Knuth's literate programming style to document the 
algorithm if it is non-trivial. That way I can have pictures and 
formulas and all other explanatory devices for when comments are really 
needed.


-- 
Everytime the IT department forbids something that a researcher deems
necessary for her work there will be another hole in the firewall.

Post a reply to this message

On 11-1-2014 19:13, Orchid Win7 v1 wrote:
> Over Christmas, I read a book called "Clean Code". My employer lent it
> to me. No, I don't remember who the authors were.
>
> While I don't agree with absolutely everything the book said, it does
> seem to speak a great deal of sense. The book says obvious things like
> DRP and SRP. It says things like "name stuff sensibly" and "don't write
> 500-line methods". But it also says more insightful things.
>
> For example, don't mix multiple levels of abstraction in a single block
> of code. Now I don't know about you, but I know for a fact that I have
> *totally* written code where I'm like "OK, so I process each item, and
> oh wait, I need to dump out some HTML here. Oh well, I'll just quickly
> hard-code it in here. It's only small anyway..." It turns out, even for
> small things, actually separating stuff out into a few tiny methods with
> descriptive names *really does* make it that much easier to read the
> code again later. Even if it seems pointless to implement a whole
> separate method just for a few lines of code.


My rule of thumb is that whenever a piece of code does a single thing 
that you can write a specification for that is independent of the 
context, it should be a subroutine/function/method. With a name that 
describes that specification.
Which would mean that because processing can be specified, and 
generating output can be specified also, and both specifications are 
independent, I would end up with two functions and a wrapper.


I am working with another group that has apparently a different opinion 
on writing code. Their code has numerous instances of this code snippet

     S=getSmode(T,dep,rep, GEOM.SPECS ,4);
     PHI=lowpassma(GEOM.AMA*S,5);	

this is a short version, there are others that do it in one line, take 
subsets and do other stuff as well:

    PSIA =lowpassma(SCAN.AMA * getSmode(ones(length(GEOM.VER),1) * ( 1 : 
SCAN.qrsduration ),dep,SCAN.rep, GEOM.SPECS,SCAN.scanmode,GEOM),SCAN.lpass);

(never mind that most variables have different names in this place for 
no obvious reason, it still does the same thing. There are many more 
unacceptable things in this snippet of code, but let's stick to the 
naming issue here.)

Question: what do you think this does?
Answer below
Would you create a separate function for this, even if it is just one or 
two lines and how would you name that function?

>
> Possibly the best insight the book has to offer is "if you need to read
> the comments to work out what the code does... the code sucks". The
> goal, basically, is to write your code in such a way that it's so
> utterly *obvious* what it does that there's really no need to write
> comments about it. The book goes into quite some detail on circumstances
> where it *is* valid to include comments - e.g., to document obscure
> quirks of external code you're interfacing to or something. But the main
> argument is that you should simplify your code to the point where
> comments become superfluous.

I fully agree, but see my other post

> Reading through the book, and looking at the examples, and so on, the
> thing that impressed me is how widely applicable the advice is. The book
> is written in Java, but the advice is (almost) all equally applicable to
> any OO language. But actually, most of this stuff would apply just as
> well to Pascal or Bash scripts... or Haskell.

Looks like this book is on programming in general. In my opinion there 
is a huge difference between being able to debug a program until it sort 
of works and being a competent programmer. This book is for the latter 
and will be totally lost on the former.

> There are a few things I don't agree with. The book asserts that a
> method should never have a bool argument. Because "if it has a bool
> argument, it does two different things, and you should change it into
> two separate methods instead". Yeah, and are you going to split every
> single client of this method into two methods as well??

I should read the book first, but I agree with your statement. If

  foo(bool,args)

is replace by

  if (foo)
    footrue(args)
  else
    foofalse(args)
  end

always, then I am of the opinion that these 5 lines should be taken 
together as one method, because there is a specification for this entire 
block.
Then either these five lines will be the body of that method, or in case 
they have overlapping computation you should do that first.
BTW in case of overlapping computations in footrue and foofalse that can 
not be logically separated from the part that differs, you should not 
use the footrue/foofalse methods at all. That would imply duplicating 
code that from then on might start to evolve separately, creating 
problems later. (alternative: use literate programming ;) )


> Similarly, you
> should never pass an enum to a method, because then it does *multiple*
> things. Again, this seems silly if all the method does is pass that enum
> on to somebody else. And even if it's dealt with locally, I don't see
> how this is "fundamentally bad" (although often you can replace a big
> switch-block with some nice OO polymorphism instead of using enums).
>
> People often complain about Haskell's heavily abbreviated variable
> names. The book says that variable names should be descriptive, but it
> also states that the level of description should be proportional to the
> size of the variable's scope. So if you have a global variable (i.e.,
> you are evil), it should have a *really* damned descriptive name. But if
> you have a loop counter that's only in scope for 3 lines of code, it's
> find to use something shorter.

My rule is: use descriptive names when possible and random names when 
that is not or when it does not matter. The latter prevents you from 
trying to understand why somebody named a variable thus, when it 
actually does not matter.

> Haskell of course *abounds* with 1-line function definitions. If a
> variable is only in scope for one single line of code, how much of a
> name do you really need? Similarly, when you have a function like "flip"
> that takes a 2-argument function and swaps its two arguments, you could
> name those arguments as "x" and "y", or you could call them
> "first_argument" and "second_argument". But how is that better? The
> shorter names are quicker to read, and it's arguably easier to see
> what's going on. And that's a common theme in Haskell: often you write
> code that's so utterly abstract that it would be difficult to come up
> with meaningfully descriptive names in the first place.
>
> Heavily abbreviating function names, however, is rather less easy to
> justify. Was it *really* necessary to shorten "element" to "elem"?
> Similarly, the book demands that names *tell you* something about the
> items they refer to. The "max" and "maximum" functions are *clearly*
> different functions - but the names yield no clue has to *how* they are
> different. There _is_ some logic there, once you look it up, but it's
> hardly self-describing. ("max" finds the maximum of just two arguments,
> "maximum" finds the maximum element of an entire list of items. So one
> has a short name, and the other a long name. Logical, but not really
> self-describing.)
>
> The same could maybe levelled at the standard class names. We have "Eq",
> "Ord" and "Num" rather than "Equal", "Ordered" and "Number". And
> "number" is a crappy name anyway; Num defines +, - and *, and also abs,
> signum and conversion from Integer. But "/" is defined in Fractional.
> (Not "Frac", thankfully.) Then again, designing a decent number
> hierarchy is *highly* non-trivial, so...
>
> The "id" function could *easily* have been named "identity" instead.
> That would have been way, way more descriptive.

Many of the examples you give are rather common and very ancient, so I 
would be surprised at the longer version.

-----------

Answer: it computes an estimate of the surface ECG from a set of 
activation and recovery times of parts of the ventricle.


-- 
Everytime the IT department forbids something that a researcher deems
necessary for her work there will be another hole in the firewall.

Post a reply to this message

Am 12.01.2014 22:21, schrieb andrel:

> My rule of thumb is that whenever a piece of code does a single thing
> that you can write a specification for that is independent of the
> context, it should be a subroutine/function/method. With a name that
> describes that specification.
> Which would mean that because processing can be specified, and
> generating output can be specified also, and both specifications are
> independent, I would end up with two functions and a wrapper.
>
>
> I am working with another group that has apparently a different opinion
> on writing code. Their code has numerous instances of this code snippet
>
>      S=getSmode(T,dep,rep, GEOM.SPECS ,4);
>      PHI=lowpassma(GEOM.AMA*S,5);
>
> this is a short version, there are others that do it in one line, take
> subsets and do other stuff as well:
>
>     PSIA =lowpassma(SCAN.AMA * getSmode(ones(length(GEOM.VER),1) * ( 1 :
> SCAN.qrsduration ),dep,SCAN.rep,
> GEOM.SPECS,SCAN.scanmode,GEOM),SCAN.lpass);
>
> (never mind that most variables have different names in this place for
> no obvious reason, it still does the same thing. There are many more
> unacceptable things in this snippet of code, but let's stick to the
> naming issue here.)
>
> Question: what do you think this does?

I have no freaking idea.

> Would you create a separate function for this, even if it is just one or
> two lines and how would you name that function?

Absolutely - even though for different reasoning: One of my primary 
personal mantras is that no code sequence should appear more than once 
in any given program - not even (or, especially not!) with slight 
variations.

Probably the most obvious reason for this is a maintainability aspect: 
Having the code sequence in just one place means that if it needs to be 
changed, it only needs to be changed in one single place. But there are 
many other code quality aspects besides:

- If encapsulated in a function, the code sequence gets a better test 
coverage. (If you have copies of it in multiple places, one of the 
copies might have a typo that the others don't, and if Murphy's right 
your test cases will be such that they'd spot the bug in any of the 
copies except the one it happens to be in).

- If encapsulated in a function, you can more easily do unit tests on 
this particular code sequence.

- If encapsulated in a function, fixing a bug in the code sequence for 
one instance where it is used means fixing it for all instances where it 
is used.

- If encapsulated in a function, you can more easily document any 
assumptions the code sequence relies on. You also have a better chance 
of verifying that those assumptions are valid everywhere the code 
sequence is used.

- If encapsulated in a function, well-documented and well-tested, you 
have a reliable building brick to use in new features.


As for a function's name, I find it very important that it denotes not 
/how/ the function does something, nor just /what/ it does, but also 
with what /intention/.


For instance, while refactoring the old POV-Ray code I came across 
plenty of places where old code computes /some/ scalar measure of an RGB 
colour's "intensity": Some just take the value of the brightest colour 
component, i.e. max(r,g,b); others take the greyscale value of the 
colour, i.e. (r*R)+(g*G)+b*B) with some constant R,G,B. Some use 
absolute values, some just keep negative values around. And some add 
some more twists to it.

Why are there so many variations on this theme?

Chances are, at least some of these don't always do what they should do. 
Some will probably break down for special cases, while some may work 
equally well for all cases but are fundamentally unsuited for the 
purpose they're used for.


As a first step towards cleaning up all this mess, I ended up with six 
different functions:

(1) A function to be called to compute a colour's greyscale value.

(2) A function to be called to compute how intense the most intense 
colour channel is.

(3) A function to be called to compute some more or less abstract 
"weight" or "importance" from a colour, currently implemented as the 
colour's greyscale value. (This reeks of bogosity, and will probably be 
replaced by (5) later.)

(4) A function to be called to compute some more or less abstract 
"weight" or "importance" from a colour, currently implemented as the 
intensity of the most intense colour channel. (This, too, reeks of 
bogosity, and will probably be replaced by (6) later.)

(5) A function to be called to compute some more or less abstract 
"weight" or "importance" from a colour, currently implemented as the 
/absolute/ of the colour's greyscale value. (Less bogus than (3), but 
ultimately to be revised as well.)

(6) A function to be called to compute some more or less abstract 
"weight" or "importance" from a colour, currently implemented as the 
highest /absolute/ intensity of all the colour channels. (Less bogus 
than (4), but ultimately to be revised as well.)


Note how the functions (1) and (3), as well as (2) and (4), do exactly 
the same thing - yet I decided to separate them, because they do it for 
different purposes. (In order to avoid duplicate code, (3) and (4) 
actually just call (1) and (2), respectively.)

Also note how (3) to (6) all do whatever they do for apparently the very 
same purpose - computing some more or less abstract "weight" or 
"importance" from a colour. So the fact that there exist four 
functionally different implementations, without any documented reason 
whatsoever, is a strong indication that at least three of them are wrong.

After some serious reasoning, I came to the conclusion that all four of 
them were wanting, and that the task was best solved by yet another, 
seventh function:


(7) A function to be called to compute some more or less abstract 
"weight" or "importance" from a colour, implemented as the average of 
the absolute of each colour channel.


The intended transition from (3)-(6) to (7) should be an easy job now 
that they're all encapsulated into functions, but only because I have 
distinguished between (1)-(2) and (3)-(4) right from the start.

Post a reply to this message

On 11/01/2014 06:13 PM, Orchid Win7 v1 wrote:
> There are a few things I don't agree with. The book asserts that a
> method should never have a bool argument. Because "if it has a bool
> argument, it does two different things, and you should change it into
> two separate methods instead". Yeah, and are you going to split every
> single client of this method into two methods as well??

To be fair, I think the expectation here is that some clients only use 
one variant, and other clients only use the other.

For example, you wouldn't write one function that takes a bool and a 
handle, and either opens or closes that handle based on the bool; you'd 
write one function to open handles, and another to close them. It would 
be silly to do otherwise.

But then again, would you write one function to open handles in read 
mode, and another to open them in write mode? That seems daft to me.

The other thing that concerns me is the potential amount of code 
duplication if you split one function into two. Maybe that bool 
parameter only affects one tiny weeny little portion of the function's 
contents. Do we really need to duplicate all that??

On the first hand again, I have certainly seen functions that take a 
bool parameter and do so much jiggery-pokery with it internally to get 
the right thing to happen in both cases that it would seem _more_ 
sensible to just write two functions that people can actually read.

I guess it depends on just how similar or not the two actions are.

Post a reply to this message

> But now suppose we refactor it to look like this:
>
>    public SortedList<int> FindPrimesBelow(int max)
>    {
>      SortedList<int> candidates = InitialiseCandidates(max);
>      SortedList<int> primes = new SortedList<int>();
>
>      while (candidates.Size() > 0)
>      {
>        int prime = GetNextPrime(candidates);
>        primes.Add(prime);
>        RemovePrimeAndItsMultiples(prime, candidates, max);
>      }
>
>      return primes;
>    }
>
>    private SortedList<int> InitialiseCandidates(int max)
>    {
>      SortedList<int> candidates = new SortedList<int>();
>
>      for (int n=2; n<max; n++)
>      {
>        candidates.Add(n);
>      }
>
>      return candidates;
>    }
>
>    private int GetNextPrime(SortedList<int> candidates)
>    {
>      return candidates[0];
>    }
>
>    private int RemovePrimeAndItsMultiples(int prime, SortedList<int>
> candidates, int max)
>    {
>      for (int k=1; k*prime < max; k++)
>      {
>        candidates.Remove(k*prime);
>      }
>    }

So you're writing 3x as much code just to avoid comments? Is the above 
really that much more readable to justify the extra work writing it?

public SortedList<int> Primes(int max)
   {
     SortedList<int> candidates = new SortedList<int>();
     SortedList<int> primes = new SortedList<int>();
     for (int n=2; n<max; n++) candidates.Add(n); // initialise
     while (candidates.Size() > 0)
     {
       int p = candidates[0]; // get next prime
       primes.Add(p);
       for (int k=1; k*p < max; k++) candidates.Remove(k*p); // remove 
multiples
     }
     return primes;
   }

Post a reply to this message

On 13-1-2014 0:56, clipka wrote:
> Am 12.01.2014 22:21, schrieb andrel:

>> I am working with another group that has apparently a different opinion
>> on writing code. Their code has numerous instances of this code snippet
>>
>>      S=getSmode(T,dep,rep, GEOM.SPECS ,4);
>>      PHI=lowpassma(GEOM.AMA*S,5);
>>
>> this is a short version, there are others that do it in one line, take
>> subsets and do other stuff as well:
>>
>>     PSIA =lowpassma(SCAN.AMA * getSmode(ones(length(GEOM.VER),1) * ( 1 :
>> SCAN.qrsduration ),dep,SCAN.rep,
>> GEOM.SPECS,SCAN.scanmode,GEOM),SCAN.lpass);
>>
>> (never mind that most variables have different names in this place for
>> no obvious reason, it still does the same thing. There are many more
>> unacceptable things in this snippet of code, but let's stick to the
>> naming issue here.)
>>
>> Question: what do you think this does?
>
> I have no freaking idea.
>

Isn't it obvious that the S is a description of the local transmembrane 
potentials (TMP) in time? And that with a simple matrix multiplication 
with a transfermatrix that describes the influence of every point on the 
heart to every point on the torso, you can compute an ECG? And that that 
has very high frequency noise because of the coarse mesh at the heart 
surface, so you should filter that with a moving average filter?

The original formulation on the back of the envelop was apparently that

PHI=A*S

Hence the name "getS" for computing TMP from timing. Later various 
versions were introduces that depended on the "mode", even if now only 
mode 4 is used, it still shows. The A in the formula you can find back 
in AMA which is short for A-MAtrix.
IMHO the moving average is not the right type of filter, but you can not 
change that without changing all instances of this call. One of the 
other things that is very wrong here.
And did I complain the "lpass' is in samples not in frequency or even 
milliseconds and that it should be odd (but that that is not 
documented)? And that it is set to 5 in a number of places in the code? 
And that lowpassma has a bug?
And... long story short: I am not happy with the programming skills of 
this group.

>> Would you create a separate function for this, even if it is just one or
>> two lines and how would you name that function?
>
> Absolutely - even though for different reasoning: One of my primary
> personal mantras is that no code sequence should appear more than once
> in any given program - not even (or, especially not!) with slight
> variations.

I fully agree, that is also one of my reasons. For me it is so obvious 
that I would not even mention it. It took me as a surprise that other 
people that consider themselves skilled programmers can produce this code.

<snipped more open doors (that are apparently closed to some)>

yes I fully agree, <sigh> :(

>
> For instance, while refactoring the old POV-Ray code I came across
> plenty of places where old code computes /some/ scalar measure of an RGB
> colour's "intensity": Some just take the value of the brightest colour
> component, i.e. max(r,g,b); others take the greyscale value of the
> colour, i.e. (r*R)+(g*G)+b*B) with some constant R,G,B. Some use
> absolute values, some just keep negative values around. And some add
> some more twists to it.
>
> Why are there so many variations on this theme?
>
> Chances are, at least some of these don't always do what they should do.
> Some will probably break down for special cases, while some may work
> equally well for all cases but are fundamentally unsuited for the
> purpose they're used for.
>
>
> As a first step towards cleaning up all this mess, I ended up with six
> different functions:
>
> (1) A function to be called to compute a colour's greyscale value.
>
> (2) A function to be called to compute how intense the most intense
> colour channel is.
>
> (3) A function to be called to compute some more or less abstract
> "weight" or "importance" from a colour, currently implemented as the
> colour's greyscale value. (This reeks of bogosity, and will probably be
> replaced by (5) later.)
>
> (4) A function to be called to compute some more or less abstract
> "weight" or "importance" from a colour, currently implemented as the
> intensity of the most intense colour channel. (This, too, reeks of
> bogosity, and will probably be replaced by (6) later.)
>
> (5) A function to be called to compute some more or less abstract
> "weight" or "importance" from a colour, currently implemented as the
> /absolute/ of the colour's greyscale value. (Less bogus than (3), but
> ultimately to be revised as well.)
>
> (6) A function to be called to compute some more or less abstract
> "weight" or "importance" from a colour, currently implemented as the
> highest /absolute/ intensity of all the colour channels. (Less bogus
> than (4), but ultimately to be revised as well.)
>

Did I miss something along the lines of 
http://en.wikipedia.org/wiki/Luma_(video) in this list?



-- 
Everytime the IT department forbids something that a researcher deems
necessary for her work there will be another hole in the firewall.

Post a reply to this message

On 13-1-2014 0:56, clipka wrote:
> Am 12.01.2014 22:21, schrieb andrel:
>

> As for a function's name, I find it very important that it denotes not
> /how/ the function does something, nor just /what/ it does, but also
> with what /intention/.
>
True, but this is a general case where the point is that there are still 
several ways to do things. More specific names are needed lower in the tree.
In this case I would go for something like:

ECG=ComputeECGfromTiming(Timing,VolumeConductionModel,ElectricalModel, 
Parameters);

or simply

ECG=ComputeECGfromTiming(Timing,ForwardModel);



-- 
Everytime the IT department forbids something that a researcher deems
necessary for her work there will be another hole in the firewall.

Post a reply to this message

Am 13.01.2014 11:19, schrieb andrel:
> On 13-1-2014 0:56, clipka wrote:
>> Am 12.01.2014 22:21, schrieb andrel:
>>
>
>> As for a function's name, I find it very important that it denotes not
>> /how/ the function does something, nor just /what/ it does, but also
>> with what /intention/.
>>
> True, but this is a general case where the point is that there are still
> several ways to do things. More specific names are needed lower in the
> tree.
> In this case I would go for something like:
>
> ECG=ComputeECGfromTiming(Timing,VolumeConductionModel,ElectricalModel,
> Parameters);
>
> or simply
>
> ECG=ComputeECGfromTiming(Timing,ForwardModel);

Sounds like a very good choice to me.

Post a reply to this message

Am 13.01.2014 09:54, schrieb scott:
>> But now suppose we refactor it to look like this:
>>
>>    public SortedList<int> FindPrimesBelow(int max)
>>    {
>>      SortedList<int> candidates = InitialiseCandidates(max);
>>      SortedList<int> primes = new SortedList<int>();
>>
>>      while (candidates.Size() > 0)
>>      {
>>        int prime = GetNextPrime(candidates);
>>        primes.Add(prime);
>>        RemovePrimeAndItsMultiples(prime, candidates, max);
>>      }
>>
>>      return primes;
>>    }
>>
>>    private SortedList<int> InitialiseCandidates(int max)
>>    {
>>      SortedList<int> candidates = new SortedList<int>();
>>
>>      for (int n=2; n<max; n++)
>>      {
>>        candidates.Add(n);
>>      }
>>
>>      return candidates;
>>    }
>>
>>    private int GetNextPrime(SortedList<int> candidates)
>>    {
>>      return candidates[0];
>>    }
>>
>>    private int RemovePrimeAndItsMultiples(int prime, SortedList<int>
>> candidates, int max)
>>    {
>>      for (int k=1; k*prime < max; k++)
>>      {
>>        candidates.Remove(k*prime);
>>      }
>>    }
>
> So you're writing 3x as much code just to avoid comments? Is the above
> really that much more readable to justify the extra work writing it?
>
> public SortedList<int> Primes(int max)
>    {
>      SortedList<int> candidates = new SortedList<int>();
>      SortedList<int> primes = new SortedList<int>();
>      for (int n=2; n<max; n++) candidates.Add(n); // initialise
>      while (candidates.Size() > 0)
>      {
>        int p = candidates[0]; // get next prime
>        primes.Add(p);
>        for (int k=1; k*p < max; k++) candidates.Remove(k*p); // remove
> multiples
>      }
>      return primes;
>    }

There is one major advantage in "commenting" your code by structuring it 
and choosing good identifiers, rather than placing comments in there: 
Comments are typically more prone to become outdated over time.

(Provided of course that the code is produced and maintained in an 
environment where refactoring is encouraged. If the policy is "try to 
avoid touching any of the existing code", it is easier to fix a comment 
that has become obsolete, rather than a once-good identifier that no 
longer matches what the function or variable does.)


Oh, and yes: I do think Andy's version is indeed easier to read. One 
aspect is about visual perception: In your version, the reader has to 
first figure out what is code and what is explanation, whereas in Andy's 
version it's one and the same.

Another aspect is that Andy's version provides clear-cut interfaces 
between the individual subsections of code. For instance, it makes it 
immediately obvious that the step to remove a prime and its multiples 
from the candidates doesn't access the "primes" list, but needs the 
"max" parameter; in your version, inspection of the actual 
implementation of that step is needed to figure that out.

Note that although all this figuring-out is quick to do, it still 
requires concentration, and constitutes a potential source for mistakes.

Post a reply to this message

On 13/01/2014 06:47 PM, clipka wrote:

> There is one major advantage in "commenting" your code by structuring it
> and choosing good identifiers, rather than placing comments in there:
> Comments are typically more prone to become outdated over time.

This is another of the major themes of the book. Inaccurate comments are 
arguably *worse* than no comments at all.

> (Provided of course that the code is produced and maintained in an
> environment where refactoring is encouraged. If the policy is "try to
> avoid touching any of the existing code", it is easier to fix a comment
> that has become obsolete, rather than a once-good identifier that no
> longer matches what the function or variable does.)

One of the things that frustrates me about my job is that I want to 
refactor things, and people complain that it would take too long and so 
we won't do it.

Obviously the customers aren't going to be too impressed if we spent 2 
years restructuring the codebase with no user-visible change in 
functionality. But that doesn't mean that *all* refactoring should be 
put off...

Post a reply to this message

Le 2014-01-12 05:38, Orchid Win7 v1 a écrit :
>
> I don't have the book in front of me now, but prime number sieving is
> actually one of the examples. They even go so far as to remove the "stop
> searching after sqrt(max)" optimisation to "make the code simpler".
> Because most of the time, being able to understand the code is far more
> important than getting maximum performance out of it.

If you do leave it in, *that* is worthy of a comment.  Not every 
programmer will be good enough with math to figure out why you don't 
need to check for factors > sqrt(max).


-- 
/*Francois Labreque*/#local a=x+y;#local b=x+a;#local c=a+b;#macro P(F//
/*    flabreque    */L)polygon{5,F,F+z,L+z,L,F pigment{rgb 9}}#end union
/*        @        */{P(0,a)P(a,b)P(b,c)P(2*a,2*b)P(2*b,b+c)P(b+c,<2,3>)
/*   gmail.com     */}camera{orthographic location<6,1.25,-6>look_at a }

Post a reply to this message

On 13/01/2014 09:36 PM, Francois Labreque wrote:
> Le 2014-01-12 05:38, Orchid Win7 v1 a écrit :
>>
>> I don't have the book in front of me now, but prime number sieving is
>> actually one of the examples. They even go so far as to remove the "stop
>> searching after sqrt(max)" optimisation to "make the code simpler".
>> Because most of the time, being able to understand the code is far more
>> important than getting maximum performance out of it.
>
> If you do leave it in, *that* is worthy of a comment. Not every
> programmer will be good enough with math to figure out why you don't
> need to check for factors > sqrt(max).

I agree. That's the sort of thing that's sufficiently non-obvious that 
it ought to be explained somewhere.

Then again, how many programs sieve prime numbers? Most software seems 
to just push database records from A to B and back again...

Post a reply to this message

On 13-1-2014 19:19, clipka wrote:
> Am 13.01.2014 11:19, schrieb andrel:
>> On 13-1-2014 0:56, clipka wrote:
>>> Am 12.01.2014 22:21, schrieb andrel:
>>>
>>
>>> As for a function's name, I find it very important that it denotes not
>>> /how/ the function does something, nor just /what/ it does, but also
>>> with what /intention/.
>>>
>> True, but this is a general case where the point is that there are still
>> several ways to do things. More specific names are needed lower in the
>> tree.
>> In this case I would go for something like:
>>
>> ECG=ComputeECGfromTiming(Timing,VolumeConductionModel,ElectricalModel,
>> Parameters);
>>
>> or simply
>>
>> ECG=ComputeECGfromTiming(Timing,ForwardModel);
>
> Sounds like a very good choice to me.
>

Well, luckily I am not the one that decides.
OTOH I am sort of forced to use it even if it is IMO unsuitable for 
research purposes. Perhaps you can understand now some of the reasons why.
One day I might just stand up and give a short introduction on 
programming. Which will probably be the end of the cooperation.


-- 
Everytime the IT department forbids something that a researcher deems
necessary for her work there will be another hole in the firewall.

Post a reply to this message

On 13-1-2014 21:00, Orchid Win7 v1 wrote:
> On 13/01/2014 06:47 PM, clipka wrote:
>
>> There is one major advantage in "commenting" your code by structuring it
>> and choosing good identifiers, rather than placing comments in there:
>> Comments are typically more prone to become outdated over time.
>
> This is another of the major themes of the book. Inaccurate comments are
> arguably *worse* than no comments at all.
>
>> (Provided of course that the code is produced and maintained in an
>> environment where refactoring is encouraged. If the policy is "try to
>> avoid touching any of the existing code", it is easier to fix a comment
>> that has become obsolete, rather than a once-good identifier that no
>> longer matches what the function or variable does.)
>
> One of the things that frustrates me about my job is that I want to
> refactor things, and people complain that it would take too long and so
> we won't do it.
>
> Obviously the customers aren't going to be too impressed if we spent 2
> years restructuring the codebase with no user-visible change in
> functionality. But that doesn't mean that *all* refactoring should be
> put off...

When I was working with a colleague we sometimes split. One did all the 
interaction with users and the other did the cleanup in a branch.
Now I am just by myself I need to do it incrementally, which does not 
work, because I do too many things already. But basically you should 
think of it as on-the-fly-garbage-collection. (With the disadvantage 
that you first need to cleanup and rewrite to support that)

-- 
Everytime the IT department forbids something that a researcher deems
necessary for her work there will be another hole in the firewall.

Post a reply to this message

On 13-1-2014 23:04, Orchid Win7 v1 wrote:
> On 13/01/2014 09:36 PM, Francois Labreque wrote:
>> Le 2014-01-12 05:38, Orchid Win7 v1 a écrit :
>>>
>>> I don't have the book in front of me now, but prime number sieving is
>>> actually one of the examples. They even go so far as to remove the "stop
>>> searching after sqrt(max)" optimisation to "make the code simpler".
>>> Because most of the time, being able to understand the code is far more
>>> important than getting maximum performance out of it.
>>
>> If you do leave it in, *that* is worthy of a comment. Not every
>> programmer will be good enough with math to figure out why you don't
>> need to check for factors > sqrt(max).
>
> I agree. That's the sort of thing that's sufficiently non-obvious that
> it ought to be explained somewhere.
>
> Then again, how many programs sieve prime numbers? Most software seems
> to just push database records from A to B and back again...

Depends on your field of work ;) I don't.

-- 
Everytime the IT department forbids something that a researcher deems
necessary for her work there will be another hole in the firewall.

Post a reply to this message

Am 13.01.2014 21:00, schrieb Orchid Win7 v1:

> One of the things that frustrates me about my job is that I want to
> refactor things, and people complain that it would take too long and so
> we won't do it.
>
> Obviously the customers aren't going to be too impressed if we spent 2
> years restructuring the codebase with no user-visible change in
> functionality. But that doesn't mean that *all* refactoring should be
> put off...

Remember that refactoring is not the same as rewriting. Refactoring - at 
least as I understand it - is a sequence of many small baby steps. 
Ideally, the steps should be small enough that you can formally prove 
that they don't change functionality in any way. Such as moving some 
portion of code into a separate function. Or making a public member 
variable protected and providing getter and setter functions for access. 
Or renaming a function.

Each such step is a separate refactoring action, so it shouldn't be too 
difficult to interweave it with your daily job.

Make it a habit to always be a bit more invasive than necessary whenever 
you need to make a change, so that further modifications in that same 
portion of code will be easier.

Make it a habit to set aside, say, two days per month for refactoring work.

Explain to your boss why you're doing this, in terms he can understand: 
You're investing time to improve overall code quality, in order to save 
more time in the future.

Also tell your boss that this is the kind of smart thinking he hired you 
for.

And to stay on good terms with your colleagues, make it clear to them, 
your boss, and also to yourself, that it is your /personal/ desire to 
improve the overall code quality, and that it's ok for you if your 
colleagues don't join in the effort.

Post a reply to this message

>> One of the things that frustrates me about my job is that I want to
>> refactor things, and people complain that it would take too long and so
>> we won't do it.
>>
>> Obviously the customers aren't going to be too impressed if we spent 2
>> years restructuring the codebase with no user-visible change in
>> functionality. But that doesn't mean that *all* refactoring should be
>> put off...
>
> Remember that refactoring is not the same as rewriting. Refactoring - at
> least as I understand it - is a sequence of many small baby steps.
> Ideally, the steps should be small enough that you can formally prove
> that they don't change functionality in any way. Such as moving some
> portion of code into a separate function. Or making a public member
> variable protected and providing getter and setter functions for access.
> Or renaming a function.
>
> Each such step is a separate refactoring action, so it shouldn't be too
> difficult to interweave it with your daily job.

Another theme of the book. The author states that your goal should be to 
have the class fully covered in unit tests, and each change must keep 
all the unit tests working.

Suffice it to say, changing code in a way that doesn't break existing 
stuff is far, far harder than just throwing the lot in the bin and 
starting again.

The other Fun Thing is that there are subsystems which are used 
*everywhere*. Some of them have awful design, we all know it's awful, 
but if you changed it, you would have to touch every client of that 
subsystem - i.e., the entire application. For this reason, we never make 
any improvements to these systems.

I personally am of the opinion that it would be *that* damned hard to 
change all the code; there are automated refactoring tools and such to 
help you do it. But it seems I am in the minority...

Post a reply to this message

> One
> aspect is about visual perception: In your version, the reader has to
> first figure out what is code and what is explanation, whereas in Andy's
> version it's one and the same.

Except Andy's version doesn't actually have any code, it's hidden away 
in a separate method (which you would either need to search or scroll to).

Especially with comments being highlighted in most code editors, I 
personally find this much more useful:

prime = array[0]; // get next prime

thank this:

prime = GetNextPrime(array);

What seems to be being advocated is to replace source code like this:

<trivial code> <comment>

with this:

<call a function with "comment" name>
...
<function definition of "comment">
  <trivial code>
</>

Sorry I don't see the benefit.

Post a reply to this message

Am 15.01.2014 10:57, schrieb scott:
>> One
>> aspect is about visual perception: In your version, the reader has to
>> first figure out what is code and what is explanation, whereas in Andy's
>> version it's one and the same.
>
> Except Andy's version doesn't actually have any code, it's hidden away
> in a separate method (which you would either need to search or scroll to).

... if I'd really bother. Which is exactly the point: Usually I won't.

> Especially with comments being highlighted in most code editors, I
> personally find this much more useful:
>
> prime = array[0]; // get next prime
>
> thank this:
>
> prime = GetNextPrime(array);

Whoever invented code highlighting does indeed deserve a medal or two.

Typically though, comments are actually "lowlighted"; while it does make 
it easier to distinguish between code and comments, it still draws the 
attention to the code rather than the comments.


> What seems to be being advocated is to replace source code like this:
>
> <trivial code> <comment>
>
> with this:
>
> <call a function with "comment" name>
> ...
> <function definition of "comment">
>   <trivial code>
> </>
>
> Sorry I don't see the benefit.

As I said, visual perception is only one aspect.

I also don't think the single-line case is typical in practice.

Then there's also the aspect that if you move into functions some 
multi-line code sections operating at array access level, it can be 
argued that the remaining single-line code sections operating at array 
access level should be moved into functions as well, in order to get a 
homogenous level of abstraction.

And last not least, I'm not really advocating the above - I just present 
arguments in favor (which probably sounds/reads just the same, but 
actually isn't). I prefer to leave it up to the reader to decide for 
themselves.

Post a reply to this message