Debugging is harder on the single-line version. Any exceptions get thrown on the single line, and you won't easily know which part of it is causing the error.
@@IIARROWS When I have a mysterious error, I step through the code line by line until I hit it. Assuming, "Well, this is simple it will never go wrong" might be correct in this case and the next 20. But the 21st is gonna screw you.
In my opinion all of them, except the Fast one, are equally easy to read. For longer variants I would prefer the "Bad" example. But the most importent point to me is debugging. It's much easier to set breakpoints, look at varaibles and find buggs in the "Bad" example.
Is this a visual studio debugger limitation? With intellij i can just mouse hover or alt+click to see the result of any value or expression. Worst case you can set arbitrary watch points and use conditional breaks.
@@adambickford8720nope, it's the same for vs. you can also get all values with mouse over, but you of course can't set a breakpoint inside a line but only one a single line. or you go into the jitted code and set a breakpoint there on the exact instruction you want xD
@@adambickford8720 We can set breakpoints on lambdas in VS. There's a lot of flexibility, but not unlimited flexibility. We can also hover, or highlight multiple expressions and Alt+F9, to see the value. If you're just chaining a bunch of methods, I don't think you can breakpoint on an individual link in that chain (unless it's in the internal of a lambada).
As a junior dev who has a lead programmer who prefers code broken down and also has a senior dev who is always telling me “ this can be put into a single line” at every PR review… being a junior dev is tough when everyone isn’t on the same page
You are completely right, that is a very hard place to be. What is important for you, is not to choose the right thing, but rather get the understanding of the pros and cons of both ways! Keep your own focus on the learning as much as possible. Not always possible.. I know :) Just evolve yourself - From some senior dev
Get them together and tell them exactly what you said here. I'd side with the one who prefers the broken down code though. I write one liners many times myself, only to break them later while debugging stuff. While the one liners look 'cool' I find them harder to work with.
As a senior dev, I can understand the frustration and this is a good example of why companies need upfront coding standards. My current employer has almost no standards which causes our code base to be a hodge podge of syntaxes... when I was a Jr. dev, my mentor told me he shouldn't be able to tell who wrote the code by looking at it. Meaning, conform to whatever standard you're currently working in and you should be fine.
@@Zalmakiz One-liners are better (to me) because once you know what they do, it's very simple when reviewing the code. When there's a bunch of line, it'd really confusing (and ugly!) Furthermore, the one line shows it's all "one" action. But to each their own.
I just realized something. The actual 'clean' way to turn this into a one-liner is just... Using the method created here. When you create this method, you create the "One liner" you will use later on in your code. In this case, its not about "how much line do i need to reverse a string", but "okay, i have a method that reverse a string, now i just simply have to call that in one line/one simple call whenever i need it"
As a lead dev who has to cuddle and teach 3-4 junior devs, working on ERP systems, 99% of the time I don't care about performance or memory usage, unless it causes obvious problems. I don't chase nanoseconds or allocated bytes. I get that in some cases it's vital, but in our context it doesn't matter much, like I said unless there is an obvious performance problem. Instead, I focus on teaching them to write well structured self-documenting code, with proper indentation, good naming conventions, etc.. The performance optimization part comes in at peer review if necessary.
Actually you lose most of the time on db calls generated by ORM, that for some reason are insane. That's one of the most crucial skills Junior developer should learn.
@@piotrszuflicki1527well written, basic CRUD queries with a decent ORM doesn't sacrifice that much performance. Some people just abuse them and give them a bad name. Or don't know when to drop down into SQL. It's a skill issue.
@@logantcooper6 Agree 100%. Most of these bad queries would still be bad queries if it was written in SQL. Common thing I see is writing a DB query and applying a cast on the DB field in a join or where clause. Junior programmer has not figured out how to conceptualize server vs client context yet, and doesn't realize his code forces a scan on ALL the records in the table... An ORM is a tool, and like any tool there is a good way to use it and multiple bad ones.
@@piotrszuflicki1527 True, as soon as any DB calls or other external calls are involved, those are more or less the only thing you need to optimize performance vice, nothing else will even be noticeable.
In this specific case I would say I prefer an approach in which I can provide more debugging steps. The oneliner is harder to debug (or I maybe I just dont know how to do it properly), but the "bad" solution offers a better way to check the "interim results". You version is the best for "readability" I would say.
In this specific example, I do actually feel the second option is "cleaner", it allows you to read the rest of the code in the screen without having to scroll, one liners may help you understand the context of the rest of the code allowing you to see more in a single look. But I totally agree, there should be a limit to the size of those "one-liners" in order to keep your code readable for others.
What is this damn obsession with having things as short as possible? Just because it takes up less real estate doesn't make it easier to understand or read. It's like you start removing letters from a sentance so you can write and read quicker. No one does that with the English language so why should we do it with the C# language. Now this is a trivial example but nonetheless. The first example will be easier to walk through, especially if you're a junior. It's also easier to debug if something were to go wrong, instead of having to understand what went wrong in the line that pretty much does 3 things already.
Sometimes when you aggressively inline code it makes it really damn hard to debug in production. A good example (don't ask how I know) is when you try to be smart with LINQ and bundle some huge ass query into a one liner. In production if any of that throws you get the line number, but not much of a clue what part of the query failed (depending on the error). I often explode the code to be easier to identify in production. I have a fair amount of Rider "suppress this warning" statements in my code when it tries to suggest minification.
I often like to give Java a bad rep for _basically anything_ but one thing that Java does well with debugging is line-by-line debugging. When you have a method chain, like you'll get with LINQ, the Java debugger is often smart enough to let me step through each of the chained method calls one-by-one, whereas in .NET, if I chain together 10 method calls to one, it will just run this as one block and if it fails, better figure out yourself how that works. Then again, with .NET I can move the debugger pointer around and rerun sections of code multiple times, which I always miss in basically any other language.
I inherited some code like this. They first time we had a need to change the code I refactored the whole thing into different methods to represent each step of the process. At a high level it is still a single LINQ query, but the details are now separated into self contained methods so you can actually debug issues.
I frequently create long linq chains because it is easy to read once you know the functions. Algorithms start to take very specific forms that are easy for me to pattern match against. Because each extension does a specific thing its hard to abuse or get wrong. The dev is forced to adhere to the single responsibility principle instead of having giant for loops that solve multiple problems co-currently, and can't be easily decoupled. When you "explode" the code, you're making it so much harder to understand because everything becomes a for loop that does a million things. I very rarely get exceptions from linq extensions unless they're "enumeration stepped while changing" or null ref, which are usually trivial mistakes. So I am very curious what the exceptions were that were hard to debug. Nesting linqs beyond 2 levels however is bad and where it becomes unreadable. But a single level can be 100 chains long and its gorgeous.
@@Nerdsown I would guess it is that nesting that he is complaining about. That was the issue I was speaking to in my example. If there is no nesting, even if long, it is usually easy to understand. I would wrap a long LINQ chain in a method to keep the calling method cleaner.
5:00 If you want my opinion... I do like the 2nd one better, but only marginally, and that's mostly because I prefer short snappy functions, and I like method chains. I dislike the 'new string' bit though, mostly because it makes the logic progress middle-right-left, whereas the 'bad' one goes top-middle-bottom in perfect sequence. EDIT: And that's an issue both your revised approaches avoid, lol.
yeah i agree, the only thing that threw me a bit was seeing the "new string", which i haven't seen often (or at all), but otherwise i found the "good" one more readable, it reminds me of Fluent Assertions for example which ironically Nick uses all the time in his UnitTests :D
The "bad" method is more explicit about the details, but that is not always simpler. I think the real problem here is the fact that it's awkward to build an array and pass it to the string constructor. When doing something awkward or unintuitive, it can help to make the steps more explicit. But the most readable version *would* be a declarative one-liner with well-named methods that fit the purpose. "string.Concat" is an improvement but not ideal for the purpose. I think the cleanest solution would look something like this: static string AsString(this IEnumerable input) => new string(input.ToArray()); string ReverseString(string input) => input.Reverse().AsString(); Now we have a well-named general purpose method, and the usage is obvious. It's not the fastest, but if we need the highest performance then readability isn't the priority anyway - that high performance code can be hidden away in a library and just provide a public interface that is easy to use and read.
Keep it simple is key here. Doing everything in a single line doesn't make it easier. Less lines of code but way harder to read. The example is very easy overall but I'd say that in most cases keeping one operation per line is the easiest to read and less prone to bugs.
While I respect your perspective, I personally prefer sticking to the 'Good' side too. Reading from left to right and seeing what unfolds next makes it more intuitive for me. It's like the flow in PowerShell, where everything pipes together, even if it involves dotting in this case. Different approaches work for different folks, and that's the beauty of it - we can adapt our methods to what suits us best!
For me the readability of both is fine. I somewhat prefer the second just because I don't have to carry context over from other lines to figure out what a single line is doing. I know what all the bits of that one line do, I don't need extra lines to pick them apart. My preference would be the fast version though almost every time. This is a method you write once ever, do it the fast way the first and only time. I think the 'fast' method you showed isn't actually the fastest it can be. Reverse-iterating the input with a for loop and assigning inside it would be faster I think than copyto + reverse, though it would be difficult to read.
As a fan of you, I recommended span approach in the comments of this post once I saw the awful codes in image. Now I see you you did the same thing in video, I became a span monster day by day with you :)))))
I personally find the one-line variant very readable. And especially in C#, I like to keep methods in one line when possible, because it allows you to use expression-bodied members, which removes the wasted space from line breaks and brackets, thus allowing you to view more of your code at once. Plus, this is the type of method that I would probably keep as a extension method in some place I don't need to see. That way the only thing I will be seeing every day is "somestring".ReverseString(), which is self-explanatory.
On the other hand, since its so much slower, if you pack it as an extension method you most definitely want the "bad" or "fast" version, since you do not have to look at the code anyway :)
First of all, thanks for mentioning the string Create method, I was not aware of that one yet, even though I knew about Spans in different use cases already. Time to dive further in them. When it comes to using multiple dots coding style and making it more readable, my very simple answer is that you can always place these on next lines, especially in combination with LINQ methods and other functional style programming C#. In this case, the methods Reverse and ToArray are really candidates for that. But the thing is, C# is still not fully optimized to deliver the best performance with that style. Even though in a part of the cases LINQ methods can actually be the more efficient choice, in other cases they are not. And strings are indeed really critical, everyone should know that. I would say, the most important thing is to learn understand all these things and to always consider when performance matters the most or when code readability, and maybe simplicity, matters the most. It depends on the situation.
maybe not in this exact example, but in more complex code, mushing everything together in one line is a pain to debug. "Error on line 69" is useless info if all the code is on line 69.
I would choose one line ower three - without creating any variables. For me "return string.Concat(input.reverse())" is the inital way to go. There is nothing you can break in such code. You can create new one - Faster, but you cann't break it and there is nothing to debug it is pretty straightforward
When coding there are 3 possible approaches: 1. Easiest readable. 2. Most efficient 3. Least amount of code KISS should be the first approach and therefore the 'bad' example should be used: it is the quickest to understand. The 'Fast' example is obviously the most efficient, but has more parameters and is therefore harder to read than the 'Bad' example (and thus less KISS). The 'Good' example is the least amount of code. At the end of the day, most code is a balance between the 3 approaches. Or as we say in the Netherlands: the truth is always in the middle.
Yes, the 2nd is much simpler and much easier to read. I can't see why it isn't. But I would prefer to work it so it used fluent-style extension methods.
The most valuable takeaway for me was how to reverse strings fast. As for which example is "simpler" or "more readable"... that's very subjective in this case. I might go as far as to make it an expression method because what actually happens in there is very very boring. Making it any more than one line just wastes a lot of vertical space you have to scroll/scan over. When there's some actual business logic going on, or some non-trivial computations, I totally agree - use super descriptive temp variables, never do more than one thing in one line and so on. Now you could argue that mixing "styles" like that hinders readability too and that might be true for lots of people as well. Thanks for always putting stuff to the test performance-wise though. I'm not a fan of nitpicking style questions, but long discussions where people just "guess" the performance of something are extremely tedious and I love that you shut those down before they even happen.
Since there is no NET built-in way for this, we need a custom solution. My approach is simple: since this is a very simple feature, I wouldn't look for nuget packages, but just create and extension method for string that does the job, and for implementation I would choose the best in terms of performance. (And of course we need unit tests for that.) I think this is the best solution, because it gives you the clean code (abstraction through the extension method so you will probably never have to go inside the method) and the best performance too.
As a senior architect/developer, I only know a few rules: readability of code by people of all skill levels (because teams are not homogeneous), maintainability and reduction of technical debt. The rest is pedantry and nitpicking. Good code must be readable by all team members, from the weakest to the strongest (and by all those who come to replace them), it must be easily maintainable and therefore testable (separation of concerns), and it must reduce present and future technical debt as much as possible. Fashions have no place in development, and alas, many developers behave either like fashionatas, or like frustrated show-offs. That's why good developers are rare... And so is good code.
I prefer not using `var` in situations where it's not obvious what the type is, so that `new string(input.Reverse().ToArray())` line is hard for me to code review. I had to go into my editor to figure out what the returned types were to see if it was needed. For a moment, I thought `input.Reverse()` might have returned a string, making everything else pointless, but it returns `IEnumerable`. I much prefer the top version because I know what's happening and the types being used. Also, I prefer more lines of code rather than several statements merged into 1 line for a number of reasons: - I can put breakpoints on separate lines and inspect the values of variables much more easily. - The exception/stacktrace information reported from log error messages is much more exact and therefore much easier to fix bugs because the line number would point to only 1 or a couple of operations instead of several on the line at fault; In the original example, if the stacktrace suggested that the `new string(input.Reverse().ToArray())` line was at fault then was it `input.Reverse()` causing the issue, or `ToArray()`? - Keeping a large number of operations on a single line instead of breaking them up into multiple lines is the equivalent of someone forgetting to use any fullstop in a huge paragraph versus using many to break up the pacing; it's just much easier to read and understand. EDIT: Got to the end of the video and yea, use string.Concat for readability and simplicity, or the fast version for memory optimisation scenarios.
Honestly, I'd just put the fast approach into an extension method, and then keep string reversing logic away from the business logic. When you're working on the extension method, I don't mind if it's not "clean", its a single purpose function that you already know the intent of when you begin looking at it.
When Martin Fowler lists the Refactoring techniques in his book, each technique is always coupled with another as its opposite, e.g. Extract and In-line. What is shown here is an example of In-line expression refactoring, and if doing the opposite, it is the Extract. As Refactoring's purpose is to make code design simpler in various aspects (flexibility, readability, debug-ability, etc.), performing a technique itself, e.g. In-line here, does not means serving that purpose without the context and scenario it is applied to. So I full-heartedly agree with Nick when pointing out the Linked-in post, as the post's author seemingly assume In-line refactoring into 1 line is a silver bullet for simplicity. Personally, I enjoy chaining methods in Functional paradigm since it can lead to nice readability, but I don't want to limit within that, and when opportunities present, will still perform Extract of the chaining later either for debug-ability or reusability. P.s. I personally think the author's in-line/chaining still does not improve much of readability while sacrificing debug-ability in vain. I would prefer sth like "return input.Reverse().ToArray().ToStringFromCharArray();", not "return new string(input.Reverse().ToArray());". If you mean to chain, at least chain all the way, not putting the chain as a method or a constructor's parameter.
I like less lines of code because you can fit more on the screen. For the reverse string array, both implementations are fine imo, but I would personally strive for the second option
For readability, I would prefer chained code (broken into lines where necessary) - I just see that it is a single expression, the code flow is simple. Properly named methods in one chain can "tell the story" clearly enough. Absence of throw-away names means the remaining named variables are important. I can give names to intermediate results even if they are used once - when descriptive names are really important there. (But I regret my style choices sometimes when debugging - have to get back and break expressions into statements to step through them...) The example in the video has one thing I can frown upon though - expression doesn't read from left to right - it ends with a constructor that is on the left. In such case I may also break it into nicer parts or check if introducing an extension method would be justified... But the benchmark is the most surprising part of this video. It raises more questions about how well the compiler actually optimized. I don't want to be questioning code style for such performance differences.
I think in this very specific case you are writing a method that you'll write once and never look at again. The task is a simple one so it is unlikely to ever need debugged. Since this is something that could easily be used all over your code base, I think the preference should be for optimized code.
The first method is much cleaner than the second and the Nicks version. The reason is because everyone is used / has faced this version so it more readable.
In the realm of software development, YKD principles - YAGNI, KISS, and DRY - lay the groundwork for clean code. Beyond these, the art of great coding is achieved by minimizing cognitive load, enhancing readability, and simplifying the process of debugging and testing. Great coding isn't just about writing the fewest lines or creating the perfect YKD example; it's about ensuring the maintainability of that code, making it not only functional but also enduring and comprehensible.
Let's abstract the problem, meaning what would we prefer, a loaded line of code, a well readable multi-line code or an optimized one that's harder to understand. I prefer function over form, so I'd pick the performant one, but only if it is relevant. The other 2 I don't care about - it's a question of execution (how it's written) to tell which is better. If it's something more complex, I prefer to use multiple lines as to make it more understandable. If it's something trivial, I prefer one-liners.
I prefer the "good" example in this case because it's simple and the code reads semantically left to right. Reading it makes me think "this method return a string, okay, that is the input string, okay, but reversed, okay, and converted to a char array, oh because that's how the string constructor works". It communicates that the intermediate states of the string are not important to understand the intended result. The version with multiple assignments, to me, is harder to read since I see "oh, you've created an array, why? is that important, do you need it later?" - this "clean code" tip seems to be "don't define temporaries if you only use them in the next line, and that's the only time you'll use them" which I _mostly_ agree with (and I know the actual assignment will be optimized but the way you understand/debug the code isn't optimized). I take the point regarding the "bad" one being easier to debug, but the "good" one wouldn't be _hard_ to debug, would it? Obviously for more complicated examples (especially if the temporaries are being passed as function arguments in a one-liner instead of method-chaining like this) defining temporaries can be clearer, but not in this case, in my opinion.
Using swap is the fastest way, swap head and tail. (Creating Span with stackalloc to handle it) And the only way to beat up above code is unsafe pointer.
I don't know if this makes me the odd one out. But in the first example, I tend to prefer code that reads like a sentence. I like having single line ifs that don't break the flow, and I like not having to name useless intermediate variables. So when I read that a function returns a new string that's constructed from input that was reversed and turned into array, I find that better than reading three unconnected lines of code.
The second one is more functional (as in the paradigm). Some people prefer functional programming but I'm more of a procedural guy so the first one looks better to my eyes. I think it comes down to your preferred paradigm.
In general, here I'd vote for the "fast" implementation, even despite it is not best readable, because the speed is what we really want from such utility functions.
Speed is only one of the things we want. We also want the code to be correct. Math.Add can always return the wrong answer very fast if it doesn't bother to actually add. We also want the code to be maintained by a team of humans who need to be able to comprehend it.
@@7th_CAV_Trooper Yes, that's correct. We of course want the code to be correct. But given it is correct, next things we typically want are speed and low memory usage, preferably, zero memory usage.
On the first example, I much prefer the “good,” compared to the bad. I find going through one line much easier, reads like a simple instruction. “It returns a string, which is the result of reversing the input and turning it into a new array.”
The OO calisthenics do not talk about one dot per line. They talk about not going deeper within the class(module) with the idea of following the Law of Demeter. In that first example (reverse string), what happens is chaining methods, and is completely fine.
There is two extra reasons to separate lines: Step by Step debug, and exception stack trace with code line of error. Sure the IDE helps, but is easier to debug if its separated. I use "one logical operation by line" guide line.
If performance is not super important, I always go for readability and maintainability. I don't like when multiple steps are combined just to be 1 row instead of 3. It's also way harder to debug, you will end up separating the steps anyway to see what's happening.
On top of the above there will be probably more than one person trying to figure out what the author had on mind writing the code. Main question is the purpose of the code - does it need to be i.e. low latency or some performance trade off can be done, in which case easy to follow and understand is the way to go
Not surprising, Linq and Enumerable is rarely the fastest since its quite a lot of overhead. It can make for easy to read code (if you stay away from long one liners) but compared to arrays its very much slower.
Rather than avoid multiple dots per line, I find the greatest readability improvement in avoiding nested brackets (of any type) in a single line, since in order to read that code you need to go back and forth which is generally more difficult. Extracting variables like this is also a good test of code understanding, since if you struggle to find a good name for an intermediate variable, there may be an issue with the approach.
Exactly, while you can read the code left to right to understand it, it doesn't actually evaluate in that order with that nesting. I don't mind more than one dot per line if it's an extension method for mapping/casting/conversion or using a builder pattern, but once you start evaluating out of order it should be broken up. Honestly, I wouldn't call the single line in this example "bad" as I don't have the luxury of being that nitpicky in a code-review, I just take actual issue with it being called better.
@@exmello Yeah exactly. The situation in the example is just making a string out of a character array which is not a huge issue and I would let it slide, but imagine if it was a more complex passing of an argument into a function or constructor.
if you have a lot of vars as middle steps, it will be harder to read. One liner is easier to read as you can literally read it as a sentence of a LINQ style query. Pointed debugging steps is a very rare case overall. If you need each step in debug - two automatic var extract refactors and you'll get it. Sometimes in the code simple chain operations are so much disintegrated to pieces with vars and names so it's very hard to read. Less code - less to read. Even make it a => instead of {}
Static analyzers usually give pretty bad advice on method chaining random stuff together. Mathematically putting everything in 1 chained line gives you a lower Cyclomatic complexity - so some people just blindly assume that is actually better
Multiple dots style leads to somewhat shorter code. I'd use this approach, however with formatting that puts every dot on a separate line. Also can use in a single line, when the code is likely to be "write once and forget".
In personal projects I would use the single-line approach bc I like it and understands it, but that's only personal choice, in my work/real world I will use the first one
I don't mind chained method calls when it's just two methods as shown in the example in the video. So in this example in particular, the "good" code is indeed more readable for me than the "bad" code. If there are more chained calls, then I would break them down into multiple lines, like people usually do when they are using Linq. But I would still not create variables for intermediate states, unless they could somehow improve readability of the code. This is all about style though. People who like functional programming probably favor chained method calls, whereas people who prefer procedural programming favor creating variables and calling functions in a step-by-step manner. There is nothing wrong with either approach, and I wouldn't block a pull request in a code review if someone were to favor one style over the other. What bothered me in this code is that I immediately noticed their performance would be terrible. And your benchmarks proved me right. Now that would be a good reason to block a pull request.
The reason it's hard to read is that you need to jump back and forth horizontally to follow the data. As long as the data flows in one direction, left or right, then chaining is not complicating anything, IMO
I prefer conditional extraction too. Sure, it's another line of code (or more), but at least it's more readable, especially if you invert the negative conditionals to be positive conditionals. It's along these same lines.
For me, the main reason i hate these 'one-liners', is not just it generally much harder to read, 'debugability/debug experience'. When you have a one-liner like that, debugging that is pain, i often had to separate them to individual lines to find what the exact problem is.
I'm okay with more than one dot per line, as long as that's the only thing happening in the line. For instance, I'm okay with: return input.Reverse().ToArray().toString(); [I'm not sure if there is such a method, I'm not from the .NET bubble -- I just love your content!]
I try to avoid evaluating complex expressions as method parameters. The nesting leads to poor readability and debugging. Splitting into separate lines fixes those issues and is more logically coherent: each line has a much more singular purpose and is read+executed in stepwise order before the method call is made.
I tend to avoid creating unnecessary variables. This is conditioning from the old adage, "number of bugs is proportional to the number of variables". However, I now think that that adage is actually a result of having functions doing too much in first place.
For some reason, the ML Java / Apache Spark community is obsessed with creating massive one-liners in their code. It’s like some kind of badge of honor to create barely readable scripts to do simple tasks.
The LinkedIn one-liner is still more readable than the first approach, though. But, since ReverseString is a common utility function that's going to be written once and used all over the codebase, aesthetics and readability don't really matter and you should focus on performance.
A friend of mine is obsessed with one-lining everything in python but only does it for shits and giggles (e.g.: he took a ~250 line program I wrote during a hackathon and compressed it down into one line just to demonstrate to me what weird shit you can do in python). He'd never actually commit any of that to their codebase
When I just started, I was able to compress all sorts of extensive calculations and comparisons into a single line of code. Until I found out that after half I year it took me too much time to decipher what exactly it did, so I started breaking it down into comments. Now I'm experienced I just use some more lines just to make things easier readable. The compiler's optimizer will do the rest for me.
When I need to jump into an unknown project to do something, the simple code with more lines, fewer files and classes are always easier to navigate and I always appreciete them the most. The ones with the smart one liners and / or piles of abstractions and files just gives me a headache. When I need to fix something asap, I couldn't care less how smart the person who wrote the code was I just want to do the thing I was supposed to do, not solve some wierd code riddles 😂 I get it though, you do some thing and then realize you can do it in another way and then you realize you can do it even more smart etc. Feels good so you tell yourself it's "clean" but there is no such thing, it's just opinion. It should be about: does the code solve the real problem in a satisfactory way? Does the newest person understand it? If those are yes, then that's good code.
input.Reverse().toArray() - reverses the input and then converts it to an array. Does what it says on the tin, in one line. What am I missing? Who gets confused by this? I'd have to look at the first one for longer to work out what it's doing, the second one reads almost like English. The first one, we have to know that charArray is mutable, and that the action performed by Array.Reverse affects it in place (and also, we now have to have some idea of what the Array class is and what its static methods do). Also, we've introduced the concept of a CharArray, which we didn't need to know about when just calling the Reverse() method on a string. That said it's weird that C#'s Reverse method doesn't just return another string - Kotlin just does "My String".reversed(), and then none of this would even matter.
I always go for performance. If the ugliest code ever is faster, I'll use it. I work processing Gigas of data every morning, and speed is everything to my clients so they have their data available in time. Of course, nothing is carved in stone here, and you need to check your situation before deciding.
I love this example of reversing a string. It made me think... The two functions dont do the same thing even though they return the same thing. Array.Reverse() in the first function is not the same as String.Reverse() in the second function. In this instance, i prefer the bad one. But I believe that the second function simply could be returning input.Reverse() and skip the .ToArray and the new string() parts. 😊
04:19 - next time I recommend using Rider's introduce local refactoring action, you can do it in very few keystrokes, its faster, simpler, less error prone.
I tend to use expression body functions to signal that it's a "pure" function (i.e. no side effects or in-memory replacements, etc.). I find expressions easier to understand because I know that the output follows naturally from the input. I don't need to remember that the contents of a variable or an array have changed, adding cognitive load. So yeah, I like the ReverseString_Nick version the best but I'd use an expression body: string ReverseString_Better(string input) => string.Concat(input.Reverse()); To handle unicode combine characters correctly, you'll have to use StringInfo.GetTextElementEnumerator: IEnumerable ToTextElements(string source) { var enumerator = StringInfo.GetTextElementEnumerator(source); while (enumerator.MoveNext()) { yield return enumerator.GetTextElement(); } } string StringReverse_Best(string input) => string.Concat(ToTextElements(input).Reverse());
Code readability is always appreciated rather than a chained single line code with a justification of "its more optimised" with no actual noticeable difference 😂
I prefer the first option. The second option looks like a more elegant solution BUT in case if I need to debug or add more specific logs in that code. The first one solution is better and it is easier to read. One action, one line.
Using one-liner methods isn't always the optimal approach. It doesn't necessarily align with the KISS principle. At times, splitting code across multiple lines can be more beneficial. Debugging becomes simpler when examining log files. For example, if an exception occurs on line 36 and that line features multiple chained methods, determining the exact location of the exception can be challenging without running the app with a breakpoint. Simplicity doesn't always mean brevity ;-)
A lot of people equate less lines with simplicity but that often leads to these issues. You're free to make the entire program one line if so wish but nobody thinks that's a good idea (JS minifiers excluded!) For the exact same reasons, it's probably not a good idea for your functions either
Obviousness is what's important. I prefer compactness when it's obvious what the compact line does as a chunk. Whereas I would keep less-related steps in separate lines, with maybe two transportations. The chained, compact style was common in Ruby coding a while ago. It's a common preference.
Keep it simple != Less character to right A code MUST be reable like a book with space, short sentence, one idea by paragraphe, argument align and wrap ... When you look at the code you should be able to identify directly the role of each element without thinking
Depends on your definition of "Clean Code". For me it means readable/maintainable code. In this trivial example I'd not be worried about either of those. In general though I consider expanded version "cleaner".
I think the shorter version is easier to read because I dont have to keep track of what has happened to chararray before it is used in the string constructor.
It doesn't have any sense. Like... we're creating the new instance of string, and we definitely SHOULD NOT manipulate the values of original string while using span. But If this comment is a simple joke, I have no objection :)
@@winchester2581 The span just servers as a fast reading technique. The modified values are on a different address. So it makes sense to use spans. And it does use spans under the hood.
In the end, this is just a matter of aesthetic prefference really. The performance is the same. Sometimes I'd use linq, or the "=>" arrows to make things less jarring for the eye, as long as readability is maintained. The first example is easier to read in my opinion. Minus points to the second example for not using lambda expressions. Neither are really a "good" or "bad" example, just a matter of prefference. If the method was one which had more potential points of failure, I'd rather have it more seperated, if it's just doing basic logic, I'd love the method to take as little space as possible.
The ReverseString_Good is fine to me and what I would do, but that’s based on how I came to coding. Chaining functions isn’t really difficult for me to parse. It easily reads as “take a string, reverse it, and give it to me as an array, then make a new string out of it”. That’s not to say the “bad” approach is something you shouldn’t do. Everything except the fast one is pretty much equally easy to read for me. I’ll admit, I’m a bit surprised that the “bad” code is so fast compared to the one-liner, considering how similar they are conceptually. But that level of performance difference isn’t necessary for my use cases.
I don't see the dot chaining as a problem, that portion is very readable to me. Each dot represents a sequential step in a nice clean predictable sequence. Taking that dot-chained expression and making it a parameter of the string constructor is where it becomes less readable. It's not the method chain that's the readability problem, nor is it the parameter, but nesting one inside the other. If I had been handed the the problem and asked to make it "clean", I would have probable wound up with: public string ReverseString(string input) { var reversed = input.Reverse().ToCharArray(); return new string(reversed); } The method chain on the first body line is nicely readable, representing a simple sequence. The return line with a string constructor is also pretty straightforward. This is assuming we don't need to deal with any culture/locale considerations like accent marks. In a real world situation, I'd probably be using a string enumerator to break into a collection of grapheme clusters, not chars, which would be less readable but more robust unless I could absolutely guarantee that it isn't needed.
l mean if you already mushing it all together in ONE return statement you really have to loose the curly braces and do a "=>" construct! Just one line compared to 4 MUST be less code! 😉😇🤪
The one line code is a reading nightmare, hard to debug and is ugly! I'm much more into readability than shortness! Extreme performance reasons excluded...
clever one line linq expressions can be a pain to debug (unless you're using something like the ozcode extension). when it is not something extremely simple like here I would prefer the multi step version.
I’d likely put an extension in play to make it more readable, e.g. input.Reverse().AsString(). It’s readable to me, but i wouldn’t complain if it were the “Bad” in the code base. Maintainability is more important than my ego.
I find the single-line version better. In my opinion, it does not really change the readability of the code. Also reversing a string is a simple and straightforward process. If I was doing something complicated where I would need to be extra careful or had to constantly debug it then I would use the longer format.
It actually took me less time to understand the shorter function than the first because in the first one, I had to keep all the variables and context in my head, whereas on the second one i could just easily see that the string is reversed, then copied. Adding the extra lines doesn't help because it is such a simple function that adding lines just obfuscates it a little bit
I would write a .ToString() method (maybe with a different name) that works on char arrays and then I could write => input.Reverse().ToArray().ToString(); if you wrote one for enumerables of char, it could become => input.Reverse.ToString();
I am very bad about writing 3 line of code that replaces 300 lines. It’s not a good practice, as it’s inpossible to debug or maintain but, for me, it’s an optimization step at the end of my method creation. I generally keep the 300 line version (commented out), for the support team.
Once, a friend told me, 'I improved your code.' He rewrote my code into a single line. His lack of experience didn't allow him to realize that I had to debug that code to provide him with tested code
I don’t like a return statement that is doing work as part of the return statement. When Eve an exception might be thrown, I want to know exactly what part of the logic is throwing the exception. So if you do it all in one line, it isn’t simple to debug any problems in the future.
Excellent video, these were my thoughts exactly! People make these wrong arguments about clean code all the time, I used to feel totally weirded out!!!
Yeah, I disagree with this take quite strongly, at least for *this* example. I would even go one step further, get rid of the braces and make it into an expression-bodied method (on two lines). The second version has significantly less characters and reads *perfectly* from left-to-right. Reading one line from left-to-right is obviously much easier than reading left-to-right then up-to-down and repeating that multiple times. And if we're talking about a 200 line file vs a 600 line file, where the later expands as much as possible into multiple lines, the former is far easier to navigate and take in at a hundred foot level. Now, I did say "at least for this example." Compacting everything into a single line is not always the best choice. We always need to be pragmatic and avoid inflexible rules because there are always edge cases. At the extreme, one-liners can become like run-on sentences, and it's not always possible to read a line perfectly from left-to-right. You might, for example, have to jump between sets of parentheses to decode the meaning, and that's not good. This example however, is an exemplar for when you, as a pragmatic programmer, should go for the one-line version.
in terms of readability, the "good", "bad", and "nick" seem close enough to not matter. if I saw either of these in a PR, I don't think I would question either one. (my bigger question would probably be "they needed to reverse a string? what are they doing in here??") if I saw the "fast" code, I'd probably be like "wtf is all this??" unless there was a comment, or other context, making it clear "this is a hot path! do not refactor without performance testing!"
I prefer one liners for quick prototyping, but at some point you'll make an extension method out of such code and that's your readability. What's happening inside the extension can be optimized and the code doesn't matter, since you know it's some "Reverse_something".
For me it's not so much about one-liners vs many-liners, but if the code is self-explanatory. Sure you can have cool one-liners with several functions that do different things, but if it's not apparant what each function does you'd need comments (and is it really a one-liner when you need to comment?). If it's hard to understand what the code does, splitting it up into multiple lines with intermediate variables can be helpful in understanding what it does, and you can easier step through if it needs debugging. On the other hand you have performance critical code, where you need to use complex functions or get into unsafe territory. Here it's not so much of clean code or one-liners (though you can still try), but optimize it for speed or memory footprint however you need. These can quickly be complex and forcing clean code can sometimes be the opposite of what you need your code to be. Here it might be better to just have more lines, and even comment sections of it to make it clearer what it does. Also, the compilers are really great at optimizing. Code you write as "clean code" might perform worse (and sometimes way worse) compared to "unclean code".
This is a big subject that touches a lot on locality of concerns and the many different contexts that humans need to interact within. Focusing specifically on the first example, I'll add: Mixing confluent dot syntax and function application syntax on the same line can be jarring when the order of operations is important because they read in different directions. Record accessors read left-to-right, and function application reads applied right-to-left (and inside-out) on the same line. Generally mixing styles on a whim *is* jarring, so in cases where this is warranted (there are many), perhaps preparing the audience (and being a good audience) is wise.
Debugging is harder on the single-line version. Any exceptions get thrown on the single line, and you won't easily know which part of it is causing the error.
Yep. Sometimes I have to break the line to debug and then put the lines back together for so that the code reviewer doesn't complain.
that's a debugger/ide issue, not a code issue
I agree a 100% to this. We do not need to compact/compress the code, code is maintained by humans, not machines.
@@IIARROWS When I have a mysterious error, I step through the code line by line until I hit it. Assuming, "Well, this is simple it will never go wrong" might be correct in this case and the next 20. But the 21st is gonna screw you.
"Debugging is harder"
Only soy developers debug. (at least according to similar videos)
In my opinion all of them, except the Fast one, are equally easy to read. For longer variants I would prefer the "Bad" example. But the most importent point to me is debugging. It's much easier to set breakpoints, look at varaibles and find buggs in the "Bad" example.
Is this a visual studio debugger limitation? With intellij i can just mouse hover or alt+click to see the result of any value or expression. Worst case you can set arbitrary watch points and use conditional breaks.
@@adambickford8720nope, it's the same for vs. you can also get all values with mouse over, but you of course can't set a breakpoint inside a line but only one a single line. or you go into the jitted code and set a breakpoint there on the exact instruction you want xD
@@the_lenny1You can really only set 1 breakpoint per line? How do you stop on the execution of a lambda vs the definition of it for a 1 liner?
@@adambickford8720 We can set breakpoints on lambdas in VS. There's a lot of flexibility, but not unlimited flexibility. We can also hover, or highlight multiple expressions and Alt+F9, to see the value. If you're just chaining a bunch of methods, I don't think you can breakpoint on an individual link in that chain (unless it's in the internal of a lambada).
Also easier to see line coverage while writing unit tests
As a junior dev who has a lead programmer who prefers code broken down and also has a senior dev who is always telling me “ this can be put into a single line” at every PR review… being a junior dev is tough when everyone isn’t on the same page
this was very annoying to me to, never could ever explain me why things are better when they are in one line.
You are completely right, that is a very hard place to be. What is important for you, is not to choose the right thing, but rather get the understanding of the pros and cons of both ways!
Keep your own focus on the learning as much as possible. Not always possible.. I know :) Just evolve yourself - From some senior dev
Get them together and tell them exactly what you said here. I'd side with the one who prefers the broken down code though. I write one liners many times myself, only to break them later while debugging stuff. While the one liners look 'cool' I find them harder to work with.
As a senior dev, I can understand the frustration and this is a good example of why companies need upfront coding standards. My current employer has almost no standards which causes our code base to be a hodge podge of syntaxes... when I was a Jr. dev, my mentor told me he shouldn't be able to tell who wrote the code by looking at it. Meaning, conform to whatever standard you're currently working in and you should be fine.
@@Zalmakiz One-liners are better (to me) because once you know what they do, it's very simple when reviewing the code. When there's a bunch of line, it'd really confusing (and ugly!) Furthermore, the one line shows it's all "one" action. But to each their own.
I just realized something. The actual 'clean' way to turn this into a one-liner is just... Using the method created here. When you create this method, you create the "One liner" you will use later on in your code. In this case, its not about "how much line do i need to reverse a string", but "okay, i have a method that reverse a string, now i just simply have to call that in one line/one simple call whenever i need it"
Yup, and I think ideally you would do this as an extension method.
As a lead dev who has to cuddle and teach 3-4 junior devs, working on ERP systems, 99% of the time I don't care about performance or memory usage, unless it causes obvious problems. I don't chase nanoseconds or allocated bytes. I get that in some cases it's vital, but in our context it doesn't matter much, like I said unless there is an obvious performance problem. Instead, I focus on teaching them to write well structured self-documenting code, with proper indentation, good naming conventions, etc.. The performance optimization part comes in at peer review if necessary.
Actually you lose most of the time on db calls generated by ORM, that for some reason are insane. That's one of the most crucial skills Junior developer should learn.
@@piotrszuflicki1527well written, basic CRUD queries with a decent ORM doesn't sacrifice that much performance. Some people just abuse them and give them a bad name. Or don't know when to drop down into SQL. It's a skill issue.
@@logantcooper6 Agree 100%. Most of these bad queries would still be bad queries if it was written in SQL. Common thing I see is writing a DB query and applying a cast on the DB field in a join or where clause. Junior programmer has not figured out how to conceptualize server vs client context yet, and doesn't realize his code forces a scan on ALL the records in the table... An ORM is a tool, and like any tool there is a good way to use it and multiple bad ones.
@@piotrszuflicki1527 True, as soon as any DB calls or other external calls are involved, those are more or less the only thing you need to optimize performance vice, nothing else will even be noticeable.
In this specific case I would say I prefer an approach in which I can provide more debugging steps. The oneliner is harder to debug (or I maybe I just dont know how to do it properly), but the "bad" solution offers a better way to check the "interim results". You version is the best for "readability" I would say.
In this specific example, I do actually feel the second option is "cleaner", it allows you to read the rest of the code in the screen without having to scroll, one liners may help you understand the context of the rest of the code allowing you to see more in a single look. But I totally agree, there should be a limit to the size of those "one-liners" in order to keep your code readable for others.
What is this damn obsession with having things as short as possible? Just because it takes up less real estate doesn't make it easier to understand or read. It's like you start removing letters from a sentance so you can write and read quicker. No one does that with the English language so why should we do it with the C# language.
Now this is a trivial example but nonetheless. The first example will be easier to walk through, especially if you're a junior. It's also easier to debug if something were to go wrong, instead of having to understand what went wrong in the line that pretty much does 3 things already.
Sometimes when you aggressively inline code it makes it really damn hard to debug in production. A good example (don't ask how I know) is when you try to be smart with LINQ and bundle some huge ass query into a one liner.
In production if any of that throws you get the line number, but not much of a clue what part of the query failed (depending on the error). I often explode the code to be easier to identify in production. I have a fair amount of Rider "suppress this warning" statements in my code when it tries to suggest minification.
I often like to give Java a bad rep for _basically anything_ but one thing that Java does well with debugging is line-by-line debugging. When you have a method chain, like you'll get with LINQ, the Java debugger is often smart enough to let me step through each of the chained method calls one-by-one, whereas in .NET, if I chain together 10 method calls to one, it will just run this as one block and if it fails, better figure out yourself how that works. Then again, with .NET I can move the debugger pointer around and rerun sections of code multiple times, which I always miss in basically any other language.
This one I also have to much of the wrong experience with ;)
Long linq statements are very hard to debug, and as shown, most likely not faster anyway.
I inherited some code like this. They first time we had a need to change the code I refactored the whole thing into different methods to represent each step of the process. At a high level it is still a single LINQ query, but the details are now separated into self contained methods so you can actually debug issues.
I frequently create long linq chains because it is easy to read once you know the functions. Algorithms start to take very specific forms that are easy for me to pattern match against. Because each extension does a specific thing its hard to abuse or get wrong. The dev is forced to adhere to the single responsibility principle instead of having giant for loops that solve multiple problems co-currently, and can't be easily decoupled.
When you "explode" the code, you're making it so much harder to understand because everything becomes a for loop that does a million things.
I very rarely get exceptions from linq extensions unless they're "enumeration stepped while changing" or null ref, which are usually trivial mistakes. So I am very curious what the exceptions were that were hard to debug.
Nesting linqs beyond 2 levels however is bad and where it becomes unreadable. But a single level can be 100 chains long and its gorgeous.
@@Nerdsown I would guess it is that nesting that he is complaining about. That was the issue I was speaking to in my example. If there is no nesting, even if long, it is usually easy to understand. I would wrap a long LINQ chain in a method to keep the calling method cleaner.
5:00 If you want my opinion...
I do like the 2nd one better, but only marginally, and that's mostly because I prefer short snappy functions, and I like method chains.
I dislike the 'new string' bit though, mostly because it makes the logic progress middle-right-left, whereas the 'bad' one goes top-middle-bottom in perfect sequence.
EDIT: And that's an issue both your revised approaches avoid, lol.
yeah i agree, the only thing that threw me a bit was seeing the "new string", which i haven't seen often (or at all), but otherwise i found the "good" one more readable, it reminds me of Fluent Assertions for example which ironically Nick uses all the time in his UnitTests :D
The "bad" method is more explicit about the details, but that is not always simpler. I think the real problem here is the fact that it's awkward to build an array and pass it to the string constructor. When doing something awkward or unintuitive, it can help to make the steps more explicit. But the most readable version *would* be a declarative one-liner with well-named methods that fit the purpose. "string.Concat" is an improvement but not ideal for the purpose. I think the cleanest solution would look something like this:
static string AsString(this IEnumerable input) => new string(input.ToArray());
string ReverseString(string input) => input.Reverse().AsString();
Now we have a well-named general purpose method, and the usage is obvious. It's not the fastest, but if we need the highest performance then readability isn't the priority anyway - that high performance code can be hidden away in a library and just provide a public interface that is easy to use and read.
Thats by far the best approach readability-wise
Keep it simple is key here. Doing everything in a single line doesn't make it easier. Less lines of code but way harder to read. The example is very easy overall but I'd say that in most cases keeping one operation per line is the easiest to read and less prone to bugs.
While I respect your perspective, I personally prefer sticking to the 'Good' side too. Reading from left to right and seeing what unfolds next makes it more intuitive for me. It's like the flow in PowerShell, where everything pipes together, even if it involves dotting in this case. Different approaches work for different folks, and that's the beauty of it - we can adapt our methods to what suits us best!
For me the readability of both is fine. I somewhat prefer the second just because I don't have to carry context over from other lines to figure out what a single line is doing. I know what all the bits of that one line do, I don't need extra lines to pick them apart. My preference would be the fast version though almost every time. This is a method you write once ever, do it the fast way the first and only time.
I think the 'fast' method you showed isn't actually the fastest it can be. Reverse-iterating the input with a for loop and assigning inside it would be faster I think than copyto + reverse, though it would be difficult to read.
As a fan of you, I recommended span approach in the comments of this post once I saw the awful codes in image. Now I see you you did the same thing in video, I became a span monster day by day with you :)))))
I personally find the one-line variant very readable. And especially in C#, I like to keep methods in one line when possible, because it allows you to use expression-bodied members, which removes the wasted space from line breaks and brackets, thus allowing you to view more of your code at once.
Plus, this is the type of method that I would probably keep as a extension method in some place I don't need to see. That way the only thing I will be seeing every day is "somestring".ReverseString(), which is self-explanatory.
On the other hand, since its so much slower, if you pack it as an extension method you most definitely want the "bad" or "fast" version, since you do not have to look at the code anyway :)
First of all, thanks for mentioning the string Create method, I was not aware of that one yet, even though I knew about Spans in different use cases already. Time to dive further in them.
When it comes to using multiple dots coding style and making it more readable, my very simple answer is that you can always place these on next lines, especially in combination with LINQ methods and other functional style programming C#. In this case, the methods Reverse and ToArray are really candidates for that.
But the thing is, C# is still not fully optimized to deliver the best performance with that style. Even though in a part of the cases LINQ methods can actually be the more efficient choice, in other cases they are not. And strings are indeed really critical, everyone should know that.
I would say, the most important thing is to learn understand all these things and to always consider when performance matters the most or when code readability, and maybe simplicity, matters the most. It depends on the situation.
Code Cop sounds like a better name for a series since it sounds like an existing analyzer like Style Cop.
maybe not in this exact example, but in more complex code, mushing everything together in one line is a pain to debug. "Error on line 69" is useless info if all the code is on line 69.
I would choose one line ower three - without creating any variables. For me "return string.Concat(input.reverse())" is the inital way to go. There is nothing you can break in such code. You can create new one - Faster, but you cann't break it and there is nothing to debug it is pretty straightforward
When coding there are 3 possible approaches:
1. Easiest readable.
2. Most efficient
3. Least amount of code
KISS should be the first approach and therefore the 'bad' example should be used: it is the quickest to understand.
The 'Fast' example is obviously the most efficient, but has more parameters and is therefore harder to read than the 'Bad' example (and thus less KISS).
The 'Good' example is the least amount of code.
At the end of the day, most code is a balance between the 3 approaches. Or as we say in the Netherlands: the truth is always in the middle.
Yes, the 2nd is much simpler and much easier to read. I can't see why it isn't. But I would prefer to work it so it used fluent-style extension methods.
The most valuable takeaway for me was how to reverse strings fast. As for which example is "simpler" or "more readable"... that's very subjective in this case. I might go as far as to make it an expression method because what actually happens in there is very very boring. Making it any more than one line just wastes a lot of vertical space you have to scroll/scan over. When there's some actual business logic going on, or some non-trivial computations, I totally agree - use super descriptive temp variables, never do more than one thing in one line and so on. Now you could argue that mixing "styles" like that hinders readability too and that might be true for lots of people as well.
Thanks for always putting stuff to the test performance-wise though. I'm not a fan of nitpicking style questions, but long discussions where people just "guess" the performance of something are extremely tedious and I love that you shut those down before they even happen.
The second definitely breaks my favorite rule, one dot per line. Makes debugging and seeing what is breaking much easier.
Since there is no NET built-in way for this, we need a custom solution.
My approach is simple: since this is a very simple feature, I wouldn't look for nuget packages, but just create and extension method for string that does the job, and for implementation I would choose the best in terms of performance. (And of course we need unit tests for that.)
I think this is the best solution, because it gives you the clean code (abstraction through the extension method so you will probably never have to go inside the method) and the best performance too.
As a senior architect/developer, I only know a few rules: readability of code by people of all skill levels (because teams are not homogeneous), maintainability and reduction of technical debt. The rest is pedantry and nitpicking.
Good code must be readable by all team members, from the weakest to the strongest (and by all those who come to replace them), it must be easily maintainable and therefore testable (separation of concerns), and it must reduce present and future technical debt as much as possible.
Fashions have no place in development, and alas, many developers behave either like fashionatas, or like frustrated show-offs.
That's why good developers are rare... And so is good code.
I prefer not using `var` in situations where it's not obvious what the type is, so that `new string(input.Reverse().ToArray())` line is hard for me to code review. I had to go into my editor to figure out what the returned types were to see if it was needed. For a moment, I thought `input.Reverse()` might have returned a string, making everything else pointless, but it returns `IEnumerable`. I much prefer the top version because I know what's happening and the types being used.
Also, I prefer more lines of code rather than several statements merged into 1 line for a number of reasons:
- I can put breakpoints on separate lines and inspect the values of variables much more easily.
- The exception/stacktrace information reported from log error messages is much more exact and therefore much easier to fix bugs because the line number would point to only 1 or a couple of operations instead of several on the line at fault; In the original example, if the stacktrace suggested that the `new string(input.Reverse().ToArray())` line was at fault then was it `input.Reverse()` causing the issue, or `ToArray()`?
- Keeping a large number of operations on a single line instead of breaking them up into multiple lines is the equivalent of someone forgetting to use any fullstop in a huge paragraph versus using many to break up the pacing; it's just much easier to read and understand.
EDIT: Got to the end of the video and yea, use string.Concat for readability and simplicity, or the fast version for memory optimisation scenarios.
Honestly, I'd just put the fast approach into an extension method, and then keep string reversing logic away from the business logic. When you're working on the extension method, I don't mind if it's not "clean", its a single purpose function that you already know the intent of when you begin looking at it.
When Martin Fowler lists the Refactoring techniques in his book, each technique is always coupled with another as its opposite, e.g. Extract and In-line. What is shown here is an example of In-line expression refactoring, and if doing the opposite, it is the Extract. As Refactoring's purpose is to make code design simpler in various aspects (flexibility, readability, debug-ability, etc.), performing a technique itself, e.g. In-line here, does not means serving that purpose without the context and scenario it is applied to. So I full-heartedly agree with Nick when pointing out the Linked-in post, as the post's author seemingly assume In-line refactoring into 1 line is a silver bullet for simplicity. Personally, I enjoy chaining methods in Functional paradigm since it can lead to nice readability, but I don't want to limit within that, and when opportunities present, will still perform Extract of the chaining later either for debug-ability or reusability.
P.s. I personally think the author's in-line/chaining still does not improve much of readability while sacrificing debug-ability in vain. I would prefer sth like "return input.Reverse().ToArray().ToStringFromCharArray();", not "return new string(input.Reverse().ToArray());". If you mean to chain, at least chain all the way, not putting the chain as a method or a constructor's parameter.
I like less lines of code because you can fit more on the screen. For the reverse string array, both implementations are fine imo, but I would personally strive for the second option
For readability, I would prefer chained code (broken into lines where necessary) - I just see that it is a single expression, the code flow is simple.
Properly named methods in one chain can "tell the story" clearly enough.
Absence of throw-away names means the remaining named variables are important.
I can give names to intermediate results even if they are used once - when descriptive names are really important there.
(But I regret my style choices sometimes when debugging - have to get back and break expressions into statements to step through them...)
The example in the video has one thing I can frown upon though - expression doesn't read from left to right - it ends with a constructor that is on the left. In such case I may also break it into nicer parts or check if introducing an extension method would be justified...
But the benchmark is the most surprising part of this video. It raises more questions about how well the compiler actually optimized.
I don't want to be questioning code style for such performance differences.
I think in this very specific case you are writing a method that you'll write once and never look at again. The task is a simple one so it is unlikely to ever need debugged. Since this is something that could easily be used all over your code base, I think the preference should be for optimized code.
The first method is much cleaner than the second and the Nicks version. The reason is because everyone is used / has faced this version so it more readable.
In this example, I think the oneliner is way easier to read because it reads like english. you have input -> reverse it -> put it in a new array.
In the realm of software development, YKD principles - YAGNI, KISS, and DRY - lay the groundwork for clean code.
Beyond these, the art of great coding is achieved by minimizing cognitive load, enhancing readability, and simplifying the process of debugging and testing.
Great coding isn't just about writing the fewest lines or creating the perfect YKD example; it's about ensuring the maintainability of that code, making it not only functional but also enduring and comprehensible.
Great job that you anonymize these posts Nick.
For this particular example, I would mind the single line.
Let's abstract the problem, meaning what would we prefer, a loaded line of code, a well readable multi-line code or an optimized one that's harder to understand. I prefer function over form, so I'd pick the performant one, but only if it is relevant. The other 2 I don't care about - it's a question of execution (how it's written) to tell which is better. If it's something more complex, I prefer to use multiple lines as to make it more understandable. If it's something trivial, I prefer one-liners.
I prefer the "good" example in this case because it's simple and the code reads semantically left to right. Reading it makes me think "this method return a string, okay, that is the input string, okay, but reversed, okay, and converted to a char array, oh because that's how the string constructor works". It communicates that the intermediate states of the string are not important to understand the intended result. The version with multiple assignments, to me, is harder to read since I see "oh, you've created an array, why? is that important, do you need it later?" - this "clean code" tip seems to be "don't define temporaries if you only use them in the next line, and that's the only time you'll use them" which I _mostly_ agree with (and I know the actual assignment will be optimized but the way you understand/debug the code isn't optimized).
I take the point regarding the "bad" one being easier to debug, but the "good" one wouldn't be _hard_ to debug, would it?
Obviously for more complicated examples (especially if the temporaries are being passed as function arguments in a one-liner instead of method-chaining like this) defining temporaries can be clearer, but not in this case, in my opinion.
Using swap is the fastest way, swap head and tail.
(Creating Span with stackalloc to handle it)
And the only way to beat up above code is unsafe pointer.
I don't know if this makes me the odd one out. But in the first example, I tend to prefer code that reads like a sentence. I like having single line ifs that don't break the flow, and I like not having to name useless intermediate variables.
So when I read that a function returns a new string that's constructed from input that was reversed and turned into array, I find that better than reading three unconnected lines of code.
The second one is more functional (as in the paradigm). Some people prefer functional programming but I'm more of a procedural guy so the first one looks better to my eyes. I think it comes down to your preferred paradigm.
In general, here I'd vote for the "fast" implementation, even despite it is not best readable, because the speed is what we really want from such utility functions.
Speed is only one of the things we want. We also want the code to be correct. Math.Add can always return the wrong answer very fast if it doesn't bother to actually add.
We also want the code to be maintained by a team of humans who need to be able to comprehend it.
@@7th_CAV_Trooper Yes, that's correct. We of course want the code to be correct. But given it is correct, next things we typically want are speed and low memory usage, preferably, zero memory usage.
On the first example, I much prefer the “good,” compared to the bad. I find going through one line much easier, reads like a simple instruction.
“It returns a string, which is the result of reversing the input and turning it into a new array.”
I write one-line "better KISS" code because My team lead angry to me if i don't write single line code in such cases and Code Review rejected.
The OO calisthenics do not talk about one dot per line. They talk about not going deeper within the class(module) with the idea of following the Law of Demeter. In that first example (reverse string), what happens is chaining methods, and is completely fine.
There is two extra reasons to separate lines: Step by Step debug, and exception stack trace with code line of error.
Sure the IDE helps, but is easier to debug if its separated. I use "one logical operation by line" guide line.
If performance is not super important, I always go for readability and maintainability. I don't like when multiple steps are combined just to be 1 row instead of 3. It's also way harder to debug, you will end up separating the steps anyway to see what's happening.
On top of the above there will be probably more than one person trying to figure out what the author had on mind writing the code. Main question is the purpose of the code - does it need to be i.e. low latency or some performance trade off can be done, in which case easy to follow and understand is the way to go
Found it funny that the "bad" approach was much faster
Not surprising, Linq and Enumerable is rarely the fastest since its quite a lot of overhead.
It can make for easy to read code (if you stay away from long one liners) but compared to arrays its very much slower.
Rather than avoid multiple dots per line, I find the greatest readability improvement in avoiding nested brackets (of any type) in a single line, since in order to read that code you need to go back and forth which is generally more difficult.
Extracting variables like this is also a good test of code understanding, since if you struggle to find a good name for an intermediate variable, there may be an issue with the approach.
Exactly, while you can read the code left to right to understand it, it doesn't actually evaluate in that order with that nesting. I don't mind more than one dot per line if it's an extension method for mapping/casting/conversion or using a builder pattern, but once you start evaluating out of order it should be broken up. Honestly, I wouldn't call the single line in this example "bad" as I don't have the luxury of being that nitpicky in a code-review, I just take actual issue with it being called better.
@@exmello Yeah exactly. The situation in the example is just making a string out of a character array which is not a huge issue and I would let it slide, but imagine if it was a more complex passing of an argument into a function or constructor.
if you have a lot of vars as middle steps, it will be harder to read. One liner is easier to read as you can literally read it as a sentence of a LINQ style query. Pointed debugging steps is a very rare case overall. If you need each step in debug - two automatic var extract refactors and you'll get it.
Sometimes in the code simple chain operations are so much disintegrated to pieces with vars and names so it's very hard to read. Less code - less to read. Even make it a => instead of {}
I like the single line of coffee, except that I usually break it up on a new line for every new function call.
input
.Reverse()
.ToArray()
Static analyzers usually give pretty bad advice on method chaining random stuff together. Mathematically putting everything in 1 chained line gives you a lower Cyclomatic complexity - so some people just blindly assume that is actually better
Multiple dots style leads to somewhat shorter code. I'd use this approach, however with formatting that puts every dot on a separate line. Also can use in a single line, when the code is likely to be "write once and forget".
In personal projects I would use the single-line approach bc I like it and understands it, but that's only personal choice, in my work/real world I will use the first one
I don't mind chained method calls when it's just two methods as shown in the example in the video. So in this example in particular, the "good" code is indeed more readable for me than the "bad" code. If there are more chained calls, then I would break them down into multiple lines, like people usually do when they are using Linq. But I would still not create variables for intermediate states, unless they could somehow improve readability of the code.
This is all about style though. People who like functional programming probably favor chained method calls, whereas people who prefer procedural programming favor creating variables and calling functions in a step-by-step manner. There is nothing wrong with either approach, and I wouldn't block a pull request in a code review if someone were to favor one style over the other.
What bothered me in this code is that I immediately noticed their performance would be terrible. And your benchmarks proved me right. Now that would be a good reason to block a pull request.
The reason it's hard to read is that you need to jump back and forth horizontally to follow the data.
As long as the data flows in one direction, left or right, then chaining is not complicating anything, IMO
I prefer conditional extraction too. Sure, it's another line of code (or more), but at least it's more readable, especially if you invert the negative conditionals to be positive conditionals. It's along these same lines.
For me, the main reason i hate these 'one-liners', is not just it generally much harder to read, 'debugability/debug experience'. When you have a one-liner like that, debugging that is pain, i often had to separate them to individual lines to find what the exact problem is.
I'm okay with more than one dot per line, as long as that's the only thing happening in the line. For instance, I'm okay with:
return input.Reverse().ToArray().toString();
[I'm not sure if there is such a method, I'm not from the .NET bubble -- I just love your content!]
I try to avoid evaluating complex expressions as method parameters. The nesting leads to poor readability and debugging. Splitting into separate lines fixes those issues and is more logically coherent: each line has a much more singular purpose and is read+executed in stepwise order before the method call is made.
I tend to avoid creating unnecessary variables. This is conditioning from the old adage, "number of bugs is proportional to the number of variables". However, I now think that that adage is actually a result of having functions doing too much in first place.
For some reason, the ML Java / Apache Spark community is obsessed with creating massive one-liners in their code. It’s like some kind of badge of honor to create barely readable scripts to do simple tasks.
The LinkedIn one-liner is still more readable than the first approach, though. But, since ReverseString is a common utility function that's going to be written once and used all over the codebase, aesthetics and readability don't really matter and you should focus on performance.
A friend of mine is obsessed with one-lining everything in python but only does it for shits and giggles (e.g.: he took a ~250 line program I wrote during a hackathon and compressed it down into one line just to demonstrate to me what weird shit you can do in python). He'd never actually commit any of that to their codebase
@@jiebaef you can write entire programs in a single line in almost every language, it's not really specific to python
Line through screen, line not worth.
When I just started, I was able to compress all sorts of extensive calculations and comparisons into a single line of code. Until I found out that after half I year it took me too much time to decipher what exactly it did, so I started breaking it down into comments.
Now I'm experienced I just use some more lines just to make things easier readable. The compiler's optimizer will do the rest for me.
When I need to jump into an unknown project to do something, the simple code with more lines, fewer files and classes are always easier to navigate and I always appreciete them the most. The ones with the smart one liners and / or piles of abstractions and files just gives me a headache. When I need to fix something asap, I couldn't care less how smart the person who wrote the code was I just want to do the thing I was supposed to do, not solve some wierd code riddles 😂 I get it though, you do some thing and then realize you can do it in another way and then you realize you can do it even more smart etc. Feels good so you tell yourself it's "clean" but there is no such thing, it's just opinion. It should be about: does the code solve the real problem in a satisfactory way? Does the newest person understand it? If those are yes, then that's good code.
input.Reverse().toArray() - reverses the input and then converts it to an array. Does what it says on the tin, in one line. What am I missing? Who gets confused by this? I'd have to look at the first one for longer to work out what it's doing, the second one reads almost like English.
The first one, we have to know that charArray is mutable, and that the action performed by Array.Reverse affects it in place (and also, we now have to have some idea of what the Array class is and what its static methods do). Also, we've introduced the concept of a CharArray, which we didn't need to know about when just calling the Reverse() method on a string.
That said it's weird that C#'s Reverse method doesn't just return another string - Kotlin just does "My String".reversed(), and then none of this would even matter.
I always go for performance. If the ugliest code ever is faster, I'll use it.
I work processing Gigas of data every morning, and speed is everything to my clients so they have their data available in time.
Of course, nothing is carved in stone here, and you need to check your situation before deciding.
I love this example of reversing a string. It made me think... The two functions dont do the same thing even though they return the same thing. Array.Reverse() in the first function is not the same as String.Reverse() in the second function.
In this instance, i prefer the bad one. But I believe that the second function simply could be returning input.Reverse() and skip the .ToArray and the new string() parts. 😊
Fewer variables is simpler code. A single line with multiple dots is fine, as it reduces considerations for scoping, closures, etc.
04:19 - next time I recommend using Rider's introduce local refactoring action, you can do it in very few keystrokes, its faster, simpler, less error prone.
I tend to use expression body functions to signal that it's a "pure" function (i.e. no side effects or in-memory replacements, etc.). I find expressions easier to understand because I know that the output follows naturally from the input. I don't need to remember that the contents of a variable or an array have changed, adding cognitive load.
So yeah, I like the ReverseString_Nick version the best but I'd use an expression body: string ReverseString_Better(string input) => string.Concat(input.Reverse());
To handle unicode combine characters correctly, you'll have to use StringInfo.GetTextElementEnumerator:
IEnumerable ToTextElements(string source)
{
var enumerator = StringInfo.GetTextElementEnumerator(source);
while (enumerator.MoveNext())
{
yield return enumerator.GetTextElement();
}
}
string StringReverse_Best(string input) => string.Concat(ToTextElements(input).Reverse());
Very clean code.
var a = b ? c : d ? e : f ? g : h;
Not possible to read, will forget what it is doing in 2 days. Code must be readable, not small.
Code readability is always appreciated rather than a chained single line code with a justification of "its more optimised" with no actual noticeable difference 😂
I prefer the first option. The second option looks like a more elegant solution BUT in case if I need to debug or add more specific logs in that code. The first one solution is better and it is easier to read. One action, one line.
Using one-liner methods isn't always the optimal approach. It doesn't necessarily align with the KISS principle. At times, splitting code across multiple lines can be more beneficial. Debugging becomes simpler when examining log files. For example, if an exception occurs on line 36 and that line features multiple chained methods, determining the exact location of the exception can be challenging without running the app with a breakpoint. Simplicity doesn't always mean brevity ;-)
A lot of people equate less lines with simplicity but that often leads to these issues.
You're free to make the entire program one line if so wish but nobody thinks that's a good idea (JS minifiers excluded!)
For the exact same reasons, it's probably not a good idea for your functions either
Entire program one line... This makes me think of a book with no punctuation, no paragraphs, no whitespace, just a series of letters. :)
Obviousness is what's important. I prefer compactness when it's obvious what the compact line does as a chunk. Whereas I would keep less-related steps in separate lines, with maybe two transportations.
The chained, compact style was common in Ruby coding a while ago. It's a common preference.
Keep it simple != Less character to right
A code MUST be reable like a book with space, short sentence, one idea by paragraphe, argument align and wrap ... When you look at the code you should be able to identify directly the role of each element without thinking
Depends on your definition of "Clean Code". For me it means readable/maintainable code. In this trivial example I'd not be worried about either of those. In general though I consider expanded version "cleaner".
I think the shorter version is easier to read because I dont have to keep track of what has happened to chararray before it is used in the string constructor.
In the first example after reversing the string what’s the point in using the array & string functions?
What ?!? There is a solution about string manipulation where Nick doesn't use span ?!?
Don't worry. The fast solution uses span!
It doesn't have any sense. Like... we're creating the new instance of string, and we definitely SHOULD NOT manipulate the values of original string while using span. But If this comment is a simple joke, I have no objection :)
Except that he did use spans, the function takes a span of char, the string is just automatically being cast to a span. :)
@@winchester2581 The span just servers as a fast reading technique. The modified values are on a different address. So it makes sense to use spans. And it does use spans under the hood.
In the end, this is just a matter of aesthetic prefference really. The performance is the same. Sometimes I'd use linq, or the "=>" arrows to make things less jarring for the eye, as long as readability is maintained. The first example is easier to read in my opinion. Minus points to the second example for not using lambda expressions. Neither are really a "good" or "bad" example, just a matter of prefference. If the method was one which had more potential points of failure, I'd rather have it more seperated, if it's just doing basic logic, I'd love the method to take as little space as possible.
The ReverseString_Good is fine to me and what I would do, but that’s based on how I came to coding. Chaining functions isn’t really difficult for me to parse. It easily reads as “take a string, reverse it, and give it to me as an array, then make a new string out of it”.
That’s not to say the “bad” approach is something you shouldn’t do. Everything except the fast one is pretty much equally easy to read for me. I’ll admit, I’m a bit surprised that the “bad” code is so fast compared to the one-liner, considering how similar they are conceptually. But that level of performance difference isn’t necessary for my use cases.
I don't see the dot chaining as a problem, that portion is very readable to me. Each dot represents a sequential step in a nice clean predictable sequence.
Taking that dot-chained expression and making it a parameter of the string constructor is where it becomes less readable. It's not the method chain that's the readability problem, nor is it the parameter, but nesting one inside the other.
If I had been handed the the problem and asked to make it "clean", I would have probable wound up with:
public string ReverseString(string input)
{
var reversed = input.Reverse().ToCharArray();
return new string(reversed);
}
The method chain on the first body line is nicely readable, representing a simple sequence.
The return line with a string constructor is also pretty straightforward.
This is assuming we don't need to deal with any culture/locale considerations like accent marks. In a real world situation, I'd probably be using a string enumerator to break into a collection of grapheme clusters, not chars, which would be less readable but more robust unless I could absolutely guarantee that it isn't needed.
l mean if you already mushing it all together in ONE return statement you really have to loose the curly braces and do a "=>" construct! Just one line compared to 4 MUST be less code! 😉😇🤪
The one line code is a reading nightmare, hard to debug and is ugly! I'm much more into readability than shortness! Extreme performance reasons excluded...
clever one line linq expressions can be a pain to debug (unless you're using something like the ozcode extension). when it is not something extremely simple like here I would prefer the multi step version.
This is like having a table with many different dishes and yet trying to compare 4 potatoes to each other.
I’d likely put an extension in play to make it more readable, e.g. input.Reverse().AsString(). It’s readable to me, but i wouldn’t complain if it were the “Bad” in the code base. Maintainability is more important than my ego.
The compiler is going to optimise both of them to basically the same funcgion anyway, the "wasted space" is only for the dev reading.
I find the single-line version better.
In my opinion, it does not really change the readability of the code.
Also reversing a string is a simple and straightforward process. If I was doing something complicated where I would need to be extra careful or had to constantly debug it then I would use the longer format.
It actually took me less time to understand the shorter function than the first because in the first one, I had to keep all the variables and context in my head, whereas on the second one i could just easily see that the string is reversed, then copied. Adding the extra lines doesn't help because it is such a simple function that adding lines just obfuscates it a little bit
I would write a .ToString() method (maybe with a different name) that works on char arrays and then I could write => input.Reverse().ToArray().ToString(); if you wrote one for enumerables of char, it could become => input.Reverse.ToString();
I am very bad about writing 3 line of code that replaces 300 lines. It’s not a good practice, as it’s inpossible to debug or maintain but, for me, it’s an optimization step at the end of my method creation. I generally keep the 300 line version (commented out), for the support team.
Once, a friend told me, 'I improved your code.' He rewrote my code into a single line. His lack of experience didn't allow him to realize that I had to debug that code to provide him with tested code
I don’t like a return statement that is doing work as part of the return statement. When Eve an exception might be thrown, I want to know exactly what part of the logic is throwing the exception. So if you do it all in one line, it isn’t simple to debug any problems in the future.
Excellent video, these were my thoughts exactly! People make these wrong arguments about clean code all the time, I used to feel totally weirded out!!!
Yeah, I disagree with this take quite strongly, at least for *this* example. I would even go one step further, get rid of the braces and make it into an expression-bodied method (on two lines). The second version has significantly less characters and reads *perfectly* from left-to-right. Reading one line from left-to-right is obviously much easier than reading left-to-right then up-to-down and repeating that multiple times. And if we're talking about a 200 line file vs a 600 line file, where the later expands as much as possible into multiple lines, the former is far easier to navigate and take in at a hundred foot level.
Now, I did say "at least for this example." Compacting everything into a single line is not always the best choice. We always need to be pragmatic and avoid inflexible rules because there are always edge cases. At the extreme, one-liners can become like run-on sentences, and it's not always possible to read a line perfectly from left-to-right. You might, for example, have to jump between sets of parentheses to decode the meaning, and that's not good. This example however, is an exemplar for when you, as a pragmatic programmer, should go for the one-line version.
in terms of readability, the "good", "bad", and "nick" seem close enough to not matter. if I saw either of these in a PR, I don't think I would question either one. (my bigger question would probably be "they needed to reverse a string? what are they doing in here??")
if I saw the "fast" code, I'd probably be like "wtf is all this??" unless there was a comment, or other context, making it clear "this is a hot path! do not refactor without performance testing!"
I prefer one liners for quick prototyping, but at some point you'll make an extension method out of such code and that's your readability. What's happening inside the extension can be optimized and the code doesn't matter, since you know it's some "Reverse_something".
For me it's not so much about one-liners vs many-liners, but if the code is self-explanatory. Sure you can have cool one-liners with several functions that do different things, but if it's not apparant what each function does you'd need comments (and is it really a one-liner when you need to comment?). If it's hard to understand what the code does, splitting it up into multiple lines with intermediate variables can be helpful in understanding what it does, and you can easier step through if it needs debugging.
On the other hand you have performance critical code, where you need to use complex functions or get into unsafe territory. Here it's not so much of clean code or one-liners (though you can still try), but optimize it for speed or memory footprint however you need. These can quickly be complex and forcing clean code can sometimes be the opposite of what you need your code to be. Here it might be better to just have more lines, and even comment sections of it to make it clearer what it does.
Also, the compilers are really great at optimizing. Code you write as "clean code" might perform worse (and sometimes way worse) compared to "unclean code".
This is a big subject that touches a lot on locality of concerns and the many different contexts that humans need to interact within.
Focusing specifically on the first example, I'll add: Mixing confluent dot syntax and function application syntax on the same line can be jarring when the order of operations is important because they read in different directions. Record accessors read left-to-right, and function application reads applied right-to-left (and inside-out) on the same line.
Generally mixing styles on a whim *is* jarring, so in cases where this is warranted (there are many), perhaps preparing the audience (and being a good audience) is wise.
i dont know about the concept, but this code example specifically is very readable to me