Is this "readable"

[Dren]

Lately I've been concerned about code legibility.
I am striving to write the most readable code I can, yet somehow end up with unaesthetic blocks of words that at first glance, are headache inducing.

I'm not claiming that visually pleasing code is easier to understand. I've written "pretty" code that is far from clear. case and point:

static inline int is_square_valid(char character, unsigned x, unsigned y)
{
  static unsigned int newlines = 0;
  static unsigned int blocks = 0;
  int ret_val;

  blocks = (blocks + (character == '#')) & (~(x || y) + 1);
  newlines = (newlines + 1) & (~(character == '\n') + 1);
  x += !newlines;
  y += ! !newlines;
  ret_val = (((newlines == BREAKS && y == COLUMNS) ||
              ((x % (ROWS + 1) && (!newlines || x == ROWS)) &&
               (y < COLUMNS) && (blocks <= TETRAMINO_BLOCKS))) &&
             (character == '.' || character == '#' || character == '\n'));
  return (ret_val);
}

But lately more and more of what I'm writing looks like an ill formatted English essay than a technical sequence of instructions.
Today I was reviewing a utility function of my printf clone; the thing is as aesthetically ugly as I managed to make it. I was looking at what I though were descriptive identifiers and realized they are not as descriptive as I thought. Plus they are very long, which is an issue because my lines are to be shorter than 80 characters -school rule- and I have to constantly split them which adds to the bulky text appearance and increases my function line count -another limit on that as well.
When I found myself creating two macros to clarify the meaning of a short, third one, that I had to use in order not to make a line go over the char limit, I thought to myself "what the heck I'm doing".

I'm I thinking too much over this? Are meaningful identifiers still meaningful when they're longer than an 18 wheeler? As experienced programmers, do you think this is "readable":

#define DOT 1
#define FIRST_DIGIT 1
#define EXTRAS DOT + FIRST_DIGIT
extern void adjust_decimal_point(char *float_str, unsigned int precision)
{
    char *str_cpy;
    char *strcpy_start;
    char *floatstr_start;

    str_cpy = ft_strnew(ft_strlen(float_str));
    strcpy_start = str_cpy;
    floatstr_start = float_str;
    if (*float_str == '-')
        *str_cpy++ = *float_str++;
    while (*float_str == '0' || *float_str == '.')
        ++float_str;
    *str_cpy++ = *float_str++;
    *str_cpy++ = '.';
    while (*float_str && precision--)
    {
        float_str += (*float_str == '.');
        *str_cpy++ = *float_str++;
    }
    str_cpy = strcpy_start;
    round_up(str_cpy, ft_strlen(str_cpy) - (EXTRAS  + (*str_cpy == '-')));
    while (*str_cpy)
        *floatstr_start++ = *str_cpy++;
    *floatstr_start = 0;
    free(strcpy_start);
}

[Salem]

Regarding adjust_decimal_point
1. 'adjust' how?

2. Mixing 'str' as a prefix or suffix is terrible. Using it at all is bad. You use it on everything, and in doing so it becomes meaningless.



Some bugs (perhaps)

    str_cpy = ft_strnew(ft_strlen(float_str));
    strcpy_start = str_cpy;
///
    free(strcpy_start);

Does ft_strlen() also count the \0 ?
Does ft_strnew() make allowance for a \0 if ft_strlen() returns just the number of characters?
Does ft_strnew() automatically fill the buffer with \0? You later call ft_strlen() again with no obvious sign that a \0 was copied.
How does EXTRAS figure in this? Does anything you're doing make the string any longer than it already is?

You really should free the same pointer (by name) that you used to store the result of the alloc. People will search for the pointer by name in an attempt to find if you free it, then wonder WTF when they see another name and then try to figure out whether it's right or not.

char *const ptr = malloc(...);
This would give you some assurance at least that you had a pointer to the memory you allocated that wasn't subsequently messed about with through increment or assignment.

IMO, you might be better off using a separate incrementing index, as ptr[index++] rather than *ptr++. Modern compilers are not going to significantly change the amount of generated code either way, so pick something for maximum readability.

As far as readability, perhaps some more functions.

    // make a function called handleNegative()
    if (*float_str == '-')
        *str_cpy++ = *float_str++;

    // make a function skipLeadingZerosOrDecimalPoint
    while (*float_str == '0' || *float_str == '.')
        ++float_str;

    // make a function called appendDecimalPoint()
    *str_cpy++ = *float_str++;
    *str_cpy++ = '.';

    // make a function called copyRemainingPrecision()
    while (*float_str && precision--)
    {
        float_str += (*float_str == '.');
        *str_cpy++ = *float_str++;
    }

Again, if you make said functions short and static, there is every chance that the compiler will just inline them for you. Even if they don't, function call overheads are not what they once were.


How does this differ from regular strcpy()?

    while (*str_cpy)
        *floatstr_start++ = *str_cpy++;
    *floatstr_start = 0;

[Dren]

Dear Salem, thank you for your reply


1. I confess I did not provide any comments or context intentionally, to see if the code alone was clear enough to convey its meaning. Obviously, it wasn’t.

This function is part of my printf implementation. Like ft_strlen and ft_strnew, it is supposed to mimic the functionality of its standard library counterpart, to the letter. I am free to implement my own improvements if I want, of course; as long as I don’t violate the previous rule.

This particular example is a subset of the %eE format specifiers. It aims to “adjust” the decimal point from wherever it might be, to the position right after the first number in the string. In order to conform with the required [-]d.ddde+-dd format.

2. I actually learned about using str[a-z] as an identifier being bad from glancing at the first few pages of C Unleashed – thanks for that, my first thoughts after 60 seconds of casually glancing at its pages, and specifically the bit about those identifiers not being guaranteed to work by the standard, were: this book is worth its weight in gold, and: how come they don’t teach this at schools!?

I wrote this code long before reading about that. I honestly aim to fix this an other “mistakes” throughout the code, not just here, but at this point I want to finish the project first, I’ve spent too much time on it already. Much more than I’d like to admit. I absolutely agree about the code losing meaning though, I just couldn’t pinpoint the cause.

3. ft_strlen is my implementation of strlen and it behaves just like the original, it does not count the \0. ft_strnew calls my ft_memalloc which does the same as malloc but also zeroes the memory. In addition ft_strnew(size) allocates size + 1 byte and null terminates it, so ft_strnew(0) would return a valid empty string.

The whole point of Extras has to do with the round_up function. Which rounds the last significant decimal, like the standard printf does with %e. I never liked calling it here, but this function truncates the decimal points after its specified precision, e.g 1410046.123 becomes 1.41004 -I add the suffix eE-+XX elsewhere. If I didn’t call round up here before transforming the number, I couldn’t tell that 1.41004 should be rounded up to 1.41005 because I lost that last 6.

The round function counts from the start of the string up to the the nth digit we pass it, and uses that as the rounding digit. In this case I want the last digit copied as the rounding digit which will be used to determine whether or not the preceding digit should be increased. So I pass strlen(number), However, round_up will not count the negative sign if the number is negative, it will ignore the first digit and it will ignore the decimal point, hence strlen(number) – EXTRAS, EXTRAS being the decimal point, 1, the first digit, 1, and if the number is negative, another 1.
In retrospect I realize DOT looks more like a weird abbreviation than the indication of the decimal point.


4. I usually set the original pointer back to its initial address before freeing it, instead of freeing the pointer holding the original address, but my line count was too high already in this case. I will do as you recommend and use const from now on regardless.

Believe it or not, I try to use smaller static, inline sub-functions whenever I can. But we are only allowed to have a max of 5 functions per source file, so I can either try and cram everything I can in a function or have a myriad of meaningless source files. For example, conversions/eE_conversion/ eE_conversion.c eEconv_util.c eEconv_util1.c eEconv_util2.c
It might be better than having unreadable code so I will go with it :shrug:

How does this differ from regular strcpy()?

It doesn’t I guess. I can’t use any library functions other than malloc, free and write though -and for this project the ones from stdarg.h as well - so there’s that. I guess it’s time to expand my library with a ft_strcpy?


Thanks for the feedback Salem. XoXo

[Dren]

When using const on the pointer I get two warnings, the first one about losing the const qualifier when assigning back to the original pointer. The second one is about freeing a const pointer.
Is it OK to cast the const away? I read somewhere something in the lines of one should never cast the const away, as that defeats the purpose. What is your opinion on this?
I obviously have to do it now to get my project to compile because of the mandatory -Werror, but as a general rule, is this something that is frowned upon? If so why?