Code Smells - 'Switches' in Unity3d / C# Architecture

hey what's up I'm Jason and today we're going to talk about code smells in your unity project if you've never heard of code smells it's essentially a term for parts of code that we can see or spot that lead us to believe that there's possibly an opportunity for improvement some opportunity to maybe put in some design patterns or refactor things to make it a little bit less fragile a little bit less brittle and easier to work with so in this example what we're gonna do is take a look at the switch statement code smell if you've used a lot of switches don't worry I'm gonna show you some really cool tricks so that you can avoid them and if you've never used switches before don't worry I'll show you what those are how they work and how you should minimize them in what you can use as an alternative so for this example I've set up a simple character and the character is gonna use some spells on other characters maybe be able to use a spell on itself or maybe on some other enemy or target we're mostly gonna just dive into code and not look too much in the editor but I wanted to set something up really simple so we've got a character here he's just a capsule he's got some health and some speed and if I hit one you see his health goes up because I'm casting my heal spell again we're gonna see that all in just a second if I it to nothing happens at all three nothing happens so let's dive into the code let's see what a switch statement looks like and let's see why it's bad and how you can fix it so here's our character class before I dive into the switch right here in the middle let's just take a real quick overview of what we've got going on here we have a health field a speed field and an is player field those are just variables that we can adjust and see in the inspector just to know what's going on and how things are working and the is player you'll see we're gonna use for actual target checking then we have a public version of is player which is just wrapping this private serialized field using an expression body property if you've never seen this it's just returning back the value of that but making it so that we can't actually change the value of is player but we can set it from the serialized field or from the editor then we have an update method well we have a target here but I'm just kind of hard setting this target and I even put a comment that we would probably do something to pick a target but our update is where some actual work happens we check to see if I press alpha 1 which is 1 the keyboard or alpha - which is - on the keyboard and you'll see that if I hit one we call you spell on another character we pass in the target and we use a heal spell type if we do - then we do a damage spell type exact same code just different value there now the spell type is an enum let's go to the definition of it so I'm just gonna go to heal click on it and hit f12 to go to definition and you see here that we have in my example three spell types that are actually hooked up three that are not hooked up that's why they're great because they're not referenced and really the opportunity to add dozens or even hundreds of other spell types think of any game that you've played all of the different spell types that could be there some games it might be tiny but a lot of games it could be a giant giant number of things and that's kind of where this turns into a mess so let's go back to the character so if we look at our use spell on another character and let's start suing in here let's take a look at the code nice and big we take a target and a spell type and then we check to see if the target is valid by calling into this spell target checker is valid target for spell type we give it the thing that our ourself - caster the target and the spell type and it's gonna tell us whether or not it's valid so we can only heal players if we're a player we can only damage NPCs or non players or for a player and then the opposite for players or NPCs right NPCs can only heal each other and they can only damage players at least in this example of course we could have whatever setup we want and implement this however we want but I wanted to show a simple implementation of that then we check to see if the targets valid if it's not a valid target we just bail out and don't do anything but if the target is valid we do a switch here now switch will do some logic based on what the value of the thing that we're switching on is so since the switch type is spell type the thing that we're passing in here spell type we have case statements for the different possible values so if the value here is damaged so that's the one that we passed in is damaged then we're gonna call the code after this case statement the target modify health and pass in a value of negative five and then break the break just says hey we're ending the case we're done with it we don't need to do anything else this is the one we've dealt with it now you may also see which statements that have brackets like this it works the same does the same thing we can do it with or without the brackets just wanted to show that so that everybody knows that there's no real difference there so then we check to see if it was damaged we did the damage if it was a heel you see that we're not actually doing negative damage we're changing the health by positive 10 and if it's a root then we call target's espied and let's take a real quick look at the modify health and set speed we can just scroll down you can see them we add the health and set the speed very simple right not a lot going on here so you might think like okay this is cool this works so if we wanted to add in another one we just go down here and add in the next case statement right and in fact if we went and looked at our spell types to see that we haven't actually implemented most of these so let's say we wanted to implement change model we went back over here to do that we would add a case statement spell type looks fixed that dot change model and we put a colon here then we'd say like target change model and give it whatever maybe we'd be able to pass in a model type or something but for now we'll just pick a random model and break and then we'd implement this on the target so that the target can actually change model so to do that by the way I just hit alt enter and then enter again to generate it the the keyword the keyboard shortcuts are a little bit different depending on the editor but just about every editor will do that there's a generate method so just find it control period or alt interest usually it and then here we could maybe change the model right and you would think okay this this could work maybe does some in fact let's let's let's try it out right let's do a debug log we're just and we're not gonna actually change the model but we'll say changing model this is gonna show where this goes wrong and why we run into problems okay so I set this to changing model or save changing model and I'm going to just go up here and when I hit alpha to instead of doing damage I'll do change model now of course in a real game we wouldn't hard code our spells to our abilities probably every we might depending on the type of game but in general we wouldn't really do this kind of a setup but I wanted to keep it really simple so that we can demonstrate it so let's hit play in fact for that kind of a setup we'd probably build some hotbar system or something so here we go it started now I'm going to hit too but I want to go to the console window I want to see the log nothing's happening right so if I hit one I see my health is going up because the heal one is calling but if it - nothing's happening the reason for that isn't obvious when you look in here right if I look in here oh okay why is this not working it's actually because we're doing switch statements in more than one spot and this is generally where switch statements become a nightmare because if they're in a single spot and it's nice and easy to read like this it's fine but what usually happens is that you'll find that you're doing this switch statement here and then you'll look around and maybe go into another method and you're doing another switch statement here and then you're doing it another place and another place in another place so here you'll see that we're just not dealing with the case of that other spell type so what do we want to do for that spell type this is a change model we really don't care right we just wanted to return true because I think the player should be able to do it to NPCs and NPCs should be able to do it to player right or maybe we want let's change it let's make it even crazier let's say for this one we want change model so we're adding a case statement for change model we want it to only be true if the caster is the target there we go so now it's going to return true only if we're casting it on ourself so if we tried to cast it on somebody else it wouldn't work and now you'll see that it's gonna work right let's let's go in a little tip play and watch and see our logs there we go look I hit to changing model we're seeing the message appear and if I hit one to damage or the health goes up so you might be thinking okay whatever I'll do it in two places the problem here again is that it's not going to be two places as your project grows things get more complicated it's gonna be three places and then four places and five and six and it's gonna keep growing and it's gonna get more and more complex and a whole lot harder to put a new functionality and a lot easier to put in bugs because if I go in and start setting this up and I don't implement it for everything I'm gonna have issues and I also just have to go into a bunch of spots I have to dig in and change code in a lot of areas so I'm no longer following that single responsibility principle instead I'm going in and changing this character or whatever the thing is and all of the other things that implement this switch every time I want to add a little bit of functionality so let me show you how we can refactor that at least in one way to make it a little bit easier there are a whole bunch of alternatives to switches but this is just one way that works really well for a scenario like this that I want to show so let's go back to our character I'm gonna zoom out a little bit and I'm gonna take this little line from the bottom that have commented and we're gonna drop it right up here and we'll comment these these two out and what we're gonna do is use another class so instead of going into our character and having a switch statement or having our character use the spell we're gonna set up a spell processor that does it all for us and that manages the way spells work on its own so that the character doesn't really need to know much about it so in fact if I do that I can even go in right here just delete that right out we will leave the change model and we'll make that public so how does this how's this going to work well let's dive into the spell processor system what we're gonna do is use the magic of reflection abstract classes and inheritance so here we've got a spell processor class and you see it's got an initialize and a use spell on another character and a lot of code that may or may not look extremely confusing so we're not gonna dive right into it instead we're gonna go to the spell class in fact let's go to the project for you just for a second I just want to show you what these files look like so we have the character we have the spell spell process or the target checker and the spell type that's the entirety of the project now let's go into the spell class the spell class I first just want to point out has multiple classes in it this file has many classes in it and that is only to make it very easy to show you it's not something that you should do should definitely split these files into their own class eventually let's take a look at the actual spell class itself that's this one right up top in fact I'm gonna collapse all of these other implementations so the spell class is an abstract class that means that we can't instantiate it you can't say give me a new spell you can say spell equals new spell it's not possible you have to inherit from it it has a constructor it needs to have a default constructor there it's got an abstract spell type property with only a getter on it so this is gonna let us assign a spell type to a spell and well require it for each implementation you see that in a second it has an abstract process on character that takes a target and doesn't return anything and then it has a boolean returning abstract is valid target method that takes a caster and a target and it will return us back whether or not the target is valid so let's actually dive into an implementation now and look at the heal spell in fact let's clean this up while we're at it so I'm on the heal spell I'm gonna hold alt and hit enter and writer it's control period in Visual Studio and I'm gonna hit move to heal spell that's yes we're gonna clean up the code while we're doing this and then we'll look at the implementation so you see we've got a public class heal spell in the base class is that spell that abstract one and then here we're overriding the spell type to implement it and we're returning back spell type of heal again this is the expression body property we could also do something like get and then have it say return that and we could write it all out like this but writer is nice and just says hey just turn that into an expression body property and make it nice and short so I do that then we have the process on character which modify is health you'll see that it takes in the character calls modify health and finally an is valid target and here we're just doing the same thing an expression body property and just returning back whether or not the caster is player value matches the target is player so only players can hit players and only non players can hit not players for a heal and that's it so that's our heal implementation and notice that there are no switch statements here and you'll see that there aren't any throughout the rest of the code let's see how this heal actually works now in the heal processor so the first thing we have here is an initialize method and I'm gonna collapse that for just a second because this is what we're calling into we're calling into use spell on other character and we're checking to see if initialized is false and if it is not initialized so initialize hasn't been set we're calling the initialize method now I also want to point out that this is a static class with a static method and a static boolean here and even a static dictionary and that's because we're not going to instantiate this spell processor the spell processor is almost stateless the only state that it really has is whether or not it was initialized and it's only ever going to be initialized once in the lifetime of our application so we can keep this all static and stateless as possible and keep it nice and clean so how does this work once we check to see if it's initialized well if it's not we call initialize and we'll dive into how that works in a second then we get the spell from a spells dictionary by spell type so if you've never used a dictionary before it's a key value pair where we give it a type for the key and a type for the value and then we can pass in the key in the init like an array index or and get back the value so if I pass in a spell type of he'll I will actually get back an object that is a heal spell object and then I call process on character on that heal spell object now if I pass in damage type I'm gonna get back a damage type object now you might wonder well how do we get this build out how does this work and that's all in this initialize so let's dive in to initialize now let's make this even bigger initialize so the first thing we do is we clear out this dictionary we probably don't really need to do that we could new it here or we could assume that it's clear but there's no reason not to just call clear on it if it's already empty it's fine if somehow this gets called again we won't be doubling it up in causing exceptions and error so we just clear it out then we use some reflection this is kind of where the magic happens so we're creating a ienumerable of types this is basically like a collection or a list of the different types a type is something like type of heal spell it's the class type so it's all of the different classes and the information about them so we're getting all of them from this assembly so the first thing that we do is get the assembly in fact let's just separate this out so let's go control shift R and introduce a variable and call this assembly right here just do it just like it did so we get the assembly for the type of spell so what it's gonna do is find the assembly in your project that contains the spell type now if you're just doing a unity project without any assembly definitions it's just gonna be your main assembly it's gonna be everything in there not a big deal though then we go through all of the types on that assembly so we have the assembly we get all of the types this is gonna return back every type that's in our project and then we use a link query to filter to where the spell type is assignable from the type that we're looking at so we only want types from our assembly that can be spells basically types that are implementing this spell abstract class so we want things that look like this that's when we say that is assignable from we're checking for this basically that this heal spell is some type of a spell now there's one other check here so we do the is assignable check and make sure that you get this right don't go backwards and but we did this check right here key dot abstract is also false because we also don't want this class in our dictionary this doesn't improve a spell type so it won't even fit in our dictionary and it doesn't do anything in we can't new it so we have to make sure that we don't get the abstract one then I loop through all of the spell types and we call another magic little method here activator dot create instance and the way this works is you can give it a type at run time and it will create an instance of that type now it's gonna return it back as an object so we just cast it as a spell because I know what's going to be a spell because I can see my code right there and then once we get that instance of it we add it to the dictionary so we say spells dot add we'd give it this spell type remember this is going to return back the implemented version of it so on the heal spell it's gonna return back heal spell and then we add that in in fact I think it would be fun and interesting to just kind of step through this so let's do it let's I add a breakpoint here and I'm gonna hit f5 and if all goes well I'll go in here I'll hit play and we should get the initialized call right away not crash and everything will work magically get to see exactly what's going on here all right so it's running oh I need to actually click right because we don't initialize until I try to cast there we go I hit one jump back over to the editor and there we go we've got a spell types is an ienumerable or a wear iterator array and if I click on it we can see the results I can expand it out here we have heal spell damage spell and root spell and these are all just types so then if we step in so I'm just gonna hit I can't hit f10 because that'll stop my recording so I'm gonna add a breakpoint here I'll hit f5 and then you'll see that we've added or we've created a heal spell because that was the first type in there so it took a heal spell it created it and the return value for spell type is heal and if I just do like a another f5 we'll go to the next one and that's gonna give us the damage and we now have a damage spell and if I hit a 5 again it's gonna continue on one more time through the loop give us the route spell and the route spell type and then if I hit one more time you should go to that initial highest yep and then I'll get rid of these breakpoints and hit f5 so I'll forgive that shows you what's happening we're creating these objects we're adding them to the dictionary and they're staying there forever so that we can use them again in our spell processor right here when we call that process on character so let's add a breakpoint here let's just see what it's doing again back to live put them right there to go back in and I'm gonna hit - that's the one for change model what what happened let's try it again nothing so oh I know why if we go back to our character nothing happened because I didn't actually implement it here so let's stop playing and we will copy that drop it down and put change model there and hit f5 again to start debugging we'll go back up I need to stop the editor before it stopped both of them if you're ever debugging by the way make sure that you stop everything go back in rebuild which is control shift B in here or F six and then then attached if you attach and stop and try to change things while the editor is still running you won't get a new version and everything will be weird and not work right okay so there we go we're in and we're gonna hit - and here's our breakpoint we're at the initialize check we have not initialized again I can't hit f10 but I'll hit f11 to see the steps there and it wants to initialize I'm just gonna let it initialize I'm gonna hit f5 and go down to this breakpoint so here we're gonna get the model from the dictionary or we're gonna get the spell for change model from the dictionaries f5 again and look what happened we got an error or something it didn't continue on if I go back over here say hey this doesn't exist in the dictionary now this is where we actually go in and implement a new one so we've set up the code for it we have the change model call on our character but we need to actually add the spell type here and again we're not going to go in and just add in another dictionary entry instead what I'll do is just go in to heal spell and I'll probably just copy it paste it I'll change this to be change model spell it change this to be changed model and then here we'll change the way it processes to do change model again the processing will probably be a bit more complicated raus to change this constructor the default constructor should match the name not be something wrong and invalid that causes an error but again the process I was gonna say should be relatively complex sometimes sometimes it's gonna do more than just call a single method on a thing and that's again where the switch statements will get messy because you know right now we have that one-liner in there when those grow out to be four or five six ten lines or start to call a bunch of methods gets a lot more complicated if we have it in the separate code the separate class file it's a whole lot easier to manage and we know where everything is for this thing so change mode spell or change model spell I mean it's gonna call change model and then how does the targeting work again oh that's right we want to check to see if the caster is the target the other nice benefit here is now I can tell exactly how this thing works for targeting since the effective type here is determining it look them say hey this only works on things where the cashier is the target then I can just go right here and move this to a new file save it off and rebuild stop editing or stop debugging rebuild and all that stuff and then go back in I should be able to hit 2 and have it actually work let's see - nope nothing why didn't it work so let's let's take another peek oh I did work it just was really slow on the logging nice apparently I have to click over to the log window that's a weird new interesting issue I never seen that before anyway it does look like it's working it's working fine just some weirdness with my editor probably in my recording setup but I hope that this kind of explains how to avoid a switch or at least one way to avoid a switch there are a ton of other cases where you might see yourself using a switch statement but most of the time what ends up happening is you'll start with something small like that character was initially then you'll add in the target checker you'll add in some other thing you'll add in another thing and as these systems keep growing and getting more and more complex you'll start to find problems I've seen this with like tile types on a on a ping pong style game or like a Tetris game or just about anything you're gonna end up with switches in enums and you want to try to minimize how much of a mess those get to be and just start refactoring start cleaning things up and when you start to see big big cases refactor them right away now if you're looking for where are some really good places for switch statements the best one that I know of and the one that I've heard everybody say is the number one spot is in some sort of a factory class in a factory classes basically it's usually a static class that you call in to give it some info maybe some enum type or some data and then it will return back out a premade class that's all ready to go if you're not familiar with that don't worry I've got a video on the factory pattern you can go check out and it'll tell you all about that and I think I even dive a bit into using some of the same same functionality and a little bit of reflection in there now if you have coat smells you're curious about or just wondering if something's bad or good or a better way to do something drop comment below and let me know be curious to see what everybody is interested in what kinds of things you guys are running into also if you have other good solutions for switch statements or other coat smells I'd love to hear them it's always interesting for me to find out new stuff and just talk to people about this kind of stuff and again thanks for watching special thanks to everybody on patreon and all my email subscribers and everybody out there you guys are awesome really appreciate it bye you

Channel: Jason Weimann

Views: 58,836

Keywords: code refactoring, code smells, clean code, switch statements, code smell, code quality, technical debt, tech debt, unity3d, unity tutorial, unity, design patterns, unity3d college, game programming patterns, unity solid, c# programming, unity clean code, unity architecture, architecture, tips and tricks, unity c#, c# code review, unity3d architecture, code review, unity project, unity tips and tricks, refactoring, unity3d solid, unity design patterns, #unitytips, unity3d c#

Id: nqAHJmpWLBg

Length: 25min 20sec (1520 seconds)

Published: Mon Aug 05 2019

Reddit Comments

[u/andybak]

If only you'd put your argument forward in a written form.

[u/FavoriteNever]

Switch statements can be abused just like anything, but I personally find them to be an excellent companion to enums like you're using here. The problem here isn't the switch though: the IsValidTargetForSpellType method would have returned a false result for the new spell with or without switch. When you add a new value to your enum you have to fix the checks on that enum wherever they may be. It's just part of being a programmer. If the code had been written using if states or a mapping or any other method it still would have failed when you didn't edit the validity check.

[u/RoarkTheOrc]

Not gonna watch a video about this, honestly, because it's slower for me. There are a lot of resources out there about switches and code smells. Here's a good one against switch statements in certain cases https://stackoverflow.com/questions/126409/ways-to-eliminate-switch-in-code#126455