Reviewing your Code: Refactoring
Vložit
- čas přidán 17. 04. 2019
- Patreon ➤ / jacobsorber
Courses ➤ jacobsorber.thinkific.com
Website ➤ www.jacobsorber.com
---
.
***
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:
www.jacobsorber.com
people.cs.clemson.edu/~jsorber/
persist.cs.clemson.edu/
To Support the Channel:
+ like, subscribe, spread the word
+ contribute via Patreon --- [ / jacobsorber ]
+ rep the channel with nerdy merch --- [teespring.com/stores/jacob-so...]
Source code is also available to Patreon supporters. --- [jsorber-youtube-source.heroku...]
Want me to review your code?
Email the code to js.reviews.code@gmail.com. Code should be simple and in one of the following languages: C, C++, python, java, ruby. You must be the author of the code and have rights to post it. Please include the following statement in your email: "I attest that this is my code, and I hereby give Jacob Sorber the right to use, review, post, comment on, and modify this code on his videos."
You can also find more info about code reviews here.
• I want to review your ...
11:36: To me that looked like you were testing it twice with register. Maybe I'm mistaken though.
Your videos are very informative and well-structured, keep up the good work
That macro is so useful. Thanks for the videos!
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 :)
Thanks. I completely agree about limiting scope, and about variable placement being a matter of preference. Keeping them up top helps me not lose them in the mix, but it does mean more jumping up and down in the code. Keeping function bodies short simplifies this, but the tradeoff is real.
@@JacobSorber I diagree with using the pre compiler. It's bad practice to encourage because pre compilers can generate code that is not easy to debug with a debugger, pre compilers break type checking that your compiler can do and the list goes on. Scott Meyer suggests to prefer the compiler over the pre compiler. Most of the other comments made in the video I agree with. I actually like the comments after the includes as it relays why they were put there. I never ran into the problem of keeping them updated but guess I could see that being a problem in some circles.
@@revealingfacts4all Thanks. The precompiler definitely has its strengths and weaknesses (including those you mention).
More if this please, loved this.
drum roll for the outro, side by side of the two source code samples, bummer, the editor signed out.
super nice either ways
More videos about refactoring and code reviews in this code smell series
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
Definitely. Thanks for adding that. The improvements in the video are definitely not the only possible improvements. They're just the ones I had time for.
I’d like a version of this code using functions instead of comments
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...
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.
If you freeze the video at just the right spot, the code does go on to check explicitly for "." and "..", he's just replacing a complicated (but technically "faster") check with a clear check.
would be great if you could make one of these for tests in C!
question, what is the best way to organize your code, especially libraries, where they make intuitive sense? grouping by function/functional area?
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 atomic_compare_exchange_strong_explicit etc. Also if possible do you know any atomic double compare and swap ways?
9:06 Having the if statement above the variables will avoid unnecessary declarations.
Maybe. Compilers have a lot of flexibility in how they generate code. A good optimizing compiler will likely sort that out for you regardless of where you put the declarations (it's one of the easier optimizations to make). It would be interesting to time them and see if you see a difference.
As I said, it's a matter of taste. I'm usually worried more about avoiding bugs and saving my time and sanity than about optimizing every last cycle. The only exceptions for me are when the code is run very often in a CPU-intensive program or I have hardware with very tight timing constraints. So, for me, having all of the declarations at the top help me keep track of what I have on the stack for that function. Others may come to other conclusions.
@@JacobSorber Understood. Thank you very much. :)
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?
There are a lot of things baked into this comment. The short answer is, yes, of course, you could use a bash script. But...Bash is a language. So, this is really a choice between two languages. Is ruby a "common" language? It is for me. I use it all the time and like it better than bash. Others will come to different conclusions. And, of course, if things get even remotely complicated, I would switch over to some sort of testing framework, because why reinvent the wheel?
Just curious...is there any advantage to using the macros over the stdlib assert? Other than the custom message?
You typically don't want to use assert when checking anything that depends on user input. That's because asserts can be automatically removed or turned off at compile time, which would break your program. But, asserts are super useful for finding bugs in programs. A few thoughts here - czcams.com/video/1Jh9BUxIw0U/video.html
Assert uses abort() in stead of exit()?
You're welcome to be wrong haha.
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.
changing code while refactoring is called optimizing (or trying to optimize at least.)
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.
You get a lot of different opinions when it comes to code style. Maybe "simple" is the wrong word. It's more "straightforward" what the programmer is actually doing. When you just check to see if the first character is a dot, a person has to infer why that's important. New (and some more experienced) programmers sometimes forget about or aren't aware of how "." and ".." show up in their directory listings. Someone might see the original code and think this has something to do with hidden files (that start with "." on unix-based systems). I think with strcmp it's more obvious what the programmer is actually doing. But, of course, it's a matter of preference. Also, IMHO, learning the basics of the C standard library should be a fairly high priority on any C programmers to-do list.
I get what you mean but in this context, I dont see a problem. Learning standard libraries and function are the basics of any programming language. If one cant even do that then they might as well start from square one.
@@zakarouf Yeah, I agree with this. Saying using the standard library functions only serve experienced programmers threw me off a little bit. Especially because the function in questrion is strcmp. If that makes one an experienced C programmer, I'm ready for job offers as a C dev.
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.