Unity3D Code Review #1 Fuze

hey what's up Jason here from unity3d college so a couple days ago I sent out an email to my list just offering to do some code reviews and record them just put them up online so I personally seen a big variety of quality and code reviews sometimes they're great and they really help people out most of the time they pretty much suck though and they just talk about you know one specific little thing or nitpick a couple little things and never really get to the bigger stuff so I'm gonna try to be closer to the first one with a good code review but we'll see we'll see how it ends up anyway I wanted to start with this project Vladimir sent this in and I just thought it was a cool little game and it was fun and it seemed like a good way to start this off and he mentioned in the email that he's a technical artist not a programmer and it shows like the the art is pretty nice I like the look I like the feel of the game you play as a ball you jump around you get these other balls they're lumps and they follow you around you have to get all of them then there's these no gravity zones like this which I thought was really cool just kind of like floating around or you click to basically add some force in you can click and hold to add a little bit more force there we go so let's see if I can stop sucking at this I'll show you a little bit of the game a little bit of what he's got going on here and then we'll just take a quick look at the code and see if we can at least get a couple good tips to how to help improve things a little bit okay let's see can I get up there apparently not apparently I'm pretty bad at this okay there we go grab that piece and nice little ball bounced out of the way I like that or slid out of the way okay let's see if I can get up in here so right about here of course my aim is terrible but I think you kind of get a sense of what the game is so you go grab these points and then get to the end and I think here I'm not sure if I'm supposed to climb up I think some event was supposed to happen and actually finish the game so cool game fun stuff it looks nice let's take a look at some of the code so the first thing I want to look at is the ball control script now this is for the actual ball like the players controlled character and see right now it's it's certainly not the cleanest thing it's not obnoxiously large but the the formatting and lack of spacing makes it really really hard to read so first thing I'm going to do that I always do is hold down control okay control D and just kind of clean up that formatting move extra using statements that we're not using and then take a closer look so we've got 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 member variables on this class it's a relatively large number especially for something as simple as this and I get the sense just at first glance that some of this stuff should probably be pulled into a separate class and this this section right here alone kind of gives the idea the fact that there's a comment and there's a whole section of parameters for a specific subset of this thing or part of it and it's not even really for the ball it's for those other little orbs that are following you these should be probably pulled off into a separate class maybe another component or just a um another class that's attached to this game object and I'm guessing that these public ones let's see let's take a look at Lum distance shift f12 and Lum parameters update it's doing something it's checking to see if it hit one in a raycast I guess again this is showing two problems one I'm not sure that these really belong in ball control because updating parameters for things that are following the ball doesn't really make sense at and ball control it seems like a separate thing but to this also is just a confusing method because I don't know what Lum parameters date really means they wire their parameters that need to be updated what are those parameters that need to be updated them to be two good questions that should be probably filled out in the name of the method so let's take a real quick look so this is setting a position offset and it's setting a position offset and it's doing anything else it's going through alum slots and holders and then there's a delayed ray point which is I don't know what that is either it's hard to say let's see let's search for all references to delayed ray point and look here to plate let's go to definition it's some lump parameter don't see let's take another quick look and see if we can figure out what it is so it's looping from here to there okay I think this is something for moving it or yeah moving the Ray that it's doing so this is really some stuff that yeah this should definitely be handled in that Lum object or another class that really handles this this entire update should be moved out of there and renamed to be a little bit more explicit because this really looks like the whole goal of this thing is to update the alum position offsets so I'd probably just rename this like update Lum position offsets just to make it a little bit more obvious so now I know hey okay this that's what this is actually trying to do now it's uh let's go back up one thing that really stood out to me and this was the casing and again and I said that a lot of times bad reviews just kind of focus on little things like this but I don't think of casing in here as a minor thing I think it's a relatively big thing and it it should probably be fixed up so there are a couple issues here first off everything is semi randomly case to some things private our Pascal case like this some things that are private are short are B's sometimes they're camel case sometimes they're camel case starting with this this part I kind of understand is trying to say this is the reference to the thing on this object I would probably just rename these though get rid of the word this just name them Lum holder or whatever and same with the animator I'm just gonna fix those real quick there you go but more importantly the private things should if we're just following some basic standards should always be camelcase if it's private make it camelcase start slower letter big letter on every word after that if it's public it should be pascal case and it should probably be a property not a field you know the reason for that is a lot of these things let's see let's look at fused by time used by time is read let's see only in ball control there's actually no reason for this to be public at all let's look at the code again and herbicide the editor and let's select the player ball and I'm guessing this field is just showing up here so what was it fuse length yeah so we go back into code here oh no it's fused by time let's take another look so is there a fused by time where was that oh oh sorry this one's private ah getting myself mixed up here so anyway this should just be a lowercase F see I got a little bit mixed up because of the casing that's exactly the problem this thing was private but it was K strong so I had my brain implicitly just kind of went to oh this is public why is it public now let's take a look at these ones so we've got a couple of particle system references I'm gonna hit shift f12 I'm guessing these are here just so they can be updated in the editor right so this is constant fused particles and burst fused particles let's take another quick look so yeah these should just be private we don't need these being publicly accessible so in this case I just changed these over to be private and add these add the serialized filled attribute just like that and then rename these to be actually not curse to first to be a camel case so that they match with what a private variable should be and give that just a second yeah and these are both are they just both referencing that same particle they were there both referencing with the same particle system anyway so just reapply that real quick and I assume this is a prefab so we'll apply the prefab now let's see what other things do we have here um all of these again just use I'm just gonna fix these up right now and Mark these all explicitly private again it doesn't matter if you put the word private or you don't as long as you just keep it consistent personally I like to put private in front of everything that's private just like this all the way if you click hold alt and drag you can actually just paste along all the lines and add a space on all the lines and done so I like to keep these all like said grouped up and separated and explicitly mark so if you want to keep all of the public things let's grab anything else oh these are the lump parameter so these are separate for a different reason so let's just look up here again so we got public vector basic views force let's take a quick look at that and here it's just reading it there that seems kind of pointless let's take a quick look again basic views force is just a vector - so this yeah this should also be just private really I'm not gonna go through and change all of these but my sense is that all of these are editor fields that should just be marked as private in serialized field and the only actual public thing on here is the instance which should be a capital I and it should probably not initially set itself - nope because it's already know anyway so right now we don't need to do that what we should do though is change this cue get er with a private Center so now nothing else can change the state of this instance and he's using this as just a basic singleton so if I look and shift f12 you see that right here in start he's setting the instance to this if it's no and then destroying the instance or this object if it already exists and that's just a common thing with single things if you have one and something else makes one spawn you usually don't want that to happen so you just call it destroy like that so that's a fine pattern and all but it's much better to make sure that you set that to private just so nothing else can break it now let's take a look at some of the methods let's see so the start method is just initializing things it looks like setting up the single fan I'd probably move this into an awake all of these get component calls but nothing big stands out here and starts it's a little bit long but it's reasonable in the update method again let's get some spacing in here update method is perfect it's calling count fuse parameters and update Lum position offsets let's see what count fuse parameter system not much either so relatively white update method we do have some slightly longer impulse methods though so let's take a look at that I think if we go down here you see is using the on touch down more spacing definitely always put a space between your methods so just it makes it much much easier to read and easier to separate just quickly like that it's of course like oh yeah that's easy to tell the difference it is but when you're looking at a glance real quick it's hard to tell that's actually two different things so you want to be able to read your code as quick as possible and as painless as possible so in on touch down you see is going into this impulse fuse and this thing is pretty big now what's it doing here first it's count fuse out fire Direction I don't know what that means what does this mean position x equals transformed up position X and transform position y ok so we're getting a vector two and then we're getting the direction - okay I see I see so the point of this is to get the direction of the click so we're probably just select this pick control period extract method and just go get direction from quick to ball give it a nice explicit name for what we're trying to do the next thing we're doing is self use and I'll get rid of this comment cuz that's confusing self use force guesses it's trying to calculate out the force required let's see if based off of force oh the distance okay so it looks like we're looking at one divided by the distance plus point zero one and then if the force is too great we set it back to zero because they went to okay interesting so first thing I do here is definitely clean this part up and if statement should almost never have the thing that it's doing on the same line just because it's hard to read again code quality is mostly about readability you know computer can read whatever humans have a much harder time so make your code human readable first so here we're figuring out the fuse force and then we're getting a vector two for the impulse force so what I do here is just select this chunk of code and extract it calculate impulse force [Music] seems good that does have some out parameters which I'm not a big fan of what's direction I'm getting used for oh yeah so it's getting used for a couple things but I'd rather have that here even without parameters eventually I think could probably refactor this a bit more and pull that out a little bit more but I want to minimize or at least make the code super easy to follow along so let's see I don't know what fused by time is fused by time it's either 5 or 0 count fuse parameters so what is this use this is it's that it's something used in the calculation here of the force not completely sure how it works and I think that could probably be clarified up just by renaming that parameter you know a little bit longer name that it's more explicit on what it's actually used for and how it's used would help except I could go through and figure it out but you really want the code to be really obvious you want to be able to go through and see exactly what's going on here now this particles control section what's cool it's good that it's it's commented at least now we know what this is but we should again extract it just call this uh what is this burst fuse particles [Music] yes particles there we go give that another method just again we the goal here is to just simplify and shorten this down and then here so if hit fused by time equals 5 or 0 slightly cleaner way to do this and cut down where we got 1 2 3 4 5 6 7 8 8 lines of code to 1 we say fused by time equals and then we say hit question mark 5 : 0 and a semicolon so what's that going to do essentially everything that's going on here it's gonna set the value of fuse by time to 5 if it is true and 0 if it is false so what it does is it evaluates the statement on the left side of that question mark if it's true you get the value right after the question mark otherwise you get the value after the : so we go cut that down and again just keep shrinking the code and shrinking the method the goal here is to make this very very readable very easy to figure out what's going on so let's see the next thing that's doing is is using an event listener component and initializing an event by type event type is fuse and it looks like it passes in a point and a quaternion and a vector well let's let's just search for this event type you know I don't see that referenced anywhere let's go into here so it's just an enum an event manager of that type I'm not sure exactly how that sucked up it's possible that it's hooked up in in the inspector somehow let's see in it event what does it do it finds an event container based on a physics material interesting and an event type what's that let's look at that interesting so in this case by the way instead of using a list of event containers you may want to just use a dictionary that just has a tuple or a type that's got a physics material and an event type or you may want to use a dictionary of dictionaries where you do like an event type top level and then physics material I didn't make the lookup a little bit faster especially for something like this where you're going through just using a dictionary will be a bit quicker and kind of not require you to write so much code there let's let's go back in though so yeah I still don't know what this let's go into an init event again so init event an in event gives us an entity or event entity pool and then what do those get used for let's see let's go to the oh it's just a list of game objects oh okay interesting well I don't want to dive too deep into there because I feel like I'm gonna be digging into a hole for a long time and people may get a little bit bored so let's take a look at one more class and then I'll just kind of wrap this up like I said I think going through a full review of this probably take a couple hours it's a reasonably complicated complex project that's doing quite a few things so a full good code review take a while but I want to at least hit a couple other things so let's look at well here let's go to our player ball again and we've also got this lump older so let's open that up so this is I think where some of that stuff that's on ball control should probably move too and first let's just clean up the formatting ctrl K ctrl D and we've got a struct here at the top now I'm not personally a fan of having multiple classes or structs in a single class usually so I usually just move that to a different file like that just get it kind of cleaned up and have our class right at the top and let's clean up those using statements again just get a little bit more spacing in here and take a quick look so this has all public fields except for a collider yeah well look first let's just rearrange those so that they're they're grouped up together now I'm guessing that all of these are set up let's see or any of these actually accessed outside the class or they all just public by the way if you just click on them and hit shift f12 and visual studio here let's let's just dock that window it'll show you all the references to things so here you can see okay this one has lumps is actually referenced outside of this class and I'm guessing that's not something that we see yeah it's getting set right here true and false and let's see in an update yeah so this one definitely can't be set in the inspector so obvious what fix for this get set done so now it's no longer in the inspector and confusing although I'd hit f2 and make that capitalize move it down here move that up now these other ones assuming that they're not referenced outside this class up ball is in range this is I think the same case so this one would also get changed to a property and whether the public properties don't show up in the inspector and it's good to really minimize the number of things that show up there you really only want to show things in the inspector that are options that you can say and sometimes you might want to put one there for debugging temporarily so you can see the state of a thing if you don't want to keep switching back and forth between debug in normal mode but as soon as you're done debugging it make the make the thing private or turn it into a property so it doesn't show up there and it doesn't confuse people otherwise you know people go through or you go through later and you let go I don't remember why I have this setting here you know is this actually a setting should I be using it whatever it didn't also on along that line let me go back into ball control one thing I'm a big fan of like fuse force I don't know what that is exactly but if there was a tooltip here you know does something I don't know if this was a more explicit just a tool tip right there it'll show up in the in the inspector in the editor like let's save and jump back over and it'll give you and other people a little bit more idea what the thing is doing and it's not so much code but it's kind of documenting the code without a comment it's better than a comment because it actually it helps designers and it's much more likely to stay accurate so where was that that was on our ball control is a fused link up it's a fuse force I don't know it was one of these which one did I send it on now I have to look fuse force does something okay so oh it's not set of serialized I need to put it on one of these public ones right now now you can use it on the private ones that are she realized field too I just happen to put it on a random private one because I wasn't paying enough attention there but now you go now we've got the tool tip anyway let's jump back over to the Lum holder again I just want to take another short look okay so again spacing and spacing and these right here these methods I don't like it all so let's take a look if we look at an update we go through each Lum slot we set a follow point equal to the position plus the Lum position offset for the slot okay I think I get what that is just saying hey this little ball that's following me should be at you know my position the Lum holders position plus whatever the offset is for that slot and I think these slots are may be defined in data somewhere I see yeah there's two realize I assume they're getting set and data although let's let's take a quick look oh no they're they're getting done in code that's right in that offset that's what this method that we had update Lum position offsets and now that it's named better since we renamed it I I'm starting to understand what it does so this updates the offsets that the holder is using again this update Lum position offsets what is this possibly doing that doesn't belong here yeah this whole thing let's take this big chunk of code cut it move it over to this Lum holder and get rid of all these comments add in these two fields now right here it looks like well here we don't need references to Lum holder don't need that Lum slots and hold sorry I'm not gonna I'm gonna undo this just because it's gonna take a little while but this thing should be moved over in all of these fields if we go up to ball control all of those fields up here that are related to it should also just get moved over the hell let's just move it cut I I'm just pretty sure this is gonna break some other things so it'll take a little while of debugging Gunther already know if it actually builds I'll be surprised if it builds an L okay good let's say like they know it's super obvious it should have moved okay it's just the initialization of these things cut those save go over to here now let's just add a new method I don't want to shove all this in the starter so say private Boyd initialize um objects and get a better name but for now we'll keep itself there we go and we'll call that in start now let's see look at this we are so close to there we go just move that into the right class now I'm not a hundred percent sure that LEM holder is the correct class four but I think it seems like the best bet it's definitely better than ball control let's jump back down though one more time the last thing I'll talk about cuz I already have 30 minutes last thing I want to talk about was this check if ball and range check if have alums both of these first off they're a little bit confusing in the name so it's doing a check if it's not returning anything it's not saying really like what it does and if you look at it you see it's actually toggling an interaction collider and a balls in range variable still not sure exactly why or what it's doing that for let's see let's search the balls in range if it's interactive and has balls and range and it will send them to the holder what by here and here interesting so I think what I do is just rename this to update ball is in range yeah that's really what it's doing it's updating that ball is in range or maybe I'll calculate and update because it's really doing the calculation to figure out if it's in range and then setting a variable here one other thing though like this tags I hate tags I talked about it a couple times before the big problem here is that if a tag breaks it's very hard to tell what the problem is it's hard to track down and find the issue you're not gonna get a compiler issue if somebody deletes the tag name to ball you're not gonna get a compiler issue or an obvious issue if somebody removes the tag from the ball this coats just not going to work and it's gonna silently fail so what I prefer to do is look at this ball collider and this is happening in an update even then at this case I'd still probably just do a ball collider here getcomponent what is a ball control so check to see if that is not equal to no so that's just gonna check to make sure that this is actually a ball it's the ball it's not the tag again I know people like using tags sometimes I've just run into problems with them enough times that I kind of try to abandon them avoid them just not use them it's just way too easy to break things and spend a day she's trying to figure out what the hell went wrong and then find out it's a tag on some object somewhere deep in a hierarchy that nobody realized was being said except for the one person who had made that change six months before and anyway turns into a mess so a little bit more code I always better in my opinion and then I do the same thing here for this check if have lumps just whatever call this calculate if we have lumps and update yeah something like that I don't know I'd probably spend a couple minutes just coming up with a good name for this but I definitely want to name it something really obvious for what it's doing although I don't know necessarily that we even need to be doing this because really the Lum slots where is this at whenever we're actually attaching these we should probably just be setting the number of them that are attached to trip to increase or decrease or something or just toggle a boolean therefore whether or not we have them so when we add one just check and say hey it's definitely true now when we remove we just check to see if we're out and then set it to false instead of doing this in an update not a big deal but it would help it help simplify the code a little bit and I like that you're doing some gizmos that's awesome gizmos are very cool anyway um I don't want to you know tear under the code too much more I said I just want to do a kind of a light quick review and I went on for half an hour just kind of going on around first again I just want to say that I thought the game was really cool it's pretty it's nice it plays well I definitely see it turning into a complete full-fledged game the code while it has some issues it's functional it seems to perform okay didn't really run into too many big problems I would definitely go about just refactoring things renaming anything that's not really obvious so don't be afraid to use long long names so obviously it gets obnoxiously long and you can't read it in a second that's it's bad but no it's much better to have it I say I'll take a long name that I can't read in a second over a short name that I can read instantly and have no idea what it is any day so ideally we want to go somewhere in the middle though yeah four or five words at most anyway I do that split some things out and definitely I excited to looked at it's more to these classes to really give great feedback but it they look for spots where a class is doing more than one thing where it's violating that single responsibility principle and pull those things out just yank them out make a new class it doesn't have to be a monobehaviour can just be a class that does some calculation or something I mean a lot of the methods in here from what I know today I saw could probably just be static methods on a static class that have a zero state at all that are just doing some kind of a calculation passing in data or they could just be smaller methods on smaller classes that are a little bit more specific like the lung stuff and you've got a couple different classes already here I'm Holder alum control I love the size of this class I don't know if it's used but I love the size there so yeah just just work on splitting things more and also very good that you're using object pooling that's great pooling is the way to go for this kind of stuff yeah spawning lots of things going to pool anyway I'll quit rambling on I plan to do a couple more of these I got a good number of submissions that came in from email I might send out a request in a couple weeks just asking for more to go through but if you're interested in code reviews and you know just having somebody talk about your code and tell you the problems with it Philly's problems I see personally or just give you some feedback let me know drop a comment below and I'll maybe do these more often if they're if they're popular and if people find some value in them alright that's all I got for today so thanks for watching don't forget to Like share and subscribe all the fun YouTube stuff and have a great

Channel: Jason Weimann

Views: 14,833

Rating: 4.9315591 out of 5

Keywords: Unity, Unity3D, Unity Code, Code Review, Unity Code Review, Unity3D Code Review, Unity Project, Unity Project Review, Architecture Review, Game Code, Game Code Review, Real Game Code, Unity3D Code, Unity3D Review, Unity Game Code, C# Code Review, Real Code Review, Project Review

Id: gTGVGcN-Kko

Length: 34min 4sec (2044 seconds)

Published: Sat Mar 17 2018

Reddit Comments

[u/awhellnogurl]

Ooof you'd have a lot of fun with my messy code.

[u/[deleted]]

Amazing series, this has always been my favourite learning method, keep them coming.

[u/Christoph680]

I think there's some good points to your video (using proper formatting, human readability, etc.), but there's also some pitfalls that should or might be avoidable especially by beginners:

• You reformat the same-line if statement telling your viewers that human readability is key here, but you should also take note of the fact that if statements without enclosing braces should be avoided at all cost as they can result in unexpected behaviour further down the line when adding or removing statements from the if clause.
• You mention extracting getting the direction to the ball into it's own method. Instead of creating highly specialized methods that might not be reusable, think of using generalized methods like 'GetDirection(Vector2 from, Vector2 to)' and passing the cursor and ball positions as parameters. Also, this could be a great place to introduce extensions methods to convert a Vector3 to Vector2 without setting x and y explicitly everywhere in the code.
• In addition the letting your viewers know about proper naming schemes, you could also let them know of the availability of enforcement of these (or any) rules by most IDEs. It's one less thing to check if it's already enforced by the IDE itself.

Apart from that, great points and very good tips for such a short amount of time! Keep it up :)

[u/iemfi]

Awesome! Probably the first tutorial video I've seen where you can clearly see that the creator is experienced and knows what they're doing.

I think you should have spent the time to show people how to pull out the class though. I think a lot of people find that to be the hard part so end up with monster classes.