Improving readability

Question

I need to find minimal date(year , month , day, hours , minutes , seconds), my code is working, but it look's terrible and it's very long. What can I do to avoid this ladder to make my code readable? ( I want to use only stdio.h )

#include <stdio.h>
typedef struct DateTime_s {
    int year , month , day ;
    int hours , minutes , seconds ;
} DateTime ;
void DataTime(const DateTime *mas , int x){
    int i;
    struct DateTime_s min={40000,400000,4000000,400000,400000,4000};
    for(i=0;i<x;i++){
        if(mas[i].year<min.year){
            min=mas[i];
        }
        else if(mas[i].year==min.year){
            if(mas[i].month<min.month){
                min=mas[i];
            }
            else if(mas[i].month==min.month){
                if(mas[i].day<min.day){
                    min=mas[i];
                }
                else if(mas[i].day==min.day){
                    if(mas[i].hours<min.hours){
                        min=mas[i];
                    }
                    else if(mas[i].hours==min.hours){
                        if(mas[i].minutes<min.minutes){
                            min=mas[i];
                        }
                        else if(mas[i].minutes==min.minutes){
                            if(mas[i].seconds<min.seconds){
                                min=mas[i];
                            }
                            else if(mas[i].seconds==min.seconds){
                                min=mas[i];
                            }
                        }
                    }
                }
            }
        }
    }
    printf("%d %d %d %d %d %d",min.year,min.month,min.day,min.hours,min.minutes,min.seconds);
}

int main() {
    int x,i;
    struct DateTime_s mas[50001];
    scanf("%d",&x);
    for(i=0;i<x;i++){
        struct DateTime_s b;
        scanf("%d %d %d %d %d %d",&b.year, &b.month,&b.day,&b.hours,&b.minutes,&b.seconds);
        mas[i]=b;
    }
    DataTime(mas,x);
    return 0;
}

If you code works fine, and you're looking for code review comments, head to [codereview.se] — Sourav Ghosh, Dec 21 '21 at 14:42
It might make more sense to convert the timestamp to an epoch time. It would cost extra, but it would reduce the comparison chain to a single check. But if you're going to do that anyway... — ikegami, Dec 21 '21 at 14:45
The code would be a whole lot easier to read if you used consistent indention. It isn't custom to indent every `else if`. — Lundin, Dec 21 '21 at 14:52
I’m voting to close this question because it was ["migrated"](https://codereview.stackexchange.com/q/272214/24930) to Code Review, where it belongs. — ikegami, Dec 21 '21 at 15:09
Amplifying on Lundin's comment: the first thing to do is to use the *same* indent level for each of the N branches of the long if/else chain. Even though this is arguably "inconsistent", just about everybody agrees it's much, much better than having it march off inexorably towards the right. — Steve Summit, Dec 21 '21 at 15:21
@SouravGhosh, the question needs work before it's suited to [codereview.se]. You should have pointed the asker at [A guide to Code Review for Stack Overflow users](//codereview.meta.stackexchange.com/a/5778), as some things are done differently over there - e.g. we need a good description of the *purpose* of the code to give context, and question titles should simply say what the code *does* (the question is always, "_How can I improve this?_"). It's important that the code works correctly; include the unit tests if possible. — Toby Speight, Dec 21 '21 at 16:57

Ted Lyngmo · Answer 1 · 2021-12-23T00:56:50.427

1

I suggest splitting it up in two functions. One that checks if one DateTime is less than the other DateTime and the function that loops.

It also helps readability to remove the massive amount of nested ifs.

Here's a function that can be used with sort and bsearch if you need to do sorting and searching too:

int compar(const void *Lhs, const void *Rhs) {
    const DateTime* lhs = Lhs;
    const DateTime* rhs = Rhs;
    if(lhs->year < rhs->year) return -1;
    if(lhs->year > rhs->year) return 1;
    if(lhs->month < rhs->month) return -1;
    if(lhs->month > rhs->month) return 1;
    if(lhs->day < rhs->day) return -1;
    if(lhs->day > rhs->day) return 1;
    if(lhs->hours < rhs->hours) return -1;
    if(lhs->hours > rhs->hours) return 1;
    if(lhs->minutes < rhs->minutes) return -1;
    if(lhs->minutes > rhs->minutes) return 1;
    if(lhs->seconds < rhs->seconds) return -1;
    if(lhs->seconds > rhs->seconds) return 1;
    return 0;
}

void DataTime(const DateTime mas[], int x) {
    struct DateTime_s min = mas[0];

    for(int i = 1; i < x; i++) {
        if(compar(&mas[i], &min) < 0) min = mas[i];
    }
    printf("%d %d %d %d %d %d", min.year, min.month, min.day, min.hours,
           min.minutes, min.seconds);
}

edited Dec 23 '21 at 00:56

answered Dec 21 '21 at 14:53

Ted Lyngmo

93,841
5
60
108

Minor remark: If you could tweak `less` to use return type `int` and two `const void*` you might be able to use it with `bsearch` instead of a loop. Probably only makes sense if there's a whole lot of sorted data though. – Lundin Dec 21 '21 at 15:07
@TedLyngmo I was about to comment as Lundin did; but only on the return type of int. Also, I think it might be a little nicer to replace the `!=` with `>` to reflect the "not less" conditions you exit early upon. – Edwin Buck Dec 21 '21 at 15:17
@EdwinBuck The `const void*` parameters would be necessary though. Unfortunately, since that lowers type safety. – Lundin Dec 21 '21 at 15:19
@Lundin I couldn't stay away from that idea so I made it into something that could be used with `sort` and `bsearch`. – Ted Lyngmo Dec 21 '21 at 15:20
@Lundin Yes, they would be necessary; but, my comments were not considering using `bsearch` but expanding the `less` to a `compare` functionality that was tied to `DateTime`s directly. If you don't care about making it compatible, you can have the stronger type binding. – Edwin Buck Dec 21 '21 at 15:20
@EdwinBuck Agree on the `>`. Fixed. – Ted Lyngmo Dec 21 '21 at 15:22

ikegami · Answer 2 · 2021-12-21T15:27:05.437

One of the most effective tools at improving readability is functions!

DateTime *min_dt(const DateTime *a, const DateTime *b) {
    if ( a->year    != b->year    ) return (DateTime*)( a->year    < b->year    ? a : b );
    if ( a->month   != b->month   ) return (DateTime*)( a->month   < b->month   ? a : b );
    if ( a->day     != b->day     ) return (DateTime*)( a->day     < b->day     ? a : b );
    if ( a->hours   != b->hours   ) return (DateTime*)( a->hours   < b->hours   ? a : b );
    if ( a->minutes != b->minutes ) return (DateTime*)( a->minutes < b->minutes ? a : b );
    if ( a->seconds != b->seconds ) return (DateTime*)( a->seconds < b->seconds ? a : b );
    return a;
}

void DataTime(const DateTime *mas, size_t n) {
    if (n == 0)
        return;  // Or whatever

    const DateTime *min = &mas[0];

    for (size_t i=1; i<n; ++i)
        min = min_dt(min, &mas[i]);

    printf("%d %d %d %d %d %d",
        min->year, min->month, min->day,
        min->hours, min->minutes, min->seconds
    );
}

Unfortunately, the const removal makes the line a bit long. But it's required for the function to work with both pointers to constants and pointers to non-constants. The issue could be dodged by returning a boolean.

For the last check, do you even need to check `if ( a->seconds != b->seconds )`? If it's passed through the `minute` filter you could just `return (DateTime*)( a->seconds < b->seconds ? a : b );` if I'm not totally off. — Ted Lyngmo, Dec 21 '21 at 15:12
@Ted Lyngmo, I know. I went for uniformity :) I'm hoping the optimizer figures that one out. — ikegami, Dec 21 '21 at 15:13

score 0 · Answer 3 · answered Dec 21 '21 at 14:49

It is possible to use continue to reduce the ladder effect. But whether it improves readability is a different question...:

void DataTime(const DateTime* mas, int x) {
    int i;
    struct DateTime_s min = { 40000,400000,4000000,400000,400000,4000 };
    for (i = 0; i < x; i++) {
        if (mas[i].year < min.year) {
            min = mas[i];
            continue;
        }
        else if (mas[i].year > min.year) continue;
        if (mas[i].month < min.month) {
            min = mas[i];
            continue;
        }
        else if (mas[i].month > min.month) continue;
        if (mas[i].day < min.day) {
            min = mas[i];
            continue;
        }
        else if (mas[i].day == min.day) continue;
        if (mas[i].hours < min.hours) {
            min = mas[i];
            continue;
        }
        else if (mas[i].hours > min.hours) continue;
        if (mas[i].minutes < min.minutes) {
            min = mas[i];
            continue;
        }
        else if (mas[i].minutes > min.minutes) continue;
        if (mas[i].seconds <= min.seconds) {
            min = mas[i];
        }
    }
    printf("%d %d %d %d %d %d", min.year, min.month, min.day, min.hours, min.minutes, min.seconds);
}

oof. Better to move the comparison to a function. Then `min = mas[i]; continue;` becomes `return a;` — ikegami, Dec 21 '21 at 14:50

score 0 · Answer 4 · answered Dec 21 '21 at 15:15

/**
 * Compares to DateTimes.
 *
 * @return -1 when the first is earlier
 *          0 when they are equal
 *          1 when the seconde is earlier
 */
int DateTime_compare(const DateTime* lhs, const DateTime* rhs) {
    if (lhs->year < rhs->year) return -1;
    if (lhs->year > rhs->year) return 1;
    if (lhs->month < rhs->month) return -1;
    if (lhs->month > rhs->month) return 1;
    if (lhs->day < rhs->day) return -1;
    if (lhs->day > rhs->day) return 1;
    if (lhs->hours < rhs->hours) return -1;
    if (lhs->hours > rhs->hours) return 1;
    if (lhs->minutes < rhs->minutes) return -1;
    if (lhs->minutes > rhs->minutes) return 1;
    if (lhs->seconds < rhs->seconds) return -1;
    if (lhs->seconde > rhs->seconds) return 1;
    return 0;
}

/** Find the minimum */
void DataTime(const DateTime mas[], int x) {
    struct DateTime_s min = {40000, 400000, 4000000, 400000, 400000, 4000};

    for (int i = 0; i < x; i++) { 
        min = (DateTime_compare( &mas[i], &min ) < 0) ? mas[i] : min;
    }
    printf("%d %d %d %d %d %d", min.year, min.month, min.day, min.hours,
           min.minutes, min.seconds);
}

score 0 · Answer 5 · answered Dec 21 '21 at 16:37

You can convert to floating type:

double min_fltime = 99999.9*9999999, fltime;
int i, xmin;

for(i=0;i<x;i++){
    fltime = mas[i].seconds
           + mas[i].minutes *                60
           + mas[i].hours *                3600
           + mas[i].day *               24*3600
           + mas[i].month *      365.25*24*3600
           + mas[i].year *   12* 365.25*24*3600;

    if (fltime < min_fltime) {
        min_fltime = fltime;
        xmin = i;

    }
    #define M mas[i]
    printf("%d %d %d %d %d %d %f\n",M.year,M.month,M.day,M.hours,M.minutes,M.seconds, fltime);
}
#define MM mas[xmin]
printf("min\n%d %d %d %d %d %d  %f", MM.year,MM.month,MM.day,MM.hours,MM.minutes,MM.seconds, min_fltime);

The fltime assignment should be a function and the first date taken as minimum to start with.

Output:

1999 12 31 12 45 7 757385124307.000000
999 6 17 10 59 59 378503362799.000000
999 6 18 10 59 59 378503449199.000000
999 6 17 10 59 58 378503362798.000000
min
999 6 17 10 59 58  378503362798.000000

For valid dates in this region it seems to work.

Improving readability

5 Answers5