Reviewing your Code: Refactoring

---

.



***

Welcome! I post videos that help you learn to program and become a more confident software developer. I cover beginner-to-advanced systems topics ranging from network programming, threads, processes, operating systems, embedded systems and others. My goal is to help you get under-the-hood and better understand how computers work and how you can use them to become stronger students and more capable professional developers.

About me: I'm a computer scientist, electrical engineer, researcher, and teacher. I specialize in embedded systems, mobile computing, sensor networks, and the Internet of Things. I teach systems and networking courses at Clemson University, where I also lead the PERSIST research lab.

More about me and what I do:

To Support the Channel:
+ like, subscribe, spread the word

Want me to review your code?

You can also find more info about code reviews here.

[MH-rifb]

Great review! I love the error checking macros in particular, I hadn’t thought of that before! Smaller code footprint makes me happy.

I think location of declared variables will always be up to preference. Personally I declare variables just before where they will be used, and (where appropriate, eg temporary storage in a loop) in the most limited scope possible. Nothing gets me more confused in code than hopping between the body of the function and variable declarations at the top to see the type of some system call related struct for example. Can’t always use this philosophy with legacy code/compilers, but I’m fortunate enough to be working with C99 :)

[liviuq]

That macro is so useful. Thanks for the videos!

[ropersonline]

11:36: To me that looked like you were testing it twice with register. Maybe I'm mistaken though.

[rustycherkas]

When you removed the static keyword(s), the values of those local variables became undefined. (Static vars will be initialised to 0)... Fortunately, this code didn't rely on the explicit 'static' nature of those vars...

MAX_PATH is MAX_PATH... When descending into a child directory, it would be better (imo) to tack the child's name (and '/' of course) onto the end of the current path buffer, then simply truncate (restore the '\0') when that recursive call returns. No need for malloc (that can fail) or forgetting to free()... (Don't overflow that single string buffer, of course!)

Oh! And!! Move the workhorse function ahead of its invocation and lose the maintenance overhead of changing both the function definition and its prototype declaration when parameters need changing... Fewer lines of code = less chance for errors to creep in...

[kar_prz]

Your videos are very informative and well-structured, keep up the good work

[Pawlsolidus]

I’d like a version of this code using functions instead of comments

[hsaidinsan]

More videos about refactoring and code reviews in this code smell series

[elmalleable]

drum roll for the outro, side by side of the two source code samples, bummer, the editor signed out.
super nice either ways

[mnfchen]

Great video! I'm not sure if having lsa3 handle only one argument is the only way to make it better/more maintainable. You could, for instance, validate the args ahead of time, checking if they exist, inserting a default "." if no arg exists, etc. And then passing the validated args to the main function

[lorenzop]

11:45 your change to the line that checks if the dir name starts with . you changed it to check explicitly for . or .. but this does actually change how the program functions. the original code would ignore hidden directories starting with a dot . but your change makes it so the code will also traverse hidden directories.

[csbnikhil]

9:06 Having the if statement above the variables will avoid unnecessary declarations.

[skeleton_craftGaming]

changing code while refactoring is called optimizing (or trying to optimize at least.)

[anupkodlekere]

would be great if you could make one of these for tests in C!

[Tktproductions]

question, what is the best way to organize your code, especially libraries, where they make intuitive sense? grouping by function/functional area?

[mehmetdemir-lfvm]

should you use "ruby" for testing? couldn't a bash script do that? do we need to learn lots of languages when common languages can do the required work?

[taragnor]

Most of the choices made here were good and intuitive, but I'm not a big fan of removing the no arguments version. Given that the program is called lsa, I'd want to keep the behavior as close to ls as possible. Since you can invoke ls without arguments, it's probably going to cause problems when you have a similarly named offshoot tool that works differently.

[jimmyhart]

Just curious...is there any advantage to using the macros over the stdlib assert? Other than the custom message?

[mihirluthra]

hey jacob, thnks for the awesome videos firstly.
Also, can you create a video about atomic operations in C. Like a lot of confusing types like _Atomic and strange functions like etc. Also if possible do you know any atomic double compare and swap ways?

[GA]

What you do at 12:00 is more simple? This is relative to whom... Author used a "basic aproach", but you used "standard C library" functions. Your approach only suits (A) experienced programmers who are already familiar with the "standard C library", but (B) inexperienced programmers aren't. But group A would understand the code anyway while you made it even worse to group B, because you are forcing them to spend even more time reading and learning the "standard C library manual" which is good in the long term, but in terms of readability of the code as it was the first approach was in general more readable.

[itsmaxim]

replacing the str[0] == ‘.’ with strcmp’s is rarely a good idea imo. The compiler will not be able to optimise that and therefore will make the program run slower, even if just marginally. An inline comment would be more than enough.