tufty
Posts: 1456
Joined: Sun Sep 11, 2011 2:32 pm

Re: Making readable modifyable code.

Tue May 22, 2018 1:04 pm

jahboater wrote:
Tue Jan 09, 2018 8:26 am
The argument that simplicity takes priority over correctness is interesting.
Sounds wrong doesn't it - since correctness is an absolute.
But simplicity engenders correctness, both now and in the future.

Code: Select all

// Not guaranteed 100% correct, but very simple and efficient.
float average(float * values, int count) {
  return 4;
}

ejolson
Posts: 2006
Joined: Tue Mar 18, 2014 11:47 am

Re: Making readable modifyable code.

Tue May 22, 2018 4:30 pm

tufty wrote:
Tue May 22, 2018 1:04 pm

Code: Select all

// Not guaranteed 100% correct, but very simple and efficient.
float average(float * values, int count) {
  return 4;
}
In modern C that should be written

Code: Select all

float average(int count,float values[count]) {
  return 3.2;
}
which is simpler because it avoids the explicit use of pointers. It is also more correct because returning 3.2 allows it to pass the unit test.

User avatar
PeterO
Posts: 4257
Joined: Sun Jul 22, 2012 4:14 pm

Re: Making readable modifyable code.

Tue May 22, 2018 4:53 pm

ejolson wrote:
Tue May 22, 2018 4:30 pm
tufty wrote:
Tue May 22, 2018 1:04 pm

Code: Select all

// Not guaranteed 100% correct, but very simple and efficient.
float average(float * values, int count) {
  return 4;
}
In modern C that should be written

Code: Select all

float average(int count,float values[count]) {
  return 3.2;
}
which is simpler because it avoids the explicit use of pointers. It is also more correct because returning 3.2 allows it to pass the unit test.
But it still fails a "clean compile" test because count and values are unused!
PeterO
Discoverer of the PI2 XENON DEATH FLASH!
Interests: C,Python,PIC,Electronics,Ham Radio (G0DZB),Aeromodelling,1960s British Computers.
"The primary requirement (as we've always seen in your examples) is that the code is readable. " Dougie Lawson

jahboater
Posts: 3027
Joined: Wed Feb 04, 2015 6:38 pm

Re: Making readable modifyable code.

Tue May 22, 2018 5:30 pm

:) :) :)
ejolson wrote:
Tue May 22, 2018 4:30 pm
It is also more correct because returning 3.2 allows it to pass the unit test.

Code: Select all

try.c: In function ‘average’:
try.c:13:10: warning: conversion from ‘double’ to ‘float’ changes value from ‘3.2000000000000002e+0’ to ‘3.20000005e+0f’ [-Wfloat-conversion]
   return 3.2;
          ^~~
It should be 3.2F

3.2 isn't exactly representable ....

tufty
Posts: 1456
Joined: Sun Sep 11, 2011 2:32 pm

Re: Making readable modifyable code.

Tue May 22, 2018 5:38 pm

PeterO wrote:
Tue May 22, 2018 4:53 pm
But it still fails a "clean compile" test because count and values are unused!
Nope.

Code: Select all

thinkpad:Desktop simon$ cat > foo.c <<EOF
> // Not guaranteed 100% correct, but very simple and efficient.
> float average(float * values, int count) {
>   return 4;
> }
> EOF
thinkpad:Desktop simon$ clang -c -std=c99 -Wall -Wunused-argument -Werror -o foo.o foo.c
thinkpad:Desktop simon$

jahboater
Posts: 3027
Joined: Wed Feb 04, 2015 6:38 pm

Re: Making readable modifyable code.

Tue May 22, 2018 5:41 pm

GCC complains:

Code: Select all

try.c: In function ‘average’:
try.c:13:23: warning: unused parameter ‘values’ [-Wunused-parameter]
 float average(float * values, int count) {
               ~~~~~~~~^~~~~~
try.c:13:35: warning: unused parameter ‘count’ [-Wunused-parameter]
 float average(float * values, int count) {
                               ~~~~^~~~~

ejolson
Posts: 2006
Joined: Tue Mar 18, 2014 11:47 am

Re: Making readable modifyable code.

Tue May 22, 2018 5:56 pm

tufty wrote:
Tue May 22, 2018 5:38 pm
Nope.

Code: Select all

thinkpad:Desktop simon$ cat > foo.c <<EOF
> // Not guaranteed 100% correct, but very simple and efficient.
> float average(float * values, int count) {
>   return 4;
> }
> EOF
thinkpad:Desktop simon$ clang -c -std=c99 -Wall -Wunused-argument -Werror -o foo.o foo.c
thinkpad:Desktop simon$
Try turning on the optimiser.
Last edited by ejolson on Tue May 22, 2018 6:00 pm, edited 1 time in total.

User avatar
PeterO
Posts: 4257
Joined: Sun Jul 22, 2012 4:14 pm

Re: Making readable modifyable code.

Tue May 22, 2018 5:59 pm

tufty wrote:
Tue May 22, 2018 5:38 pm
thinkpad:Desktop simon$ clang -c -std=c99 -Wall -Wunused-argument -Werror -o foo.o foo.c
thinkpad:Desktop simon$[/code]
Nowhere near enough warnings enabled for a "clean compile" test... :D
After a discussion on here some months ago I now use this set in all my projects.

Code: Select all

-std=gnu99  -Wall -Wextra -Wunused -Wconversion -Wundef -Wcast-qual -Wmissing-prototypes 
-Wredundant-decls -Wunreachable-code -Wwrite-strings -Warray-bounds
-Wstrict-aliasing=3 -Wstrict-overflow=1 -Wstrict-prototypes -Winline
-Wshadow -Wswitch -Wmissing-include-dirs -Woverlength-strings -Wpacked
-Wdisabled-optimization  -Wformat=2 -Winit-self
-Wunused-parameter -Wlogical-op -Wuninitialized
-Wnested-externs -Wpointer-arith -Wdouble-promotion -Wunused-macros
-Wunsafe-loop-optimizations
PeterO
Discoverer of the PI2 XENON DEATH FLASH!
Interests: C,Python,PIC,Electronics,Ham Radio (G0DZB),Aeromodelling,1960s British Computers.
"The primary requirement (as we've always seen in your examples) is that the code is readable. " Dougie Lawson

jahboater
Posts: 3027
Joined: Wed Feb 04, 2015 6:38 pm

Re: Making readable modifyable code.

Tue May 22, 2018 6:03 pm

:) Compilers have moved on - nowadays I use:

Code: Select all

-Wfatal-errors -Wall -Wextra -Wconversion -Wunused -Wundef -Wcast-qual \
-Wredundant-decls -Wunreachable-code -Wwrite-strings -Warray-bounds \
-Wstrict-aliasing=3 -Wstrict-prototypes -Winline -Wshadow -Wswitch -Wformat=2 \
-Wmissing-include-dirs -Woverlength-strings -Wpacked -Wdisabled-optimization \
-Wmissing-prototypes -Winit-self -Wmissing-declarations -Wunused-parameter \
-Wlogical-op -Wuninitialized -Wnested-externs -Wpointer-arith -Wdouble-promotion \
-Wunused-macros -Wunused-function -Wunsafe-loop-optimizations -Wnull-dereference \
-Wduplicated-cond -Wshift-overflow=2 -Wnonnull -Wcast-align -Warray-bounds=2 \
-Wsuggest-attribute=malloc -Wmultistatement-macros -Wstringop-truncation \
-Wpacked-not-aligned -Wsizeof-pointer-div -Wcast-align=strict -Wrestrict \
-Wformat-overflow -Wformat-truncation -Wif-not-aligned
and -std=c18 !

ejolson
Posts: 2006
Joined: Tue Mar 18, 2014 11:47 am

Re: Making readable modifyable code.

Tue May 22, 2018 6:48 pm

jahboater wrote:
Tue May 22, 2018 5:30 pm
:) :) :)
ejolson wrote:
Tue May 22, 2018 4:30 pm
It is also more correct because returning 3.2 allows it to pass the unit test.

Code: Select all

try.c: In function ‘average’:
try.c:13:10: warning: conversion from ‘double’ to ‘float’ changes value from ‘3.2000000000000002e+0’ to ‘3.20000005e+0f’ [-Wfloat-conversion]
   return 3.2;
          ^~~
It should be 3.2F

3.2 isn't exactly representable ....
Hmm. Maybe simple is more important than a clean compile. Better update the unit test.

ejolson
Posts: 2006
Joined: Tue Mar 18, 2014 11:47 am

Re: Making readable modifyable code.

Tue May 29, 2018 5:48 pm

ejolson wrote:
Tue May 22, 2018 6:48 pm
Hmm. Maybe simple is more important than a clean compile. Better update the unit test.
I can't decide whether the following code is an example of simplicity, do the right thing, or an attempt at humor.

Code: Select all

/*  A possible example of readable and modifiable code to compute the
    average value of an array of floating point numbers.  Compile with
    the command

    $ gcc -O3 -std=gnu99 -Wall -Wconversion -o average average.c

    and run to produce the output

    $ ./average
    Error=  -4.571579250399471e-08

    To create a .o file for inclusion in a library compile with

    $ gcc -O3 -std=gnu99 -Wall -Wconversion -DNOMAIN -c average.c
*/

#include <stdio.h>
#include <stdlib.h>
#include <math.h>

/*  Computes the average value of an array of single-precision floating
    point numbers using a recursive formula that associates the numbers
    in the array from right to left while computing the average using 
    double precision arithmetic.  By definition arrays of length zero
    have zero average.

Input Parameters:
    count     -- the count of elements in the array.
    values[]  -- the values to be averaged.

Return Value:
    (1/count) * sum of values[i] for i from count-1 down to 0.

*/

double average(int count, float values[count]){
    if(count<=0) return 0;
    return (values[count-1]+(count-1)*average(count-1,values))/count;
}

#ifndef NOMAIN
int main(){
    int n=100;
    double exact=5.1873775176396202;
    float x[n];
    for(int i=0;i<n;i++) x[i]=(float)(n/(i+1.0));
    double e=average(n,x)-exact;
    printf("Error=%24.15e\n",e);
    if(fabs(e)<1e-7) return 0;
    printf("Failed!\n");
    return 1;
}
#endif
Maybe the code satisfies simplicity if the main routine and comments are omitted, but as written maybe not. Any thoughts?

Heater
Posts: 9963
Joined: Tue Jul 17, 2012 3:02 pm

Re: Making readable modifyable code.

Tue May 29, 2018 10:56 pm

Let's just say a thousand lines of that densely packed code in a module would make my eyes bleed. Can't we at least have some spaces and blank lines to make it easier on the eyes to scan?

Most of the comments are redundant and annoying clutter. They repeat what we see in the code. Especially if more meaningful variable and function names were used.

I'd rather see braces around conditionals and loop bodies. Even if they are only a single statement. Many a bug has been introduced by people modifying braceless conditionals and loops whilst forgetting that they now also need to add braces.

ejolson
Posts: 2006
Joined: Tue Mar 18, 2014 11:47 am

Re: Making readable modifyable code.

Wed May 30, 2018 5:33 am

Heater wrote:
Tue May 29, 2018 10:56 pm
Let's just say a thousand lines of that densely packed code in a module would make my eyes bleed. Can't we at least have some spaces and blank lines to make it easier on the eyes to scan?

Most of the comments are redundant and annoying clutter. They repeat what we see in the code. Especially if more meaningful variable and function names were used.

I'd rather see braces around conditionals and loop bodies. Even if they are only a single statement. Many a bug has been introduced by people modifying braceless conditionals and loops whilst forgetting that they now also need to add braces.
Maybe the code looks dense because your font is too small, or mine too big.

I removed the main routine and the comments to obtain

Code: Select all

double average(int count, float values[count]){
    if(count<=0) return 0;
    return (values[count-1]+(count-1)*average(count-1,values))/count;
}
Then I added braces (though I'm actually against them in this case) and ran the result through indent with the -kr flag to add some white space. The result is

Code: Select all

double average(int count, float values[count])
{
    if (count <= 0) {
        return 0;
    }
    return (values[count - 1] +
            (count - 1) * average(count - 1, values)) / count;
}
If there were any bugs before, there are now only half as many per line. Note that leaving out the -kr option would have reduced the number of bugs per line even further.

One could also double the number of bugs per line by writing

Code: Select all

double average(int c, float v[c]){
    return c<=0?0:(v[c-1]+(c-1)*average(c-1,v))/c;
}
which might or might not be easier to read (depending on your font and screen size).

The question remains whether this is an example of simplicity or an example of correctness and if neither then what needs to be changed to make it so.

jahboater
Posts: 3027
Joined: Wed Feb 04, 2015 6:38 pm

Re: Making readable modifyable code.

Wed May 30, 2018 6:49 am

Its looks like someone who has just discovered recursion trying it out!
Or perhaps someone who has used recursion for factorials say and thought it was applicable here.
Obviously its neither scalable, efficient, nor particularly readable. I had to read it carefully to make sure it wasn't doing something special. In general I think using a unusual algorithms or idioms for a simple and common problem makes for poor readability. Readers have to look twice and think why.
The standard code should be easier to understand:

Code: Select all

static double
average( const int count, const float values[count] )
{
  if( count <= 0 )
    return 0.0;
  double total = 0.0;
  for( int i = 0; i < count; ++i )
    total += values[i];
  return total / count;
}
adding "const"s advertises to the user that the function doesn't alter the values array - and makes it safer and faster.

Obviously the the count <= 0 test avoids a divide by zero or worse, but it also tells the compiler the range of values "i" can take in the loop - so it will change the readable "for" loop into a fast "do while".

I prefer to give float constants as 0.0 instead of just 0
Last edited by jahboater on Wed May 30, 2018 7:27 am, edited 3 times in total.

User avatar
PeterO
Posts: 4257
Joined: Sun Jul 22, 2012 4:14 pm

Re: Making readable modifyable code.

Wed May 30, 2018 6:59 am

jahboater wrote:
Wed May 30, 2018 6:49 am

Code: Select all

static double
average( const int count, const float values[count] )
{
  if( count <= 0 )
    return 0.0;
  double total = 0.0;    // <--------------------------
  for( int i = 0; i < count; ++i )
    total += values[i];
  return total / count;
}
I don't like the arrowed line. Whilst sprinkling declarations through the body of a function (or block) may make it easier to read in a linear fashion, it makes it harder to read in a random fashion which is what I find I do the most when debugging or modifying code.

Putting them all at the start of the function/block means you know where to look when you need to find them.

PeterO
Discoverer of the PI2 XENON DEATH FLASH!
Interests: C,Python,PIC,Electronics,Ham Radio (G0DZB),Aeromodelling,1960s British Computers.
"The primary requirement (as we've always seen in your examples) is that the code is readable. " Dougie Lawson

jahboater
Posts: 3027
Joined: Wed Feb 04, 2015 6:38 pm

Re: Making readable modifyable code.

Wed May 30, 2018 7:11 am

OK yes good point about being easy to find.

I like to put the pre-condition checks right at the start and get them all over with before starting the main logic.

Also if they fail, no time is wasted setting total (though the compiler would deal with that sort of thing).

User avatar
PeterO
Posts: 4257
Joined: Sun Jul 22, 2012 4:14 pm

Re: Making readable modifyable code.

Wed May 30, 2018 7:29 am

jahboater wrote:
Wed May 30, 2018 7:11 am
OK yes good point about being easy to find.

I like to put the pre-condition checks right at the start and get them all over with before starting the main logic.

Also if they fail, no time is wasted setting total (though the compiler would deal with that sort of thing).
I take your point. So your structure is like this ?

Code: Select all

Function
{
    Pre checks with get out quick return statements.

   Variable declarations.

   Body.
}
PeterO
Discoverer of the PI2 XENON DEATH FLASH!
Interests: C,Python,PIC,Electronics,Ham Radio (G0DZB),Aeromodelling,1960s British Computers.
"The primary requirement (as we've always seen in your examples) is that the code is readable. " Dougie Lawson

jahboater
Posts: 3027
Joined: Wed Feb 04, 2015 6:38 pm

Re: Making readable modifyable code.

Wed May 30, 2018 7:41 am

Only in that the checks are all at the start so they don't interfere with or confuse the body.
Though in this case the check is also really part of the algorithm, so its all moot!

Given the choice, I place the declarations near where the item is used - which also helps with your point about them being easy to find (in a different way!!).

Further, "total = 0.0" is a crucial part of an averaging algorithm, so I felt it should be immediately prior to the summation loop.

Other than that, I try to minimize visibility wherever possible.

Not so long ago i had to work with coding standards that said a function may only return at the end.
That meant the entire body of the code was nested by the argument checks ...............

Heater
Posts: 9963
Joined: Tue Jul 17, 2012 3:02 pm

Re: Making readable modifyable code.

Wed May 30, 2018 9:07 am

ejolson,

No, there is nothing wrong with my font size. Theexamplelooksdenseandhardtoreadbecauseithasnospacesinit.

Why would one be against using braces for conditionals or loops?

a) Doing so is consistent with other conditionals and loops. Why do something weird in this case?
b) Doing so clearly signals to the reader the intention.
c) Doing so makes it easier for a maintainer to add another line to the conditional or loop body if the need arises one day.
d) Not doing so has been the source of many bugs in the past.
The question remains whether this is an example of simplicity or an example of correctness and if neither then what needs to be changed to make it so.
As for simplicity. No. I'm pretty sure a recursive algorithm to compute an average is less easy to comprehend than the traditionally expected loop. It also uses up a ton of processing time and stack space unnecessarily which may cause weird problems for the rest of your program. Slow execution, out of stack space errors.

As for correctness. If I were given that code to review I would not even start to think about correctness until the author had run it through clang-tidy or some such to make it legible.

The example as originally given is not correct it does not work. A correct, and much more easily readable (as far as a recursive average can be) version is:

Code: Select all

//
// Compute the average of "length" number of values using a recursive algorithm
//
// For use in a program compile with:
//
//     $ clang -o averageRecursive.o averageRecursive.c
//
// To self test compile as:
//
//    $ clang -DTEST -o averageRecursive averageRecursive.c
//

#include <math.h>
#include <stdio.h>
#include <stdlib.h>

// Compute the average of "length" number of values using a recursive algorithm
double averageRecursive(int length, const float values[length])
{
    // Can't have an average of zero or negative number of values
    if (length == 0) {
        return 0;
    }
    // The base case
    if (length == 1) {
        return (values[0]);
    }
    // Recurse...
    float result = ((averageRecursive(length - 1, &values[1]) * (length - 1)) + values[0]) / length;
    return (result);
}

#ifdef TEST
int main()
{
    int length = 100;
    float testData[length];
    double expected = 50.5;
    double result;
    float error

    // Fill test data array
    for (int i = 0; i < length; i++) {
        testData[i] = (float)(i + 1);
    }

    // Test the recursive average function
    result = averageRecursive(length, testData);
    
    // Report results
    printf("Result = %24.15e, Expected = %24.15e\n", result, expected);
    error = expected - result;
    if (fabs(error) < 1e-7) {
        printf("Passed\n");
        return 0;
    } else {
        printf("Failed\n");
        return 1;
    }
}
#endif
[code]
Last edited by Heater on Wed May 30, 2018 9:19 am, edited 2 times in total.

User avatar
PeterO
Posts: 4257
Joined: Sun Jul 22, 2012 4:14 pm

Re: Making readable modifyable code.

Wed May 30, 2018 9:09 am

jahboater wrote:
Wed May 30, 2018 7:41 am
Not so long ago i had to work with coding standards that said a function may only return at the end.
That meant the entire body of the code was nested by the argument checks ...............
Oh I'm soooooo pleased that I don't have to follow those things any more.
Having a "single return" is so bone-headed ! A colleague of mine flatly refused to follow that rule because it assumes a code structure that in many cases is inappropriate.

Taking your point about "limiting visability", that's why I wrote "function (or block)". I some times add a block just for the purpose of limiting variable scope, but all the variable declarations go at the start of the block.

But in the long run anything that makes the code understandable and maintainable by other programmers is "good stuff" 8-)

PeterO
Discoverer of the PI2 XENON DEATH FLASH!
Interests: C,Python,PIC,Electronics,Ham Radio (G0DZB),Aeromodelling,1960s British Computers.
"The primary requirement (as we've always seen in your examples) is that the code is readable. " Dougie Lawson

jahboater
Posts: 3027
Joined: Wed Feb 04, 2015 6:38 pm

Re: Making readable modifyable code.

Wed May 30, 2018 9:21 am

I get

Code: Select all

ave original  returns 0x1.4bfdfe147ae13p+2
ave loop      returns 0x1.4bfdfe147ae14p+2
ave recursive returns 0x1.4bfdfep+2
I'll have to think about why the new recursive code is slightly different.
They are close enough when printed normally with rounding:

Code: Select all

ave original  returns 5.18738
ave loop      returns 5.18738
ave recursive returns 5.18738

Heater
Posts: 9963
Joined: Tue Jul 17, 2012 3:02 pm

Re: Making readable modifyable code.

Wed May 30, 2018 9:45 am

That thing about having to have "return" at the end of a function or not annoys me.

I think it comes about because having a "return" in a high level language is pretty silly to start with. It's just gragging the low level, hardware or assembler, idea of RET up into the high level.

If we really want the rule that a function can only have one return, perhaps at the end, then the "return" is redundant and should not be in the language.

A better way would be to have a "result" keyword. Which more clearly states that I am creating the result of the function. My recursive average would then look like this:

Code: Select all

double averageRecursive(int length, const float values[length])
{
    if (length == 0) {
        // Can't have an average of zero or negative number of values
        result = 0;
    } else if (length == 1) {
        // The base case
        result = (values[0]);
    } else {
        // Recurse...
        result = ((averageRecursive(length - 1, &values[1]) * (length - 1)) + values[0]) / length;
}
Arguably that "result" is redundant so we could have just this:

Code: Select all

double averageRecursive(int length, const float values[length])
{
    if (length == 0) {
        // Can't have an average of zero or negative number of values
        0;
    } else if (length == 1) {
        // The base case
        values[0];
    } else {
        // Recurse...
        ((averageRecursive(length - 1, &values[1]) * (length - 1)) + values[0]) / length;
    }
}
That still looks a bit icky. I'd rather have "when"and "otherwise" like so:

Code: Select all

double averageRecursive(int length, const float values[length])
{
    when length == 0 {
        // Can't have an average of zero or negative number of values
        0;
    }
    when length == 1 {
        // The base case
        values[0];
    }
    otherwise {
        // Recurse...
        ((averageRecursive(length - 1, &values[1]) * (length - 1)) + values[0]) / length;
    }
}
There, much better.

Heater
Posts: 9963
Joined: Tue Jul 17, 2012 3:02 pm

Re: Making readable modifyable code.

Wed May 30, 2018 9:47 am

jahboater,
I'll have to think about why the new recursive code is slightly different.
I think that says all we need to know about the redability, simplicity and correctness of the original.

:)

User avatar
PeterO
Posts: 4257
Joined: Sun Jul 22, 2012 4:14 pm

Re: Making readable modifyable code.

Wed May 30, 2018 9:57 am

Heater wrote:
Wed May 30, 2018 9:45 am
A better way would be to have a "result" keyword. Which more clearly states that I am creating the result of the function.
In Algol-60 there is no way to exit a procedure other than by execution reaching the closing "end" and the return value from a procedure was assigned to a pseudo-variable with the same name as the procedure:

Now I'm going to have to write this recursive average example in ALGOL-60 :lol:

PeterO
Discoverer of the PI2 XENON DEATH FLASH!
Interests: C,Python,PIC,Electronics,Ham Radio (G0DZB),Aeromodelling,1960s British Computers.
"The primary requirement (as we've always seen in your examples) is that the code is readable. " Dougie Lawson

Heater
Posts: 9963
Joined: Tue Jul 17, 2012 3:02 pm

Re: Making readable modifyable code.

Wed May 30, 2018 10:10 am

It's possible that I had Algol haunting the back of my mind when I said "...having a 'return' in a high level language is pretty silly to start with".

Algol 68 was my first love. On an ICL 2960.

For a long while after that I wrote pseudo code that looked a lot like Algol.

Return to “General programming discussion”