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