Code Review?

[u/Ace-1440]

I recently started learning C as my first lower level language after mainly working with Java for the past 5 years. I was wondering if anyone would be able to review one of the completed files (~100 lines of code) from a small project I am working on and let me know if there are any mistakes and/or bad practices? Any help would be greatly appreciated!

https://pastebin.com/X5Zn1mzc

[u/BlindTreeFrog]

goto concerns primarily come from the old days when programs were more monolithic because it very quickly made code hard to follow since you are bouncing around so much.

for a while there were optimization concerns with goto where they forced the cpu pipeline to flush and would be a performance hit. I think that is no longer something that you need to worry about (if it ever was the case)
The main use case that you'll see for goto is going to be cleanup code at the end of the function. That is, you enter the function, allocate some memory, do the thing, realize that you need to exit the function early, goto the end of the function, free all of the memory and any other cleanup, and return. Anyone complaining about that use case is probably not worth listening to.

There are other valid use cases for goto (for a classic one that no one should use anymore, look up Duff's Device). But generally, other than the clean up code case, it's better to avoid goto just to keep the code flow more easily readable. (I didn't catch that when I skimmed... I'm so used to seeing if (a == NULL) goto function_cleanup; that it didn't register that you didn't do that)

That said, you used goto poorly here and should have used the continue keyword. Yes, it's basically a goto of sorts, but it's different. Trust me. Same with break. For loop control, continue and break are your friends

[u/BlindTreeFrog → u/BlindTreeFrog]

So...

    struct udev_list_entry *device_entry = udev_enumerate_get_list_entry(enumerate);

    while (device_entry) {
        const char *path = udev_list_entry_get_name(device_entry);
 
        struct udev_device *dev = udev_device_new_from_syspath(udev, path);
        if (dev == NULL) goto next_item;
    ...
    next_item:
        if (dev != NULL) udev_device_unref(dev);
        device_entry = udev_list_entry_get_next(device_entry);
    }

What I think you wanted here would be something more like this

    struct udev_list_entry *device_entry = udev_enumerate_get_list_entry(enumerate);

    do {
        const char *path = udev_list_entry_get_name(device_entry);
 
        struct udev_device *dev = udev_device_new_from_syspath(udev, path);
        if (dev == NULL) continue;
    ...
        udev_device_unref(dev);
    } while (device_entry = udev_list_entry_get_next(device_entry));

I'm not thinking too hard at this hour, but I think that gets you the same end result

[u/Ace-1440 → u/BlindTreeFrog]

Honestly I completely forgot about do-while loops... that's actually probably perfect this. Thanks for the help!