Code Review - Merge (Love2D Android Game)

in this video we are going to do a code review we are going to look at the code of a game called merge which was created by immo this game is available on google play store if you want to check it out the purpose of this is to take a look at the code that is behind this game and try to find some interesting things that we can look at and hopefully improve and make cleaner and better code now this video is a very different type of video than what i usually do so i would love to hear some feedback down in the comments or you can join our discord server and let me know there alright let's have a look at the actual code one interesting thing which i'm actually glad is here is the fact that this require here does not use parentheses like these others do now this is actually lua syntactic sugar where if you send in a single string or single table you can omit the parentheses however i really only see this ever being used when it comes to the require which i believe leads a lot of people to believe that this is a specific require thing that require is special a keyword that has this magic ability where it does not need parentheses this is not the case requires simply a function and you can use any other function in a similar way so for example i can say print hello world just as well i can use a function which i have declared myself for example player set state dead and this will also work i would however say that unless you have a specific and good reason you should stay away from this and simply use it like a normal function an interesting thing is that this is the only require which sets itself to a local variable while all of these will be registered in the global scope and it is a little bit ironic because settings is actually a in my opinion good candidate for being something global because this is a file which for example stores some abbreviations so you can type s.lg instead of love.graphics and s.l.r instead of love.filesystem.read which just makes your lines shorter can be good i personally don't use this but to each their own the other thing that this file does is that it houses all variables like the center of the screen and other constant values like what specific colors do this m.byte is actually something that i needed to look up because i had never seen it before but basically all it really does is it divides all of these by 255 so that they become in the range between 0 to 1. anyway it has a lot of constants and this is you know some of these constants maybe should be in other files and so on but yeah basically this out of all of these files i feel is the one which you can make the best argument for keeping global while you probably want to keep all of these local one very interesting thing is that it sets a lot of values to be equal to nil and this actually accomplishes nothing i can remove these and run the game and everything will work just as before so you may think that that's unnecessary since it doesn't actually do anything however i quite like this i actually do this often myself because this is a way of letting me and other programmers know that this is a value which does not exist currently this is a variable uh if we're setting it to be equal to nil but at some point later on this will be set to something so i know that this is going to exist eventually now some of these variables like the difficulty i would probably just set to a default value such as easy or normal but sometimes you have values where you can't set it to be something beforehand and i prefer to set it to be equal to nil over for example just omitting it because it becomes a little bit like documentation let's return to the main.lua file and have a look at the rest of the structure so in the love.load file we load a bunch of things there isn't really all that much to talk about here but if we look at the love.update you can see a fairly primitive but effective way of doing game states so we just have a variable and we check if that variable is currently set to be equal to game and if that is the case then we will do all of the updates that game requires while if the game state is set to something else well then we do some other things now handling game states and all that is a fairly big topic which i intend to cover at some other point but in general this is a fairly good way of doing it if you just have a simple simple game and you don't really want to spend too much time and energy on it then this is absolutely fine i used to do variations of this before one very interesting thing is the semicolon here now a lot of programming languages will require you to add a semicolon at the end of each line to signify that it's a line break now in lua the semicolon doesn't actually do anything i believe it just reads as an empty space however it does have a use case and that use case is this if you have a line which contains something which normally should be on separate lines you can add a semicolon to make it more clear that this is one thing and then here's a new line and this is another thing and so on if we take a look at this first line here in the update function that is a thing which is put there in order to make sure that we don't have too big steps of time so if somebody has a huge lag spike where their delta time is greater than 0.2 it will simply skip that update and try again now this threshold is actually set to fairly high and i think one of the reasons for that is that if you put it slightly lower then the game will simply stop updating for people with low fps what i like to do myself is to instead of return here is to actually set dt to be equal to the amount that we're checking so if delta time is greater than 0.2 then we set it to 0.2 this allows me to actually lower this threshold a little bit so we can say for example 0.1 this means that regardless of how hard you're lagging your game will always progress at least a little bit so if you have moments where you have very low performance your game isn't completely frozen which might make people think that something's happened that the game has crashed there's a big difference between this update function and the draw function because in the draw function it's more anarchy all of these are being called all the time and then instead we pass in the game state to allow them to check if they should draw themselves now i would say that the most important thing is that you keep consistent regardless of what system or what methods you use you should do it the same way so i would have liked to see this method being applied down here because in my opinion this is far superior to the anarchy which is going on down here speaking of game states one thing that really sticks out is this thing called state now that sounds like sort of game state territory so let's have a look at that this game state is actually in charge of the game itself so it checks if you have lost and it checks if you have one and then it does a bunch of things and basically keeps track of how your game is going as i was looking through this file one function stood out in particular and that is this draw function here we can see that it takes a argument named bool which actually isn't a boolean it's a string so it should be named string if we're going to name the data type however i think that it would be better to be more descript and say that this is the game state now obviously like i said it would be better if we didn't have to pass in this variable to begin with but i'm not going to be changing the entire game structure just simply try to make small incremental changes to make things better now this draw function is in charge of displaying the text that you see when you win the game it basically just prints out some text and it calls another function which is called pop-up background and this is just a rectangle now the interesting thing is that this wind text is being set here to be equal to well done and it's set to other things here like you've completed the tutorial and you've completed all of the something difficulties but apart from that this here is similar to this while this is identical to this so we have a great opportunity to refactor this and make it a lot smaller to do that i'm going to create a new function which i will name draw wind text this function will take one argument wind text then i'm going to select all of this code here cut it out and paste it into the function instead of first declaring these variables local and then setting them i'm going to remove them and just set them to be local as i declare them now this background i'm going to place back up here because it has nothing to do with drawing the text next i will remove the wind text equals to because we can't hard code what it's going to print instead we are going to get it through the argument now all we need to do is simply call this function and pass in the string that we wanted to display we can do the exact same thing over here just replace the assignment with the function call and then we can go ahead and remove all of this code and we can go ahead and remove these as well so now we end up with a function which is both shorter and easier to understand what actually is going on now this line here is actually really long fortunately there are a couple of tricks that we can do in this kind of situation and the reason this line is so long is because we have a lot of arguments that need to be calculated and in this situation i like to break that out into local variables which i can then pass in now this significantly reduced the line width this isn't even the longest line in this function anymore and at any time we can look at this print here and see okay what does x do well we can look that up and it's much easier because when you have a really long and wide function call it can be very difficult to keep track of which argument you're actually on and that can lead to errors where you try to modify for example the y argument but you end up modifying for example the x here however it's super easy to add in whatever other things you need to do there's another function here which is worth to take a look at and that is the function named check win now the function name check win indicates to me that it should simply observe if you have one and probably return true if that is the case otherwise return false however it falls into the classic trap of doing more in general it is a great rule of thumb to make sure that your functions do first of all what they say they do and second of all preferably one thing this first part here actually does what the function claims it loops through all of the tiles in the game and looks if any of them isn't equal to zero and if that is the case then it will just return because in this game the tiles are set to zero when they have been merged so any tile which isn't zero indicates that it hasn't been merged and you haven't won yet however this line here saves that you have completed the level so it does more than just checking if you have one it actually sets that you have indeed one these lines here they reset the moves so the move count how many moves you've made and also this game keeps track of what moves you've made so that you can so that you can revert a move and go backwards so it just resets that moves table then we have this code block down here which basically just looks if you have finished all of the problems in one difficulty so it first checks that you haven't completed all of these before then it loops through all of the problems of that difficulty and checks to make sure that none of them are actually equal to false because if that is the case then finished all problems is set to false and it breaks so that it isn't set to true because you can skip levels here so if you hadn't done level 5 but you have done level 6 then it would set finished all problems to false and then in the next iteration it would set itself to be equal to true so basically this function does a whole lot of other things apart from checking if you have one and this is something known as side effects and that is generally really bad because you don't want to call a function that you believe checks something only to have it perform a bunch of other things that you had no idea would happen so let's actually break this up into multiple functions to remove these side effects like i said this top part kind of does the whole checking if you have one so we'll leave that in and cut out all of the other things i want this function to return a boolean so i will create a new local variable and i will set it to be equal to true by default because we will begin with the assumption that we have one and then we are going to loop through all of the tiles again checking if any of them isn't zero and if that is the case then instead of returning we're going to set our boolean to be equal to false if this boolean is ever set to false then there is no need to continue looping because we can never recover from that it will always be false so we might as well break here then at the end we're going to return this variable now we can call shekwin and we will get a boolean result of if we have one which to me is much closer to the actual name of this function next i will create a new function which i will name one game this function will be run when you actually won the game we will move this first line inside of it because it just sets this level as complete and that feels like something that we should do in the one game function i will create another function which i will call state reset moves this one will simply reset the moves in turn i will call this function inside of the one game so that when we win the game we reset the moves finally we have this code block that needs a home and like i said it basically checks if this was the first time that you completed all of the problems in a set difficulty so let's create a new function which i will name completed difficulty then i will move the code inside of it now if we take a look at how this function actually controls things we can see that it sets booleans to be equal to true or false so there's a lot of this going on in this project where the sort of control and structure and flow of the program is determined by booleans and i actually used to do this a lot when i was a beginner i just had a million of different booleans and those would just control everything i would just flick them on and off and i would decide what is running and what would trigger and and all of that the problem with this approach is that it's kind of difficult to know what all of these booleans actually do where do they lead where are they used and what's the purpose of them so this is one thing that i want to kind of change a little bit that we break away from this boolean dependency and have a different kind of structure and flow which decides what is being run and when first of all i will move this piece out of here because this has nothing to do with the completed difficulty this boolean is in charge of triggering another function after a set amount of time so like i said the booleans here have a lot of power now as a first step of breaking away from this boolean dependency i'm going to move this finished all problems boolean to be set in the one game and i will set that to equal the result of this function then we don't need to set anything here all we need to do is actually return false because if one of these problems isn't complete well then we can't have finished the entire difficulty now we can't return true here because then it would just return immediately when it finds something which is complete and that's not what we want instead we will return true after the loop is done because if we never return false then we have a situation where we have completed everything there is a third case which can happen and that is if the difficulty actually already has been completed before and in that case we will just return false oh and i just realized that i moved this run function to the reset moves i wanted to move it up here into the one game well now we have a situation where we're just setting a local variable to be equal to a result of a function and in this situation we can instead go to where this is actually used which is here and instead of using the local variable we can simply call this function and it will return true or false instead of checking if this boolean is true or false now this actually means that we can get rid of this boolean we no longer need it since we use that function which means that we have one less boolean to worry about here in the update function we can see some more of these booleans that are being set to equal false here i want to get rid of these as well and in order to do that we need to look at the timer function which is being called here because it's clear that these booleans are supposed to be set to false after a specific amount of time and at first this timer call looked very odd to me i didn't quite understand how it could constantly call this which i presumed was a callback function in a update function and still have it work i thought that it would create a new timer every frame but this time actually works slightly differently if we go into this file named libs which is where the timer lives we can see how this actually functions here we see a local variable which keeps track of the time that has elapsed and then in here we increment it by delta time which we pass in we also pass in a timer length which is what it checks against and once the time weighted is greater than the time length then it resets the time back and returns true now this approach to a timer is very different from anything else that i've seen but it has a major flaw and that is that you can only ever have one timer active at the same time because if you ever run into a situation where two things are using this timer it will basically double the speed because two things are incrementing this same variable and then whichever of these have the shortest length is going to trigger this first and return true and that can lead to all kinds of strange and difficult to debug problems and we need a solid timer to be able to get rid of our boolean dependency so i'm actually going to go ahead and create a new file which i will name timer.lua here i'm going to create a very simple timer class when we create a new timer we want to take the duration of it as well as an anonymous function which is the callback then instead of returning this new instance that we're creating here i'm going to create a new local table which i'll call active and we're going to insert this timer into that table then i will just create a function which will loop through this active table and update all of the timers then in the timers update function we are simply going to decrease the duration as long as the duration is greater than zero after decreasing the duration we can again check the duration to see if it is at zero or below in which case we should trigger the callback after we have called the callback we should go ahead and remove this timer from the active table finally we need to actually return the timer at the bottom now we have created a new timer class and the main benefit of this one over the one that we have in libs is the fact that this one can have multiple instances running at the same time without us having to experience weird bugs and very difficult to debug problems in order to be able to use this we need to go to the main.lua file and actually require this and then we need to call the update function in the love.update similarly to the sounds always updating i will also leave this timer to always update regardless of game state because this is sort of a universal thing that should always be running now let's return to the state.lua and also require the timer up in the top here because we will be using it here and more specifically we will be using it to try to deal with this situation here where this old timer function is used in the update function so it's continuously called and i will just begin by trying to fix this first chunk of code i'll cut this out and let's go find out where this run function variable is being used well that's being set to true in our one game function i will paste this code here just for guidance so i can keep track of what i'm actually supposed to do and then we will try to recreate this using our new timer function so the first argument is the duration which we will just nick from there and then we will pass in an anonymous function inside of this anonymous function we're going to call the result that happens from this old function and all this really does is that it wants to call this function run finish check win after 0.8 seconds so we'll call it in here now we can go ahead and remove all of this code because this timer accomplishes the exact same thing now let's return to the top and do the same thing for these two so we'll cut out this wind draw and then we'll search for windrow and more specifically where we set it to be equal to true that is inside of this run finished check win function and again i will just paste this here for reference again i will call the timer.new passing in the duration and then the function in this case we're going to set windraw to be equal to true and then after two seconds we will set it to equal false now we can remove this temporary code and return to the top only one of these remain and i'll do the same thing cut it out find where it's being set to true and paste in the code here this is going to be very similar to this down here so we will actually set this variable to be equal to true first and then after that we will call timer.new and pass in 10 seconds then we will pass in the function and then we'll just move this code inside of the new timer instead and again we're able to remove this old code now we've actually managed to make sure that our update function is completely empty and no longer uses that unsafe timer we've now reworked it to use our new and improved timer now we've changed a lot of things in this state mainly with the one game and check win functionality we need to go find where shaquin is being called and update that slightly so that the game actually will function now the check win function is actually called inside of tile.lua and more specifically the tiles mouse released callback function now down here we see two calls to state first of all check loose and check win the reason that these two calls are here is because this is the mouse released call on the tile and this basically just checks if two things have merged and a whole bunch of other things and then it sort of evaluates if you have won or lost which might make sense then that like okay i i've merged a tile so we should probably check if we have won or lost however this leads to increased coupling of the files state and tile we have a situation where a lonely little tile has the power to call the big state and tell it that the game is over this is something that we kind of want to avoid so to accomplish this i'm going to break these two functions out and go back to the state.lua now there is no mouse released callback in state so we'll create one and then we will move these two function calls inside of it now inside of the callback function in the tile.lua we can see that it checks and make sure that the button is actually one so only when you release the left mouse button should we actually check this so i'm going to add that in by saying that if the button is not equal to 1 then we return now naturally this is going to be called every single time the player releases the button and when it was here in the tile this was only ever being called when there was a real possibility that you had actually won however i feel like this is a small trade-off compared to the decoupling that we're doing and obviously if we had completely reworked everything we could probably have both but i feel like this is a good middle ground since we changed the check wind to just be a function which returns true or false we need to wrap this in an if statement so if we have actually one then we will call the other function one game and since all of these functions are inside of the state mouse released which has a colon we can replace this with self now to make this work we need to head over to the main.lua file in the mouse release callback function we're going to add it in here and to keep the same style as we have over here we're going to use the semicolon to show that this is a new thing and then just paste it here now we can go ahead and run the game and test that everything works like intended now let's return to that function in the tile.lua the mouse released because this is another interesting function where i feel like we have a lot that we can do to this to make it easier and just cleaner but first we need to understand what it actually does first of all it will check if the number is equal to zero and if that is the case it will return then it will also make sure that if it's not dragging then it will return as well because there is no reason for us to check this if we just clicked and released and then we also make sure that the button that we released was the left mouse button then we set the tile drag and self dragging active to be equal to false i don't know what the difference between these two are they feel sort of similar so that would be interesting to find out next we declare a local variable which is the target id and we get that from a function which basically according to this comment says that it checks the neighboring tile in the direction which you drag the mouse then we do a bunch of different checks here and according to the comment we make sure that the tile actually can be merged so we make sure that we have a target id and that that target id isn't nil and that the target id number isn't zero this is actually the first thing that i want to tackle instead of having a kind of complicated check here that does a lot of things we can break this up and just say self can be merged then we can create that function here in the bottom and inside of this function we will simply return that condition so now we will get true or false depending on if we actually can merge these tiles the target id is actually a local variable so we need to pass that in to be able to access it similarly we need to add it here into the function call now we can go ahead and remove the comment because our code tells us what this check actually does and at any time we can go down here and verify what exactly that means now both of these statements will actually return if they validate true so instead of doing this on two different lines we can bring this up to the same line and separate them with an or next we look that the button is equal to one and if that isn't the case then nothing else will actually happen so that means that instead of checking if the button is equal to 1 we could just say or button is not equal to 1 and then we will just return this means that we can get rid of this if statement and indent all of this code one step down in general you want to keep the indentation as low as possible because it becomes more and more difficult to keep track of what does what now we have a similar situation here that we had down here where we're checking a lot of conditions so i'm going to do the same thing i'm going to break this out into a function instead and this function is going to be named should check merge because it will check if we even should be bothered checking if something has merged or not and then down here i will go ahead and create that function inside we simply return it now the name suggests that it should return true if we should check it however this will return true if we shouldn't check it an easy solution would be to wrap this in a parenthesis and add a not at the beginning to flip it but the problem here is that we get this not not business which is just kind of entangled and not very nice so instead we're going to go through these and flip them so instead of checking that the number is equal to zero we're going to say that the number is not equal to zero and that we are dragging and that the button is equal to one then we just need to set these ores to be equal to and instead so now we have the reverse that if all of these are the case then we should check merge instead of if either of these are not the case then we shouldn't that means that we can simply add a knot up here instead which makes this line read very clearly that if we shouldn't check merge then we won't this function is quite large it does a lot of things and i want to kind of call the herd and reduce it one of the things that sticks out is that we get four local variables here and then we have four code blocks here which are all basically identical they do the same thing the only difference is that they use a different local variable so this one uses l this one uses r u and d to fix this we can head over to this get neighbors function which is right above here and look at it and this function basically returns the neighbors of a tile and if you pass in all as the argument then it will return all of its neighbors as separate variables what i'm going to do is wrap this in a table that way we can say that instead of saving these as a bunch of different local variables we can say one table which i'm going to call neighbors now that we store them in a table we can go ahead and create a loop instead where we loop through each of the neighbors inside of the neighbor table and then we can just take one of these and put it inside here and instead of using this l local variable we'll use the neighbor variable the result is that this loop here accomplishes everything that the old code blocks did so in other words we can go ahead and remove all of these now i noticed a tiny issue and that is that we're checking against button here which is an argument that we get from the mouse released callback so to make it actually work we need to pass that along into the should check merge and also add it in as an argument now if we go ahead and run our game after reducing this code we are going to get an error and that error is because this function that we changed the get neighbors that's actually used in this state at the checklist function so we need to give that the same treatment i'm going to instead of using these local variables i'm going to say neighbors and then we have the same situation where we're checking the left right up and down which instead can be replaced with a loop i'll go ahead and move this inside of the loop and then we'll replace the left with with neighbor then we can go ahead and remove the other checks here now this did the same thing where it first declared these variables as local and then assign them so we can go ahead and remove this and then we also need to make sure that we add the local keyword to the neighbors and that should basically fix this function so if we run the game then we can see that everything is still working just like it should let's go back to the tile function again because while we've reduced it significantly it actually now fits on my screen there are still things that we can do to reduce the code mainly this code block here and this code block the interesting thing is that upon closer inspection we can see that this function if it runs sets neighbor check to be equal to true and if that is true that is what decides if this chunk of code is actually going to run or not and that isn't really clear it actually took me a little bit of time to figure out what this all did and how it was connected so one way we can make this more clear is to go ahead and break this out into a new function this function i'm going to name neighbor check basically inspired by this variable name which we are going to replace at the top here i'm going to make a new local variable which i will name found it will be false by default and then instead of setting neighbor check to be equal to true i'm going to set found to equal true then at the bottom i can go ahead and return the found variable now this neighbor check function can be used instead of this local variable here so i can just say self neighbor check now we have managed to do two things we've made it more clear that this function here this code block here decides whether or not this is going to actually run and we have also reduced the size of this function which is starting to become fairly manageable as a last thing i'm also going to break this out i'm going to cut this and create a new function which i will name tile merge and then i'm going to call this function in here so if this neighbor check returns true then we call this merge function now there are a couple of things that we need to do in order to make sure that our two newly created functions actually function first of all they both use the local variable target id this target id is locally created here and we need to pass that along the second thing is that the neighbors check uses the neighbors table and that is created here the good thing is that this neighbors table is not used anywhere else so we can just move that down to the neighbor check all we need to do in order to fix the target id situation is to add it as an argument to both of these functions and then just make sure that we pass it along now we are able to run the game and see that everything functions still just like it should do and i even managed to win well done now i'm actually kind of happy with this function it's very much shorter than what we began with and it has been simplified and just basically reduced in many ways and refactored and i'm also kind of happy with this video so far i feel like this is a good point to sort of stop i could go on and on obviously there's a lot of things to look at but this video has been going on for quite a while and yeah if you've watched until now thank you very much that's awesome please let me know in the comments what you thought of this video this is a new format that i'm just trying out and i would love to get some feedback on how you felt that this worked i mean this is obviously very different from a lot of the other videos that i do which are heavily scripted so here i'm more improvising as i go along if you would like me to take a look at your game please contact me on for example discord we have a discord link down in the description so you can join there and send me a dm alternatively you could just write down in the comments or send me an email or something but yeah i had a lot of fun making this video and i hope that you enjoyed watching it and hopefully you also picked up a couple of tips and tricks along the way and just in general learn something please leave a like and subscribe so you don't miss the next video and i'll see you then [Music] you

Channel: DevJeeper

Views: 545

Keywords: programming, coding, lua, love2d, game development, love 2d, code review, fixing your code, lua code review, android game love2d, android game lua, love2d mobile game, code review love2d, how to learn programming, learn programming

Id: c_tLdo8RmjM

Length: 42min 39sec (2559 seconds)

Published: Mon Oct 25 2021