For the Love of God Don't Write Code Like This (Clean Code with Javascript examples)
Vložit
- čas přidán 18. 03. 2023
- How to Write Clean Code! In this video we show you how to become a better software engineer. Writing high performant code is easy, writing clean code that future developers can understand? Now that's the real challenge. This video takes inspiration from Robert C. Martin's famous book "Clean Code" and illustrate the ideas from the book using examples in Javascript.
🟪 Check out AlgoMonster: algo.monster
🥷 Discord: / discord
╔═╦╗╔╦╗╔═╦═╦╦╦╦╗╔═╗
║╚╣║║║╚╣╚╣╔╣╔╣║╚╣═╣
╠╗║╚╝║║╠╗║╚╣║║║║║═╣
╚═╩══╩═╩═╩═╩╝╚╩═╩═╝
What's the ugliest piece of production code you've seen? Someone once wrote i += 0x2000 >> 13 and the whole team has yet to figure out what it means, please help.
That’s one way to write i++ wow
The ugliest piece of production code I've seen was a kludge I wrote myself.
My team makes training simulations. I took over a project that was simulating a component in our training simulation. This project was written by a mathematician. This guy was great at math and wrote a solid mathematical model. Unfortunately he never had any formal training in computer science. He would write 10,000 line files that were all just one giant function. Apparently someone had told him to stop doing that by the time he worked on this project, so he had broken his 10,000 line function into a series of ten 1000 line functions. At the end of each function, it would call the next function in the series.
Anyway, his simulation did a great job at correctly modelling physics, but presented me with two design challenges. The first challenge was that all of his models were based on predicting what would happen next and storing this information in advance so it could be sent to the computer system it was talking to in real-time. This is important because the simulation had to account for every ten milliseconds of activity. The data had to be sent at a constant rate, and the other system expected several different types of reports and if there was any discrepancy between the reports, the other computer would just drop the messages on the floor. So he created a giant revolving array so he could pre-cache and correlate every message for the next few seconds of run-time. Whenever time in the training exercise incremented, it would just send all of the messages who's timestamp was below the current time. If the simulation changes (due to user interaction), he would just find the messages in that giant revolving array and modify them. It was a bit more work on our end, but the other computer system was very temperamental and this kept it happy.
My task was to replace this method of modeling with the new generic simulation library our team had just made. We needed to do this because the new library had a bunch more features that our customer wanted. Problem was that this library worked on reporting what had already occurred in the simulation. Also, it was implemented according to clean code practices. This leads to its other problem. This generic simulation library was great at telling you what happened, but it wasn't strict about how it figured out what happened. You could poll it to find out what had happened down to ten millisecond increments, but it had a tendency to not be reliable in terms of predicting what it would do, at least down to ten millisecond accuracy.
So we just switch from prediction to the new method, right? Just poll it and use that information to correlate the different reports for the other computer, right?
Not so fast. The problem is that the computer system we are talking to is made by a competitor who doesn't really see any interest in helping our software package to work. They're system is so opaque in how it operates, it's almost a black box. Worse, the documentation they delivered is terrible. Message definitions are separate from message field definitions. They name each field in the message definition, and then you have to flip to the appendix to look up the field details in a giant (twenty page) table of names. Message fields frequently have no type information, and you have to guess based on size. Message fields occasionally don't come with units, requiring you to reverse engineer it. And none of the message definitions explain when and why you send this message to the computer.
It gets worse though. I talked with the original developer, asking if he had any working notes. Come to find out, he got this information by directly interviewing a developer from the other company. He doesn't have any notes about this interview, because the competitor had stipulated as a precondition to the interview that he was NOT ALLOWED TO TAKE NOTES. Likewise, all of his code was uncommented because they required him to NOT MAKE COMMENTS in any code changes that came about because of this interview. If he had not agreed to these terms, they would not have sent anyone.
So what this meant was that I had two mutually exclusive methods of modelling the simulation. I had to use the new model because it had new features our customer wanted. But I also could not afford to make ANY CHANGES WHAT-SO-EVER to how the original developer sent the messages to the other computer system.
The solution I came up with was to pretend the simulation was running a few seconds behind. This meant I can take the information on what had already happened and convert it into "This will happen in the next few seconds". If that sounds clean, it wasn't. Getting it to work ended up being the most god-awful kludge I've ever written. Everyone who works on this project hates it as much as I do. But they also tell me they don't see any better way it could have been done given the circumstances. That does make me feel a bit better.
100 print statements for top 100 record in javascript
Good one. Entire teams is a bunch of frauds, not programmers if they don't know how bit shifting works. ChatGPT and Python. xD
While I do agree with most of the statements in this video, I have to disagree about a couple of things. Type checking in a language like JavaScript is good because it prevents a lot of runtime bugs. Polymorphism and oop are often times hard to maintain in the long run, because you create all of those layers of abstraction and put a lot of constraints on the code by extracting the common behavior into an abstract class or an interface, and then when you have to change something it just becomes a mess. Also, if you are dealing with code which has to be very efficient then inheritance and polymorphism are a no go. It kills the performance. CPU can't predict branches, you have a lot of heap allocated objects, which cause memory defragmentation and you can't vectorize the operations. Composition is almost always better, even if you have to duplicate a little bit of code. With composition it is much easier to change things in one class and leave the others unchanged and still working as intended.
I read the book "Clean code" and your spot on, but clean code isn't the most important thing Imo, it's surely something to keep in minds and we should always try to write clean code, but some of the "Clean code" recommendation, don't really apply well in the real world, splitting functions does indeed improve readability, but it can also greatly impact performance, same goes or if else statements or switch statements, yes having some nice functions is more readable, but switch/if statements are sometimes just simply quicker, and i'm not gonna start on polymorphism OO, or inheritance. Of course performance isn't always a big concern and in this case yes the "Clean code" paradigm should be a goal. But imo a good engineer, is someone that has a good toolbox, and know which tool to use, which style fits the needs of the project, role, situation etc.
True, I totally agree. I guess in the case of this video they're using JavaScript where high performance isn't probably the goal haha. When I'm writing high performant C++ code I don't even want to think about code style, just how many constants on my runtime I can get rid of when I'm optimizing.
@@KuaisArts lol true it's just that "Clean code" is kind of language neutral. I think one of the good use cases of clean code tho is on personal project (at least for me) because unlike working at a company, on side project you have to keep in mind the entire scope of your project to not write absolute spaghetti and clean code help to read and pick up on your code faster. but we all know that it's way more fun to write totally unreadable but blazingly fast code.
@@pierreollivier1 I hate writing code that's fast but unreadable
@@user-dh8oi2mk4f sometimes you just have to left a hell of a comment to explain what it does and why especially when you use something like Bitwise operation those are really unreadable, or if you use bitfield manipulation same unless you are familiar with the technique it can look so cryptic but it's also super performant so depends on the job i'd say
the dark magic code definitely needs the comments!
Well-made video though, some things I would disagree on, but I have to say that the video is surprisingly good, keep going
Learnt many things thanks
I think the vehicle example at 5:28 does demonstrate an example where subtypes should follow the same contract to avoid switch-like code, but in some situations, subtypes don’t follow the same contract. Example: optional type might have subtypes None and Some(T). In such situations (ie sum types) we need the switch-like code
Why in the world would you want to avoid switch-like code? It's the cleanest way to collect the differences in behavior in one place! Having one large switch statement makes following the program logic much, much easier than having to compare seventeen different files with very similar code snippets for differences. The real question a sane engineer will ask at this point is, of course, the following: Why does the bridge have pillars in seventeen different architectural styles, to begin with? Does it really serve the purpose of the user to differentiate that many sub-cases? Better ask yourself why you need so many different subtypes. What advantage, other than satisfying the architect's OCD, are you deriving from such a mess? Why can't you solve it with three or even just one solution?
Also Gitlogs can be very annoying when you want to find a specific piece of code that exited for like one version three weeks ago.
good video, there are few points which I agree and few I disagree
very good advice, I almost agree on all, except perhaps on the copy in addItemToCart function for memory concerns.
But truth is, addItemToCart should be a method addItem to the Cart class and therefore modifying the Cart object
Is there a vs code theme that can make my code look like the screenshots in video
Is this video satire? I'm guessing it's satire from your "i += 0x2000 >> 13" comment, but you are playing it so straight I can't tell. Most of the code changes are harder for me to read. I mainly work in strongly-typed languages though, so maybe something is getting lost in translation?
Take your second example, "GetUserInfo(), GetClientData(), and GetCustomerRecord()". Info, Data, and Record are all synonyms. The most I get from that is the amount of data being returned. If you're using a strongly-typed language, you can just hover your mouse cursor over the function name to get that info. On the other hand, "User, Client, and Customer" contain context information about who you are getting information about. They tell you the account type.
Users: anyone using the software
Clients: users who are part of a paying organization (as opposed to personal accounts using the free version)
Customers: People making purchasing decisions (either as personal accounts or as part of an organization, may or may not be users if part of an organization)
These three terms give you much more context on what kind of information is being returned. Knowing the account type will also tell you how much information is going to be returned, and what type of information will be returned. That means the terms "User, client, and customer" also imply the terms "info, data, and records". The function names should have been, "GetUserInfo(), GetClientInfo(), and GetCustomerInfo()".
Also take your TravelToTexas() example. This one is over-simplifying to the point of hiding important information. If you have two object types that are polymorphic, then having both of them implement move() is the standard implementation. That means that having separate function names tells you something important. It tells you that the author of these two functions thought there was enough of a difference in how these functions work that they should NOT be considered the same function.
In code review I would flag your TravelToTexas() function, not drive() and pedal(). Are you really sure that bicycling cross-state is a valid use-case? If so, there is probably a lot more constraints to the trip than driving by car.
Mate there are more languages than JavaScript.
The major weakness of this video is that it explains almost none of the whys for the advice it gives. People giving advice on best practices should never forget to include the rationale for them (which often leads to a discussion of trade offs, something far more useful for young engineers)
I always thought as your objects creation parameters get overall large or you have a lot of variations of a constructor, there is a potential problem (telescoping constructor problem) and the solution was to use a Builder pattern to help this?
Yes, builder pattern is the way to go for statically typed languages. In dynamic languages like js and python, it's easier to use objects and destructuring.
@AlgoMonster On the note of the builder, I recently had a discussion with my team about it's responsibility. I put some code up for review and while we have don't it in a previous builder class, I decided to not check the validity of the constructed object which sparked a debate. Does checking if the input parameters to a builder violate the single responsibility principle? I imagine the builder was meant to solve the telescoping constructor problem, not solve that AND check it returns a valid object. I'd love to hear your thoughts on the matter.
I think one can argue both ways. As long as it's consistent either is fine. :)
5:17 It really issent... I agree that maybe it would be good to give the expressions some names... The following code should simply be showSpinner(), so the information you might derrive from the if expression really issnt helpfull. To me this is obfuscating the code unnesecarly. If the expressions where more complex, maybe you would make it more clear by moving the first expression into something like isLoading()...
If the next statement in your code would not be showSpinner() or something like that, I would focus on fixing that, so that is clear, instead of hiding the logic inside the if statement. Also another thing that would make it more clear is to fix the fsm variable, from reading the code I assume it is a Finite State machine, but that is a educated guess. I would do what you propose here: 0:53... I agree that splitting things like this out is a good thing when things become complex, or it is repeated many places, but from this issolated example, I dont think it makes the code more clear, or easier to reason about.
The other examples I agree with, but I dont agree with this one. At least not given this example
in my opinion these rules only apply to API interfaces.
I think it applies to any 'maintainable' code base
Anyone else guilty of just console logging errors and not properly handling them? 😅
I won’t even log my errors :D
IMO commented code aren't necessarily bad, be careful deleting something seems unecessary
The risk is comments and code could go out of sync. Also if we want to find previous comments, we can always find them in git logs.
No. Why make a copy of an object and not modify the original. That's slow. Software is slow enough.
functional paradigm for the win! I feel like even though software is slow already, cleaner code can go a long way for future developers on your team, especially when things go wrong. Sharing state is fine, so long as we don’t mutate it. Mutating state is fine, so long as we don’t share it.
compiler may optimize it away as mutation under the hood, just like when you write a SQL select the optimizer finds the best path to get the result
Don't have 8 parameters on a function. Have 8 parameters bunched together so they look like two... Yeah that will help reading.
The object uses keys corresponding to each argument. This provides clarity to the user, allowing them to know which argument they are assigning values to. It is a less error-prone approach compared to relying on the order of the arguments.
...But still leaves you in test hell...
Clean code? Java script? Class? Object? Are you kiddin' me? You really weren't paying attention while reading those Quake engines, or Linux source. Aaaa. You are not a C developer.
Some of this isn't really good advice, tbh.
Care to elaborate?
@@algo.monster - don't encourage people to idolise "10x engineers". They are usually toxic, and unnecessary in a well-running team.
- the getUserInfo() etc example makes the assumption that user, customer etc are the same concepts, and doesn't deal with how important it is when they really are different, and what to do in that situation. "We don't know what the difference is, so let's assume they're the same, as long as they're the same" except your point was that you don't know that.
- functions having more than 2 parameters being a problem is debatable. Also replacing them with an object with properties as a solution is kidding yourself (and doesn't solve the problem you describe around testing with all combinations of said parameters).
- on functions shouldn't take boolean arguments - I see what you mean but it's not always a universally bad thing. It's like saying you can't use 'if' statements.
- *never* do type checking? Really? Patten matching is out in functional programming, then?
I stopped watching at this point. Honestly the main problem is talking in absolutes. Never say "never"or "always". "A bool parameter may indicate your function is doing more than one thing" is possibly a more useful statement.
@@neilbarnwell
each if/switch statement is an hidden polymorphism
Dudes, you need to stop this idiotic advice to youngsters. What you name your variables is completely irrelevant to the compiler. Unless you go out of your way to obfuscate your code, whatever names you pick are fine. That is NOT what makes the difference between good and bad code.
you obviously missed the main point : readability
@@nicejungle The compiler can read "supercalifragilisticexpialidociousModelController.superheterodyneTransmitterInitializationFunctionFactoryMethodEvaluatorSuperviserIncrementCounter" just fine. Or you could write "i". It does not matter. How many people other than you are going to read that throw away code, anyway? In all likelihood you will be asked to code the same thing or almost the same thing again in three months, anyway.
@@lepidoptera9337
you obviously didn't work in a team of developers.
My bad
@@nicejungle I didn't? Do 8000 people count on the largest project I was on? I even had to read somebody's code on that one. It was horrible, even though he had good variable names. The problem was that the code performed, on average, 50 times slower than it should have. The guy didn't know about cache misses. I cleaned it up. Afterwards they had to replace the entire computer farm the code was running on because he had messed up so badly. He had caused millions in damages and months of delays. Rookies... ;-)