Download source code ► / zoranhorvat
Join Discord server with topics on C# ► codinghelmet.com/go/discord
Enroll course Beginning Object-Oriented Programming with C# ► codinghelmet.com/go/beginning...
Subscribe ► / @zoran-horvat
Deep nesting is one of the principal sources of confusion, as well as bugs in code. This video shows one method with several nesting levels and many design issues.
One step at a time, we shall refactor that method into a sequence of one-liner calls, each subordinate method with no nesting at all!
The only nesting remaining at the end of the demo will be the one required to validate the input.
▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬
👨 About Me 👨
Hi, I’m Zoran, I have more than 20 years of experience as a software developer, architect, team lead, and more. I have been programming in C# since its inception in the early 2000s. Since 2017 I have started publishing professional video courses at Pluralsight and Udemy and by this point, there are over 100 hours of the highest-quality videos you can watch on those platforms. On my KZhead channel, you can find shorter video forms focused on clarifying practical issues in coding, design, and architecture of .NET applications.❤️
▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬
⚡️COPYRIGHT NOTICE:
The Copyright Laws of the United States recognize a “fair use” of copyrighted content. Section 107 of the U.S. Copyright Act states: “Notwithstanding the provisions of sections 106 and 106A, the fair use of a copyrighted work, including such use by reproduction in copies or phono records or by any other means specified by that section, for purposes such as criticism, comment, news reporting, teaching (including multiple copies for classroom use), scholarship, or research, is not an infringement of copyright." This video and our youtube channel, in general, may contain certain copyrighted works that were not specifically authorized to be used by the copyright holder(s), but which we believe in good faith are protected by federal law and the Fair use doctrine for one or more of the reasons noted above.
#csharp #dotnet #objectorientedprogramming
I switched to functional style of coding, while ago, after reading of "FP for rest of us". Book is dedicated to Scala, but same conceptions can be used in C#, especially with the latest versions of the language, where we observe adding of more & more functional features to the language. Functional code is less verbose, less prone to errors, more self-explanatory and more pleasant to read. My 2 cents, if you want to adopt the functional style, before coding always first ask yourself - can I simply describe the desired properties(characteristics) of the final result ? In most cases answer is yes, then you don't implement algorithm step by step, but just express what you need and let it be framework's problem how to achieve it.
I would not suggest getting rid of the trimmed hint ahead. The Trim function nested inside the lambda gets unneccessary repeatedly called at worst N-times for each item.
Great video, I always try to apply similar approaches. If the linq cannot be applied, you can easily start moving chunks of code that has ifs or loops into a separate methods, till your main method which you tried to refactor does not have more then one nesting. I remember this since I watched your video on refactoring 1000-line method into cleaner code. Its an old one but I still use it as a great example on showing other devs why I hate nesting.
Thank you, Zoran! As usual your refactoring brings a simple and elegant solution!
Pretty dumb solution
Strange that this "code smell" is still prevalent and requires your excellent commentary given the level of tool support we have these days. Way back at the beginning of my career in RTS this type of code was frowned upon for many reasons. As I approach retirement, I still wonder at the state of software development and its focus on current technology and not choosing the right tool, pattern or algorithm for the job. I lament the amount of times I have to provide a new technology solution to a problem that was solved decades previously. However, when we still must confront this same old same old, your videos provide excellent food for thought - well done Zoran.
I know exactly what you are saying. My way of dealing with that problem was to quit working for corporations, start working with small teams and, eventually, to start making video courses and videos like this in an attempt to draw attention to the problem and make new programmers start noticing it.
Some time in the 1990s I was told a function should have only one way in and one way out. I wrote so much complex and nested code trying to obey this stupid rule. The advice in this video is better in my opinion. Smaller methods make returning from the middle less problematic.
@@Robert-yw5ms yes, but it does so for the right reason, because it does not need multiple exits. If a method is short and well structured I very much prefer multiple exits over temp variables or similar and a set of guard clauses in the beginning is not a problem, especially if all of them guard a very short code snippet. Its easy to find all the conditions in one place, I would not break out guard clauses in many cases since keeping them within the method ensures no one tries to reuse the inner most method without them. If you break out the guard clauses and do not hide away the inner most method you could have a case where someone reuse the logic without guards and cause a bug or security risk.
@@Robert-yw5ms I was not objecting, just adding my 5 cents. The way is often quite important also, not only the how :)
Pascal has one way out. C has many. Your advisor was stack in Pascal
@@1Eagler I had forgotten that Pascal was one way out, but I heard this one way in one way out thing from Assembly jockeys.
@@1Eagler I never heard of anyone using Pascal until Delphi was released.
0:55 I was getting really annoyed by the else block until 6:30 when you changed the code to a guard clause. Great explaination of the process of getting from start to finish. Always a joy to watch your videos!
If I had to select only one improvement, it would almost always be the guard clause. It is so cheap and elegant, and its result is instantly visible.
Awesome tip and perfect refactoring: 10/10
Thanks for sharing Zoran
Great content , keep going
Before I watched this video, if I went through what you have here, I would have the hint as the 2nd parameter of the extracted methods... But now seeing it as the 1st parameter is quite mesmerising
I see one major problem with the end result, Most built in TryGet methods in C# returns a boolean for if it succeeded or failed not a nullable value, the return value is an out param. I do not think one should deviate from that pattern since it reduce readability in my mind, if I see TryGet I will assume it follows the standard that is present in Dictionaries and similar.
Yeah, "Try" preceding a method has always meant to me a method that returns a bool indicating whether or not an operation succeeded
That pattern is ages old and I think we should finally deviate from it. C# alone is deviating from out parameters anyway, making that convention dubious at least, if not outright obsolete. There were other conventions that have been abandoned in the past, such as adding Base to abstract classes. Conventions are not cast in stone.
@@zoran-horvat Could be but in any existing code base you are likely to find hundreds or thousands of usage so I still feel it will add confusion. In a new clean project sure, but as long as NET still use the pattern in many cases I think we should avoid confusion so in this case do not use the tryget naming. I also try to limit the use of null so a Maybe pattern would be a better and skipping the Try prefix.
@@davidmartensson273I don't like the try naming convention he used, but since there is a find naming pattern it is okay to return null. Only get naming pattern should never return null
@@zoran-horvat I don't think it's an obsolete pattern. What the "Try" function returns, and whether it succeeded, can mean vastly different things. A Try function can succeed, but return nothing. And the conditional that tests if the Try succeeded may need to do things unrelated to whatever was returned. For example, TryProcessElements() can return the elements it processed - or maybe the list of elements it _didn't_ process. That return list might be empty. If you get rid of the boolean return, then you have to return a nullable in order to distinguish between success and failure, and that complicates later logic by requiring a null check. The Try format also makes it very easy to put the call into an if() statement, making reading and understanding much easier than having to do null checks. Doing the null check (or HasValue, for Nullable types) requires two steps to both acquire the value and check whether it succeeded, whereas the bool syntax can be done in one step with inlined variable declarations. That said, I think the _TryFind_ you're doing in the video is the closest to not needing the bool return. Unlike, say, Parse() vs TryParse(), there's no need to throw exceptions if the function fails, which means you don't have to build your normal processing logic around exception handling. (Note: It's possible that the Parse() functions might have returned Nullable types if they had been available in early C#, so the exception side of things is a weaker argument in modern settings.) But in general, the bool return style for 'Try' functions facilitates function composition and simplified caller logic much more readily than nullable value returns.
Perfect example
Props for the video! Inverting IF statements can also be useful but of course that was not the case in your example. Having many methods also makes debugging a little bit jumpy but it depends on person.
Great video
Zoran I found one of your Pluralsight courses many years ago and have been watching your stuff ever since. I love your approach to code. I wish you could come into my work and do this on some of our stuff! It's so bad. Can you find a "real" example and refactor? Perhaps an API with bloated controllers?
Sometimes I do refactoring on my bookstore application in other videos, but that is limited.
Personally, I think you hit a sweet spot at around 6:30 minutes where the code was the most easily readable and understandable. If the goal is to make the code understandable at a glance and to reduce the need for a developer to 'think' then the final refactor, for me at least, seemed unnecessary. Sure, it made everything more elegant and following the same expression body pattern, but at the cost of readability. Not a criticism, just a personal preference. :)
That is fine. Guard clause is a good choice as it invariably cuts nesting at almost no cost. The next step is into pattern matching. That technique requires getting used to, but once you get used to it, guard clause would look like a half-baked product. I am always making that additional step in my videos, as I want more programmers to make that leap. BTW, C# is introducing more and more of the pattern matching syntax for this very reason.
I think that programming must be an easly reading of a "step by step acitons" and we can see it here when you use clear names for each action. It take a while to learn it, but worth every single effort to acoplish it. Also, it's a constant exercise, improving every single day. I suggest to ask a friend to take a look: an amazing way to learn different approaches (this is also the reason I'm here)
lovely lovely video tjanks
This is looking at a minor version of the client base I'm currently working on. I already converted over a C++ CLI service although I haven't finished the refactoring. My goal with the long term contract is to rewrite code from the ground up for a web form site because we have one file alone which is 12k lines of code.
Even though I much prefer to code in F#, I very much enjoy your videos, and have noted that they help make my occasional forays into C# less tiresome.
And with linq and good structure, C# is edging closer and closer :)
C# today supports many techniques that were limited to functional languages in the past. Pattern matching expressions in C# are an excellent example.
Is the Trim() function now running repeatedly on every check in linq, creating more garbage to clean up, and taking more time?
I had to write a test to find out. In debug mode, a breakpoint is hit on every call to Trim. I don't know if LINQ will optimize that away in release mode, but I doubt it. You could create a Hint value type with implicit cast operators (to and from string) that trimmed the string for you and use the Hint type as the argument to the TryGetBook method.
I do like it, but do not think I would have gone that far in production code. One objection I have is that hint is now trimmed multiple times. Another one is that one-liner method looks weird after a few months from now. If you want to know what a match is really about, you have to go through a few one-liners and make a mental model of it in your head. Also debugging will be a lot harder 😅 But maybe now the code is bugfree and less error prone, debugging will not be needed at all 😝
How much pain did writing the deep nested version cause you ? It hurts just looking at it.
Well, I had to make two passes through the video cutting long portions of it to make it look short. If I left it the way it was, 2/3 of the video would be me struggling with the nested version.
Good luck debugging that 😂
You didn't know? Pattern matching expressions are trivial to test, unlike methods with loops and branching.
I'm gonna bet money he writes tests so he doesn't have to debug like a junior dev. But I'm curious why you think one-line functions with single responsibility could be hard to debug or reason about?
nice. i'd like to see also some benchmarking, comparing nested and fancy methods. how big difference in time between them? because many people (and me) thinking about performance cost on methods call and linq. so, some people may say to clean code that much is pointless. and, it may be ugly, but it easy to understand nested method to unexperienced person.
That makes sense, and I already made some videos comparing performance. You can find them on my channel. The most striking result of doing measurement, though, is in noticing that, whatever you did with or without LINQ, it would remain at least two orders of magnitude, if not three, faster than the fastest access to a database. In other words, if you are using a database, forget about optimizing the loops and ifs.
These refactoring bullshits are much slower as well as the linq that you have to have good knowledge what it does under the hood
@@dr.angerous Sounds like you love low-level optimizations but don't fancy learning the language. So much about bullshit.
@@zoran-horvat I actually care and yes I do. Functional programming is a mess, as is in your example that works for this little one, but imagine real world scenario with caching and some Polly retry policies and so on. Your delusional here.
@@dr.angerous Oh, give me more of that "functional programming is a mess" stuff...
As always, I have learned something! I would have done it like this: Book? TryFindBook(IEnumerable books, string hint) => string.IsNullOrWhiteSpace(hint) ? null : books.FirstOrDefault(book => book.Authors.Select(author => author.FirstName) .Union(book.Authors.Select(author => author.LastName)) .Append(book.Title) .Any(value => value.Contains(hint, StringComparison.OrdinalIgnoreCase))); But I don't think it's as readable as your approach, and something about the code above feels a bit funky. Many thanks for the video, you have single handily inspired me over months to tackle problems from a functional approach, even if I have much yet to learn.
wow
What I like about this approach is that you can then easily test the individual methods. The downside is that you have to make them internal so that they're accessible in the test.
To what end? If you need to test internal methods you have likely created an untestable or hard to test design already
Don't test private or internal methods directly. Test the public method that calls the private/internal method, and change the parameters of the test to match what you need to test that method. This form of over testing is why so many developers hate writing tests. You should only be testing at the public entry points of your application. Any other testing is likely to be overkill, and a waste of your time.
Yes, you can test the public methods that indirectly test the private helpers, but I would argue its harder to come up with a combination that tests all combinations of the helpers. Take for example a class with a constructor that takes in a string. Where the string then is passed to private helpers to do some validation from the constructor. Without exposing the validation results, you can't test the validation is working as expected. You have to decouple the validation into another class that can be tested. Which is essentially what I meant in my original reply.
@@SirBenJamin_ you need to create those combination of parameters anyways. That is the only way you can prove the unit as a whole works. Sub-unit testing holds no value for verification purposes without verification that the unit as a whole works. You only need sub-unit testing when that sub-unit testing helps you move faster during development due to complexity around or in the sub-unit.
@@evancombs5159 I guess I agree and disagree. With the way you describe, I think you would end up with a silly amount of combinations just to make sure that all code paths and edge cases are tested. Making a hard to read test (and probably a lot of comments (e.g this is to test this werd edge case etc etc)). Where as if you test individual components, its much easier to understand what you're testing and what is going wrong. If a test fails, the area and the issue will be obvious. Where as in your case, you're going to have to start stepping through the code to see where its going wrong as it could be deep into the call stack.
Tell it to the Pascal programmer 😅
No joke, i know people that like the nested approach more because they can read it more clearly with nested code.
I know them, too. The purpose of this video is to make them change their mind and to stop draining life from everybody else.
But when it's a client that demands it, you adapt and move on.
@@turcanuioangeorge4750 Does the client write your code?
Not the current one. But had projects where the lead dev would approve deny code. We would do our due diligence pointing ups and downs of approaches but at the end of the day you can't win all battles.
@@turcanuioangeorge4750 I'd just leave my PR there and report the user story complete. It's their problem if they don't merge the PR.
Your first mistake was returning a nullable instead of an index.
What would you do with the index, and what index would you return when the book was not found?
@@zoran-horvat If you return an index (since you're Searching a list), you leave room to further computation or you can access the list at another point with virtually zero cost. If it's not found you could return -1. For example with an index you an grab check if the book is still available, if it is, you wasted 0 performance, since the book you grabbed at that index matches what you looked for. If it's changed or is not available, you can do the more expensive computation. Or if it's ordered you can just skip searching from the index.
@@chudchadanstud You assume it is a collection, while all you have is a sequence. Operating on an index assumes the element still exists after the sequence was iterated. Many sequences do not come with such a guarantee: deserializers, database queries, streams, etc. Some would execute twice. Others would throw. What does index 1 represent in: { yield return a; yield return b; }
@@chudchadanstud There is also a use case in which your idea causes an irreparable bug. If the operator was applied after a Where, then it would not observe the elements of the original sequence and its integer result would be wrong. Consider the FirstOrDefault operator - try to imagine it returning an index and you would see how wrong that idea is.
I have mixed feelings about creating functions just to separate behavior. I feel like sometimes these just hurt Locality of Behaviour. I'd rather have long functions with meaningful comments than a billion '1 reference' functions in my class.
Do you really comment your methods?
@@zoran-horvat rather have some comments then all functionality reduced to one liners. when looking at the first resulting function its not clear what the conditions are to find a book while in the original function it was clear at a single glance Reduction of everything into elements never works in more complicated projects without losing local meaning. Just imagine maintaining an old 200K line project filled with one liners like this..
In the example, these "one reference" functions are the result of language optimisation. Correct naming and positioning as well as the use of regions or partial classes will better organise the code. Furthermore, code organisation does allow higher-level refactorings to take place like duplicate code analysis.
@@zoran-horvat There are still software groups where documenting of methods using comments is still mandatory! Personally, I only comment when I get paid for it 🙂
@@unityasteroids1562 Just try and maintain comments as well as the code in a 200k line project then!
Sorry this is where you are completely wrong. I hate this type of code. It's not readable, methods are dumb and ugly. What a mess refactoring
Which line of code is not readable in the final version?
I can't sum up all my thoughts in a KZhead comment but I have been coding for over 20 years. There's good advice in this video but like everything think about the context to determine if something is a good fit for your situation. Realize why you might or might not do something a certain way and try not to over do "good advice" or force something to fit because you think that is the way "it should be".
I don't agree at all. The problem I see with this approach is one line methods are everywhere. In my codebase at work, we have created far too many one line methods or short methods that resemble a sort of domain specific language. In our case this has caused me many headaches over the years because you end up having to f12 all over the place to find definitions of things before you can make sense of the calls higher up the call stack. In addition you run the risk of having to change many more methods if those methods are tightly coupled. There are some places breaking things up makes sense but sometimes you are better off with a longer function that captures the logic in one readable place. If you are only referencing the function in one place, why do you need it in a function? I would recommend to anyone that you should keep the specific context in mind. If the logic changes in a significant way that forces you to modify several functions down the stack, you might be better off keeping it together. As a general rule, things that change together should live together. I have a nightmare codebase at work composed of far too many one line methods. It has a lot of problems when pushed too far.
Please note that in my code only one method should be public, with all others being private. In a large code base it doesn't matter how large a method is if you don't read its body. A hundred lines of code method is equivalent to a one-liner that passes the call to a private method.
I think the right balance is someplace between you and Zoran, but leaning more towards Zoran.
"If you are only referencing the function in one place, why do you need it in a function?" I think I can help answer this question: When a function (one liner or not) is being extracted like Zoran has done in the video, it is mainly for 2 purposes. The 1st one is to reuse the codes and behaviors, preventing them from being duplicated and keeping them consistent where they are being referenced. This purpose creates extracted functions with high number of references. The 2nd purpose is to encapsulate the layer of abstraction, which is the main reason for most of the extracted methods in this case. One of them actually serves both purposes (IsMatch). The nightmare codebase that you have seems to be the result of the author not being aware of the 2 purposes well. They may try to do extraction for the 2nd purpose, but do so aggressively where the extraction is not appropriate nor natural (e.g. IsDeleted(bool deleteFlag)). They may also set inappropriate access modifier of these methods (e.g. default as public), which make the class that holds them bloated in the eyes of the external users/codes. They may also do it for the 1st purpose, to reuse, but in the manner that "maybe later I can reuse it" and then not reuse it at all (which is YAGNI problem). As a recommended remedy for such codebase, you can consider first fixing the access modifier of these functions to be as minimal as they are allowed to (e.g. turn them to private, see where the compiler throws error, then slowly bring them up to protected, internal, or public). Be careful, that sometimes even 0 reference public method when turned private can still breaks the system (e.g. the method is not referenced by the codebase directly, but by codes using Reflection, by Template codes that are stored in database or files, or by an external system outside your codebase through API). Therefore, you have to understand the codebase well enough to fix the modifier. Next you can do the reverse refactor of Extract Method, which is Inline Method. If the method extracted does not seem to help any of the 2 purposes (reuse or create a natural layer of abstraction), inline the method's body back to where it is referenced. The lower the number of references for such method (ideally 1), the easier to perform this refactor. And if the method has 0 reference, don't bother to do the Inline refactor. Just delete it. And again, you must be sure the inlined method is not being used through Reflection, Template data, or an external actor via API.
@@petervo224 Thank you for assembling this detailed overview!
@@zoran-horvat Well... I am guilty for creating similar nightmare codebase in the past, so I guess this is the least I can do as redemption.