Refactoring in C# - Improving an Existing Application

Share
Embed
  • Published on Jul 9, 2018
  • Most of your development career will be spent fixing and improving existing code. That is called refactoring. In this video, I am going to walk you through a project that is badly in need of some refactoring. Along the way, I will show you some techniques for spotting areas that should be refactored. By the end of the demo, what was a jumbled mess of code should be cleaned up so that it is more readable and reusable.
    Source Code: leadmagnets.app/?Resource=RefactoringInCSharp
    Patreon: patreon.com/IAmTimCorey
    Newsletter signup: signup.iamtimcorey.com/

Comments • 67

  • queenstownswords
    queenstownswords 2 months ago

    Hello Tim.
    A few things I invite your comment on:
    1. having more than 10 lines in a method *could* be an indication to break up the methods.
    2. scrolling (as you noted) is a large flag to break up the method.
    3. doing unit tests seems to force the developer to think (and test) in small testable units.
    4. a unit test that takes over 4 lines to set up is a red flag that the tested method needs to be broken up.

    • queenstownswords
      queenstownswords 2 months ago

      @IAmTimCorey Thanks for getting back to me Tim. Do you have a video on mock-ing yet?

    • IAmTimCorey
      IAmTimCorey  2 months ago

      I mostly agree with these, although the last one isn't necessarily true. If you have to mock anything, it will be more than four lines of code to set up your unit test and most method depend on other classes, which means they will need mocking.

  • Nick Kusters
    Nick Kusters 2 months ago

    Why make the function explicitly private? That is nog the standaard for c# (it is for vb.net), the Class and Main are also private.

    • Nick Kusters
      Nick Kusters 2 months ago

      @IAmTimCorey do you then go out and change this for the namespace and class as well? It's like learning the difference between value types and reference types. IN C#, if you don't specify the accessibility, it's private. If you don't teach that, it won't prevent people from making that mistake.

    • IAmTimCorey
      IAmTimCorey  2 months ago

      I prefer not to have assumptions.

  • std661
    std661 2 months ago

    As a developer I also like to give the smallest set of operations for methods on the input parameters. The same applies for model classes. For example, if PayEmployee(List, EmployeeModel) method is not intended to modify the list, it is a bad practice to ask for a List in my opinion. There are many interfaces that can be used to reduce the number of available operations:
    1) IEnumerable - Use it if you want only to iterate through the sequence only once. Only once, because this allows LINQ queries to be executed multiple times.
    2) IReadOnlyCollection - Use it if you want to iterate through multiple times and/or you need to know how many elements are in this collection.
    3) IReadOnlyList - Use it in case of IReadOnlyCollection but you also want to access the elements with the indexer-property.
    4) ICollection - Similar to IReadOnlyCollection but allows to modify the collection and check if it contains a specific element.
    5) IList - Adds indexer-property, ability to insert item to specific index, to the ICollection.

    • IAmTimCorey
      IAmTimCorey  2 months ago

      I'm not sure I see the benefits you gain here, especially with the conversion from one list type to another being implicit in every call.

  • Jonathan Rigsby
    Jonathan Rigsby 2 months ago

    This was very satisfying video to watch

  • Salomon Frux
    Salomon Frux 6 months ago

    That was helpfull Tim, thanks a lot.

  • Mihow Bogucki
    Mihow Bogucki 6 months ago

    I found this very helpful and practical. Tim takes the time to explain what he is coding. He helps you think about what you are doing in the moment but helps you understand where you can take the code going forward.
    Thank you Tim for being such a great teacher! 👏🏼🙏🏼

    • IAmTimCorey
      IAmTimCorey  6 months ago

      Thanks for the kind words. I am glad you are finding the content so helpful.

  • Jelle Wijma
    Jelle Wijma 7 months ago

    He, man really like what you're doing. you're helping me a lot getting to know c# and or coding overall. thanks a lot.

    • IAmTimCorey
      IAmTimCorey  7 months ago

      You are most welcome. Thanks for watching.

  • Wayne VanWeerthuizen
    Wayne VanWeerthuizen 9 months ago

    Tim, would you consider producing a followup video on refactoring specifically in reference to XAML? What are possible ways a typical 2000 line long MainWindow.xaml file might be improved? I am not really clear how to break XAML into smaller parts the way we do in C#. I assume a solution might have much to do with making use of custom controls and resource libraries. But how would that be done in practice, specifically as a refactoring of existing code, while requiring only a minimum of extra boilerplate code and overhead? And are there any Visual Studio features or free extensions that help with this process?

    • IAmTimCorey
      IAmTimCorey  9 months ago

      I'll add it to the suggestion list. WPF code is a mess and there aren't great ways to make it better. You can make controls but then you have the issue of hunting all over to modify something.

  • Wayne VanWeerthuizen
    Wayne VanWeerthuizen 9 months ago

    Great video overall. Thank you for creating such a great channel.
    Even though I was already had some rudimentary familiarity with the idea of refactoring, this helped me better understand particular details of the process.
    I do wonder, though, why you never used Visual Studio's helpful "Extra Method" refactoring shortcut. I noticed another thing conspicuously absent from this video: most other books and articles talking about refactoring say to start by writing automated unit tests. I am not sure how one would do that for the particular console application you used as an example. But anyway I was hoping to see any good demonstration of the unit testing process being applied to existing code, which might be ugly. (And hopefully shown in a matter compatible with the features of VS 2017 Community Edition, that is, if its unit testing framework is more limited than the full version.)

    • IAmTimCorey
      IAmTimCorey  9 months ago +1

      I'm not entirely comfortable with Extract Method. I use it sometimes but too often I'll then forget that I created it and won't find out until I throw the NotImplementedException in my code. It also moves the method away from where I'm at, which is frustrating.

      As far as unit testing, that is something I would also do but I didn't want to mix those two together in one video. It makes it less clear, longer, and generally messier to communicate (although that pretty much replicates real-world development). I do have a couple videos on Unit Testing and I'll be implementing unit testing in my new course coming to TheXvid.

  • Wayne VanWeerthuizen
    Wayne VanWeerthuizen 9 months ago

    At 40:55-41:05 of the video Tim says, "You know it's in those couple things you changed."
    I wish that were true!! But, no. It is not that simple. Or at least with code that has failed to use sufficient defensive programming techniques, it isn't that simple.
    It is quite possible for changes only made to one part of the code to trigger bugs caused by bad code elsewhere! In fact, I had this happen to me just recently and it was annoyingly time consuming to debug.
    My code has a method that takes some time to generate a list of strings. The list would be consumed two ways: one portion of the program would use the list as part of some business logic, and another part of the program displayed the list in a listbox full of comboboxes within a WPF GUI. The later needed the comboboxes to add a blank item at the top of the list, so the blank line was added to the list within the getter of the property bound to the itemssource of comboboxes. At that point, after running the program it appeared everything was working okay.
    I then decided the method that generates the list should be improved so as to avoid unnecessary repeated generation of the exact same list when the method called several times in a row with the same arguments. So I then I proceed to modify just that method, AND NO OTHER PART OF THE CODE, so that it would check if the the arguments were the same as the last time it was called, and if so, just return the same list it returned before (stored in a static variable). At that point, my program started failing in more than one way. The business logic part of the code starting throwing exceptions. And in the GUI, the ComboBoxes now showed multiple blank items at the start of the list, instead of just a single blank item. That was no good!
    I took an hour trying to figure what could possibly be wrong with the code I changed and found nothing. I only found the bug after setting aside the mindset the bug MUST be in the part of the code of the method I had been working with.
    I eventually figured out that the problem I had was caused by code outside that method, code which inadvertently all now held references to just one List object. So when a blank line was added to the start of the list for display purposes, it also affected the list used by the business logic (which didn't expect the blank item at the start.) Even worse, it would add another blank line every time the list was used by the GUI. Just a mess.
    It took some more refactoring to fix those issues. One thing I realized to do was use a ValueConverter in the XAML to add the blank entry needed by the ComboBox without modifying the list. But I probably still should find a way to fix my list-generating method to protect its list from being modified outside of the method. (Is the best way simply make a clone of the list before returning it?)
    But anyway, I wish the bug was "in those couple things I changed", so I wouldn't have had to study the rest of the code to find it!

    • Wayne VanWeerthuizen
      Wayne VanWeerthuizen 9 months ago

      That was well stated, Tim.
      I have a followup question related to finding the best solution for fixing the bug I had, and preventing the same sort of issue from happening again. What standard approaches exist for ensuring that objects passed out of a method are not modified when they shouldn't be? One approach I see, might be to only ever pass out a copy of the object that would be returned and not pass out the original. But that could waste enormous amounts of memory if the exact same object needs to be passed out multiple times. I also see as an approach passing out an object that has no public setters, only getters. But that doesn't appear to be the best solution either, especially when passing simple collection objects such an array of strings; is it really wise to store such things in wrapper classes all the time? Would that not hurt efficiency? So what OTHER approaches exist for preventing that sort of error? Does C# have a way to somehow freeze an array or other object so that it is made immutable before it's passed out?

    • IAmTimCorey
      IAmTimCorey  9 months ago +1

      I've been there. However, I would say this is more interpretation rather than misstatement. The error was caused by the couple things you changed. It wasn't there before you put the code in and now that you put the code in, the error manifests. The tricky part is that the fix, as you found out, doesn't necessarily get put in the few lines you changed. Sometimes the fix is somewhere else. The key, though, is that you know what caused the issue (your new code). Even if that code is right, it was the cause of the issue. Now you can track down why it caused the issue. If you had changed 100 lines of code, the issue would have been caused by code in one of those 100 lines. That means tracking down what each line does all the way back to the beginning to even identify which line triggered the issue. Then you have to find the actual issue. Much harder.

  • Frank Oppermann
    Frank Oppermann Year ago

    Great Job:) The List I would encapsulate in a class. In that class you have 2 methods "BillCustomer" and "PayEmployee". This methods than only have 1 parameter and not 2 -> better to read and the new class have one task: Managing the List.

    • IAmTimCorey
      IAmTimCorey  Year ago

      I would have to see this in action. My concern is that it would cause other problems trying to solve this problem. Maybe try it out and let me know the results. Don't forget to change things after you get it all set so that you see how it responds to changes (do you have to change it too or can it weather those changes?

  • steve hoff
    steve hoff Year ago

    Love the tutorial great work thank you. However, one small niggle about what you're saying about Console.Write* cannot be used in other project types.
    You simply have to redefine what the standard out is in your solution ( It needs to inherit from streamwriter) and console will direct output to that writer.

  • Master Skriptor
    Master Skriptor Year ago

    What to you think of Domain Driven Design?

    • IAmTimCorey
      IAmTimCorey  Year ago

      I think it is crazy-complex and useful only in certain, limited scenarios. Even the people who advocate DDD recommend only using it in large projects (projects that have a team of developers and will take over six months to complete). Here is a good explanation on what it is and when to use it: stackoverflow.com/a/1222488/733798 My personal opinion is that if your system to help out one piece of an application is that complex, you probably need to re-think things.

  • Eddie Andersson
    Eddie Andersson Year ago

    Great lesson, love your channel one of the best for sure.
    I'm fairly new to code I will add.
    Just a very minor comment.
    I'm sure there is a reason, but why do you opt on making an empty console line, in a new line of code?
    Instead of using an escape key like \r in the output string?

    • Eddie Andersson
      Eddie Andersson Year ago

      IAmTimCorey thx again

    • IAmTimCorey
      IAmTimCorey  Year ago +1

      Well, the way C# makes it easy is to provide System.Environment.NewLine so that you don't have to worry about it. If you really wanted to use the switches, here is a post on what they all mean (basically, use /r/n): stackoverflow.com/questions/3091524/what-are-carriage-return-linefeed-and-form-feed

    • Eddie Andersson
      Eddie Andersson Year ago

      IAmTimCorey thx fof that answer.
      Follow up on that though what's the difference between \r and \n?
      As I understand \r is equivalent to carriage return on an old typewriter right?
      And \n new line, isn't that the same?

    • IAmTimCorey
      IAmTimCorey  Year ago +1

      Good question. There are a couple reasons. First, it is obvious. An empty Console.WriteLine() means a blank line. The escape character after the end of an existing line can be missed. Second, I like consistency. Doing the same things the same way helps avoid errors. When I want to write a line to the console, whether it has text on it or not, I use Console.WriteLine().

  • Harag
    Harag Year ago

    Great tutorial Tim, glad to see you back and hope you're feeling much better now. The refactoring could be done even more in places, but it's a good start for anyone new to this sort of thing.

  • Ибрагим Иванов

    Hello, Mr Corey! Thank you for video. Do you plan to make a lecture about programming patterns?

    • IAmTimCorey
      IAmTimCorey  Year ago +1

      I have five videos on the SOLID design patterns and I'll be putting out more design pattern videos in the future so yes.

  • Je ENgl
    Je ENgl Year ago

    Youre a legend

    • IAmTimCorey
      IAmTimCorey  Year ago +1

      Yep, it is on the list. I bumped it up the priority a bit.

    • Je ENgl
      Je ENgl Year ago

      IAmTimCorey Before I forget to ask, is there any chance you can make a video about memory leaks and memory profiling and how to detect and fix them? I have been facing a lot of out of memory issues in the last applications I made.

    • IAmTimCorey
      IAmTimCorey  Year ago

      I appreciate the kind words.

  • joaquin acuña
    joaquin acuña Year ago

    Welcome back Tim, i'm glad to see you again!

  • Calex
    Calex Year ago

    Glad to see you back, hope all is well!

  • Over Engineer
    Over Engineer Year ago

    So glad to see you're ok, and not just because it means I won't lose my new job :)

    • IAmTimCorey
      IAmTimCorey  Year ago

      I'm sure you got your new job because of your existing skills, not because of what I will teach you but I'm glad to be back.

  • Ian Trembirth
    Ian Trembirth Year ago +1

    Nice to see you back Tim!

  • Vinay Palaksha
    Vinay Palaksha Year ago

    Nice to see you back! Are you feeling better now?

  • Bharath Yadav
    Bharath Yadav Year ago

    Welcome Back Tim... Hope you are doing well now... ☺️

  • ferrad1
    ferrad1 Year ago +4

    Welcome back to the BEST C# teacher out there !!!

  • Colin Campbell
    Colin Campbell Year ago +7

    Great to see you’re back!

  • Damian F.
    Damian F. Year ago +1

    Welcome back - looking forward to where I can make some improvements, always able to find something I can make cleaner after watching your vids

  • Hard to Pronounce
    Hard to Pronounce Year ago +1

    Nice to see you back! Are you feeling better now?

  • mattd390
    mattd390 Year ago +5

    Tim's back! Hope you're feeling back to 100%, broseph!!

    • IAmTimCorey
      IAmTimCorey  Year ago +1

      Thanks. Just did a video on how I'm doing (short answer is I'm doing well).

  • Konstantinos Karagiannis

    New video, thank you! :D