Lesson 16.2 – Refactoring the player movement function

Lesson Objectives

At the end of this lesson, you will know…

  • How to break a large function into smaller, easier-to-understand functions
  • How to move functions from the UI to the “business classes”

 

In the last lesson, we created a huge function to move a player to a new location. However, it was too big to easily maintain.

Now, we’re going to clean it up and move things around so it is easier to read and understand. This is often called “refactoring”.

Refactoring is a large subject, and there are many techniques you can use to do it. We’ll focus on a couple of the common, simple, techniques that have the biggest benefits. As you continue writing programs, you’ll learn more refactoring techniques.

 

Creating the functions to handle user input

Step 1: Start Visual Studio Express 2013 for Desktop, and open the solution.

Step 2: Right-click on the SuperAdventure.cs form, in the SuperAdventure project, then select “View Code”.

Step 3: Double-click on the Player class in the Engine project, and replace it with the code from the Player class here (don’t update the SuperAdventure code yet): https://gist.github.com/ScottLilly/6670da749c4ad7e7bff7

 

Steps for refactoring

Refactoring is just rearranging your code so it is easier to work with. You’re not looking to add any new features, fixing any bugs, or improving the performance.

There’s a lot you can do for refactoring, but we’ll focus on a couple of the most common techniques.

Step 1: Look for duplicated code.

If you have the exact same code in more than one place, then you can usually move that code to its own function. Then, in the places that used to have that code, change it to call the new function.

This is especially important when you might make changes in the future.

When we created the MoveTo() function, we could have put all that code in the four functions that moved the player in each direction. However, if we ever decided to change how the movement logic worked, we’d need to remember to make the change in four places. If we weren’t paying attention, we might only change three of the four. Then, the game would suddenly start acting strangely every time the player moved in the one direction that didn’t have the change added.

We don’t have duplicated code in this function. We have a few places that look close to each other, but no exact duplicates. So, we’ll look for other ways to refactor this function.

Step 2: Look for code that has one distinct purpose, and move it to its own function.

In this huge function, we have lots of code that matches this description.

For example, lines 56 through 78 checks if there is a required item for a location, and (if so), if the player has it in their inventory. We can move this to its own, smaller function.

When we move this section of code, we should think if it might belong in a better place – maybe a different class. We currently have it in the code for our user interface. However, this code is looking at the player’s inventory. So it makes sense to move it to the Player class.

Look at lines 28 through 48 of the new Player class code that you just added.

In this function, we pass in the location and see if the player can move there – either because there is no required item, or because they have the required item in their inventory.

This new HasRequiredItemToMoveToThisLocation() function is 20 lines long. It does one thing, and is small enough that it’s very easy to understand. If we ever want to change this logic, we’ll be able to do it in this one place.

For example, you might want to change the game to also have a minimum level requirement for a player to enter certain locations. You can go to this function and easily make the change, instead of digging through the 300 line function.

Now that we have the function in the Player class, we can clean up the SuperAdventure class. Replace lines 57 through 78 with this code:

We just replaced 20 lines of code with 6, and made this function a little easier to read and understand. That’s what refactoring is all about.

 

Now we’ll move the code that checks if the player already has a quest, and if they’ve completed it, to the Player class.

Look at the two functions in the Player class at lines 50 through 74:

In the SuperAdventure.cs class, replace lines 86 and 87 with these calls to the player object, and delete lines 88 through 100.

 

Now, let’s move the code that checks if the player has all the quest completion items in their inventory.

In the Player class, look at lines 76 through 106:

Go back to the SuperAdventure.cs and replace line 96 with this:

Then, remove lines 97 through 133.

 

We can also move the code that removes the quest completion items from the player’s inventory to the Player class.

In the Player class, we have this new function at lines 108 through 122:

Then go back to SuperAdventure.cs and remove lines 106 through 117, and replace them with this:

 

We can also move the code that adds the reward item to the player’s inventory into the Player class.

In Player.cs, look at lines 124 through 139:

In SuperAdventure.cs, replace lines 119 through 138 with this:

 

In Player.cs, look at the function at lines 141 through 154:

And go back to SuperAdventure.cs. Change lines 122 through 132 to this:

 

In the rest of the function, we have some code that updates the ComboBoxes and DataGridViews, since the player’s inventory may have changed because of completing a quest.

In the SuperAdventure.cs class, create these new functions:

Then replace lines 183 through 272 with these lines:

 

Is the function simpler now?

Before, the MoveTo() function was over 300 lines long. Now, it’s 140.

It’s still long, but it’s much easier to read (and understand) now.

We’ve also moved a lot of the “logic” code out of the user interface class – which is a good thing. The code in the user interface class should only be used to handle receiving input from the user, and displaying output. It shouldn’t have a lot of logic in it.

Now we have more of the game logic in the Engine project, where it belongs.

We could do more refactoring on this function, but I think this is a good place to stop for now.

If you’re interested in going further, I suggest you look at .Net’s LINQ. You can use it to make the new functions in the Player class even smaller, and more concise. But LINQ is a whole other thing to learn, and I won’t be showing it in these starter tutorials.

 

Summary

Refactoring doesn’t change what a program does, it only cleans up the existing code, so it’s simpler to understand and work with.

We often do that by finding pieces of a function that can be moved to their own function. Then we have the original function call these new, smaller functions. The smaller functions are easier to understand, since they aren’t buried in a huge function. And the big function is easier to read, since it is shorter and more concise.

NOTE: There were a lot of changes to the code. If you’re not sure that you typed everything correctly, you can paste the code from the link below into the SuperAdventure.cs class.

 

Source code for this lesson

Get it from GitHub: https://gist.github.com/ScottLilly/6670da749c4ad7e7bff7

or DropBox: Lesson 16.2 – https://www.dropbox.com/sh/8mjg324h3wk5jzw/AAAWClu0FLR0eEK7vYXKtaKTa?dl=0

 

Next lesson: Lesson 16.3 – Functions to use weapons and potions

Previous lesson: Lesson 16.1 – Writing the function to move the player

All lessons: Learn C# by Building a Simple RPG Index

29 thoughts on “Lesson 16.2 – Refactoring the player movement function

  1. You should show us what code to replace, not tell us what line. Our code might not look 100% identical to yours, so telling people what lines to replace/delete isn’t amazingly helpful.

    1. Totally agree with Timothy Harrelson’s comment. Show us what code to replace. In my case, I was making my own comments in the code as an aid to memory for everything I’ve learned in the lessons up until this point, so due to those comments, my code was about 20 lines out of sync with yours. At the end of the day, I simply copy-pasted your code over my own (hopefully I shouldn’t need the comments anymore, after all that’s the point of trying to learn) but as Timothy notes, we all have slightly different ways of doing things, such that absolutes like line numbers only work for those who don’t deviate even slightly.

      Otherwise, it’s been a great tutorial and I’m very happy you went to all this effort! After getting halfway thought Herbert Schildt’s “C# 4.0: The Complete Reference”, I found your tutorial to be a welcome breath of fresh air, bringing simplicity to many concepts in a fun and practical context. Thanks!

      1. Thanks. This was a tough lesson to figure out how to write. Every way I could think of had one problem or another. Although, it does show why refactoring large functions is important (since they are difficult to work with). I may do a separate guide on refactoring in the future.

  2. I’m kind of stuck here. If I run the Game and try to walk right in the beginning to the North by pressing the North Button, nothing happens. VS doesn’t throw anny Errors or something. Somebody able to help?

    1. The first thing to check is the SuperAdventure.cs class. Ensure this function exists:

      private void btnNorth_Click(object sender, EventArgs e)
      {
      MoveTo(_player.CurrentLocation.LocationToNorth);
      }

      If the function exists, go to the design mode of SuperAdventure.cs and click once on the “North” button. In the “Properties” section (lower-right corner), click on the lightning symbol (to see the events for the button) and see if the “Click” event has a value of “btnNorth_Click”. That’s where the button’s click event connects to the function.

      Click event for the 'North' button, in the 'Properties' section of Visual Studio

      If the click event is empty, double-click on SuperAdventure.Designer.cs (in Solution Explorer) and scroll until you find the btnNorth section (shown in the image below). Then add the missing line:

      Code in the code-behind page for SuperAdventure.cs that connects the 'North' click button to the function to run when it is clicked.

      this.btnNorth.Click += new System.EventHandler(this.btnNorth_Click);

      Please let me know if that does not fix the problem.

      1. The Buttons didn’t had the Click Event saved. After adding the events to the Buttons everything works fine now!!
        btw. it’s a great Tutorial!! Thank you for that.

  3. Im a little lost on what code Im replacing.

    I am suppose to replace line 57-78 which for me is

    }

    }

    return false;

    }

    public bool CompletedThisQuest(Quest quest)

    {

    foreach(PlayerQuest playerQuest in Quests)

    {

    if(playerQuest.Details.ID == quest.ID)

    {

    return playerQuest.IsCompleted;

    }

    }

    return false;

    }

    public bool HasAllQuestCompletionItems(Quest quest)

    {

    // See if the player has all the items needed to complete the quest here

    which makes me a bit confused. could you please advice me on which code Im replacing?

  4. Hi Scott,

     

    In this lesson I replaced both my forms code as well as the player class’ code with the code you put on your github repository at the end of this lesson. Later on when running the game I noticed my comboboxes (to select weapons ) didnt stay visible on the UI. I figured it out because the forms constructor used to have a line that adds a rusty sword to the inventory of the player. In the new version this line was removed. So whenever the moveto method was called and the location would have a monster, it would instantiate a monster, then put my combobox and respective buttons Visible property to true, then run the update UI methods which would find no weapons in inventory, thus immediately hiding them again.

    I fixed it by re-adding this to the forms constructor:

    _player.Inventory.Add(new InventoryItem(World.ItemByID(World.ITEM_ID_RUSTY_SWORD), 1));

     

    Just pointing this out in case people have problems with it.

     

    Kind regards,

     

    J

    1. Thanks. I’ll take a look at this and tried to get it resolved. At one point, I moved that line from SuperAdventure.cs into the Player constructor. I might have missed something when I made that change.

  5. Hello Scott,

     

    first of all. Thank you very much for this great tutorial. I’ve never seen something that shows the using of objects so clear like here!

    I’m sure you can help me with this issue I get when I push the button “North” the first time:

    In my Player class I get a NullReferenceException on this line:

    if (location.ItemRequiredToEnter == null)

     

    in the function:

    public bool HasRequiredItemToEnterThisLocation(Location location)

     

    Regards,

    CodeB

    1. It might be coming from the World.PopulateLocations function. Can you double-check that function, to be sure all the LocationToNorth, LocationToEast, etc. values are set?

      If they look correct, can you upload your solution (all the files, including the one in the sub-directories) to Dropbox or GitHub, so I can look at it? There are a few places I might need to check, to find exactly where the problem is happening.

      1. Hi Scott,

        I double checked the World class. As far as I Can see the locationToNorth is distributed correctly.
        The alarm happens when the compiler is coming to the if statement where it looks if the Item to enter is Null.
        I’ve looked about that process with some breakpoints and debug it.
        This Information I got:
        locationToNorth=Engine.location –> Thats fine for me. I can enter the area to the north.
        ItemRequiredToEnter= null –> This is why the NullReferenceException happen (I guess) But I don’t understand why it happen because we set this Variable to Null(because we don’t need anything to enter) and we just ask with the if statement “if its Null then return True”

        I try to setup my Dropbox later on when I get a bit more time.

        Thanks, CodeB

        1. Hi there,

          I found my fault:
          The problem was in the SuperAdventure.Designer.cs file.
          Here I had this code line:
          this.btnNorth.Click += new System.EventHandler(this.btnWest_Click);

          it must be:
          this.btnNorth.Click += new System.EventHandler(this.btnNorth_Click);

          As well I had some other very simillar problems with this buttonevents.
          This all was happend because I forgot to doubleclick some of the buttons at the very beginning of the tutorial.

          Now it works fine and I can go on.

          Regards,
          CodeB

          1. I am getting the exact same error, but your fix below is not the same for me.
            This is driving me crazy.

  6. Hello,

    i did a huge mistake and dbl-clicked on a Label somewhere, at a time i didnt noticed and now i got this Error:

    ‘SuperAdventure’ does not contain a Definition for ‘SuperAdventure_Load’ and no Extension method ‘SuperAdventure’ accepting a first Argument of type ‘SuperAdventure’´could be found (are you missing a using directive or an assembly reference?)

    On the SuperAdventure.cs[Design]
    i recieve the error “The designer cannot process unknown Name ‘SuperAdventure_Load” at Line 287. The Code within the method ‘initializeComponent’ is generated by the designer and should not be manually modified. Please remove any changes and try opening the designer again.

    Would be great if you could help me again with my error.

    Best regards
    Gerrit

    1. To fix that problem, open SuperAdventure.Designer.cs

      Solution Explorer, with SuperAdventure expanded to show SuperAdventure.Designer.cs

      Look for line 287 (which will have the line for the SuperAdventure_Load eventhandler). You can either delete that line, or put two forward slashes in front of it “//”, to comment it out.

      You can learn more about how the eventhandlers work in Lesson 21.3

      Please tell me if that does not fix the problem.

  7. Hello,

    First of, thank you for this tutorial. I’m learning C# at school but I really like the way you teach us.

    I come here because I have a problem with my game. I’ve made the .exe file, I can use the UI but the game doesn’t work properly. I can’t manage to find a solution, that’s why I’m asking you to help me if you don’t mind …

    Here is a screenshot of the finale program output (once compiled):

    http://imgur.com/a/rsBEi

    Here is my whole project (in a .rar file):

    https://www.mediafire.com/?exmxymh19b405v8

    Thank you in advance,

    Sincerly,

    Jean.

    1. Hello Jean,

      The problem is that the movement and combat buttons are not connected to the function in SuperAdventure.cs. They are missing their “eventhandlers”. You can fix this by reading Lesson 21.3, and following step 2 for the buttons. Or, you can open the SuperAdventure form in Design mode. Click on each button once. After you click on it, look at the “Properties” section of Visual Studio (in the lower-right corner). Click on the lightning bolt, to see the events for the button. Find the “Click” event, and click on the area to the right of it. That will show you the available functions. Select the correct function for each button. That will create the “eventhandler”, and should fix your program.

      Please tell me if that does not work, or if you have more questions.

      1. Hi. I checked to see if the btnNorth_Click, all had the right event handler. It still does not work. Love your tutorial btw.

        -Rasmus from Denmark

  8. I keep getting this error when running the application:
    (player.cs)
    public bool HasRequiredItemToEnterThisLocation(Location location)
    {
    if (location.ItemRequiredToEnter != null)
    {
    // There is no required item for this location, so return “true”
    return true;
    }
    “An unhandled exception of type ‘System.NullReferenceException’ occurred in Engine.dll”

    I have looked at solution for a prior commenter, but that is not the solution for me. I have checked every reference and everything has been coded correctly. I even went back, copied and pasted the code you supplied, and I still get this error.

    Please help!!!

  9. Hello Scott. Thanks for this GREAT guide. I’ve spent whole September with it and I’m loving it 😉
    Got one questions though:
    My weapons list displayed in cbWeapons has 3 weapons in it.
    Whenever I change location, selected item goes back to the first one on the list. This can be annoying, especially when player wants to keep using different weapon (e.g. with index 3) all the time.

    This is caused by: cbWeapons.SelectedIndex = 0;

    Is there any way to save item’s index that was selected and keep it when player moves into new location?

  10. Hello. I get this message when i pasted the last code in.

    “Severity Code Description Project File Line Suppression State
    Error CS0106 The modifier ‘private’ is not valid for this item SuperAdventure C:\Users\ander\Documents\C# projekter\SuperAdventure\WindowsFormsApp1\SuperAdventure.cs 193 Active”

    This is on all of the private methods i pasted in from the last box we had to paste in.

    1. That’s a strange error to see here. The first thing I can think of that might cause that error is if the code was pasted outside the curly braces “{}” for the class.

      If you can’t see if that is the problem, can you upload all the files of your solution (including the directories under it, and the files in those directories) to GitHub or Dropbox, so I can look at it?

Leave a Reply

Your email address will not be published. Required fields are marked *