CHESS! // Code Review

hey what's up guys my name is ashana welcome back to another code review the series in which you guys sent me your code and your projects and i take a look at them if you have a cool project you'd like me to take a look at then check the description below there's some instructions on how you can send it in today we're going to be taking a look at something a little bit different than usual we're going to be taking a look at a chess clone now i'm not really sure what like a chess clone is because like chess is not like a video game i guess i think this is just a chess game because the the original chess board game i guess if you made a board game that would be a chess clone right anyway i'm not sure how many of you are fans of chess i am a fan of chess that's why this kind of stood out to me i don't think anyone sent me uh chess before okay uh i stand corrected but anyway i guess julian really lucked out here because we're going to be taking a look at this today i'm a huge fan of your seatbelt series and your code reviews videos always helpful for me i'd like to thank you for that thank you i created a chess clone in cpus with sdl2 it basically initializes the field renders the pieces calculates possible moves and game states and prints out the winner at the end every piece has got its own class derived from a base class called peace lovely lovely object-oriented programming it works fine by the way that was like half a joke and half not i know that a lot of people like to bash on like object oriented programming i'm not really here to bash on this especially because this person is likely like a beginner let's just let's just dial down the hatred it works fine and i couldn't find any bugs yet but i'm pretty sure the code is not very well written okay cool here's a link to the repository which we will take a look at in a minute would be an honor if you could review my code and tell me a bit more about what exactly could have been written smarter and stuff like that okay so just general kind of advice i guess i'm 22 years old and a statistics student from germany who got into programming at my university i don't really i don't really care if my name is displayed on i'm not really sure if the build process works if you just climb the river my gosh everyone and the builds man what is it with you people and just making code that just builds i'm honestly considering just making a video dedicated to like how to build c plus plus code how to make a repository how to make a project and then just build it how to distribute that how to how to build i never learned stuff like that and it's much self-taught knowledge i'm already excited about your response well luckily i am responding hello again hello [Music] again i rewatched a code review of yours and made some mistakes previous people also did like not putting stl to into my project i tried to fix that messed up everything i'll text again when it works like it should thanks sorry these code reviews are just getting very boring hello one last time hello one last time after some painful hours of trying to set up the repo i think i'm finally done it should now be possible to just follow the steps in my readme and have a fully working project sorry for the spam and thanks for being patient all right so what we're going to do with today's video right is we're going to we're going to not go over the build process i have done this pretty much every single uh code review episode almost i've gone through and fixed the code given advice on how to make code that is buildable that will just work that you can just clone set up properly how to do that relative paths absolute parts literally covered all of that go through the passcode review videos if you are interested in seeing that i'm going to cut that out of today and we're just going to cut exactly right now to the code but first this video is sponsored by skillshare for those of you who don't know what skillshare is skillshare is an amazing online learning community that has thousands of different classes on literally every topic imaginable that is designed to help you learn new skills as quickly smoothly efficiently as possible they have classes on like literally everything if you're trying to learn more about photography videography how to make youtube videos illustration productivity marketing singing i could probably benefit from that skillshare have got you covered with plenty of amazing classes to choose from and one of the things that i really like about these classes is that they're not like you know they're not like lengthy they're not just stretched out they're very much to the point they're not optimized for youtube where like watch time matters or anything like that they are specifically their own platform designed to make learning as easy as possible for you and the cool thing is that skillshare is offering the first 1000 people who click the link in the description below a one month free trial of skillshare premium you'll be able to check out their entire library of classes and see if there is something there for you so definitely go ahead and do that sign up for the one month free trial and see if there is something there that floats your boat huge thank you as always skillshare for sponsoring this video and over here as promised we see the code does it run of course it runs did i have to spend hours figuring out how to run it of course no i'm kidding it wasn't like hours it was like maybe 10 minutes but anyway we're here so f5 and this is the game this is the chess clone so uh i'm assuming white moves forward yes just just checking just checking relax i know how to play chess so we click okay so we have to click and hold i guess and then we can drag this over here uh okay let's play with game of chess uh what are we thinking like bomb cloud opening probably okay so the knight seems to move as normal i mean i'm just going to try and i guess evaluate the pieces i don't even know how i should be testing this let's see oh can't castle okay so that's that's the problem we definitely want to be able to castle i'm assuming well if we do it that way she's going to move the rook yep can't castle also i'm noticing the extreme kind of blurriness of the pieces for some reason we'll check that out in a minute as well but otherwise i guess it's a pretty functioning game of chess again apart from the lack of castling i'm just gonna go ahead and try and like check mates let's see what's a quick little checkmate i can do i guess what we should probably do is yeah like let's take a piece okay that works yep yep this is pretty cool and quite functional um what i was going to do uh let's test the check right so here's a check uh can i just move the bishop no i can't it doesn't say check or anything that's okay i can move the king uh can i block yes i can block maybe like move this here move this here for a little check okay why doesn't check that doesn't indeed seem to be working this has just become like a chess video really i mean i'm not sure what you guys expected who needs c plus code i am entertained would it be nice to have like some kind of chess ai uh but i guess this will do i guess i'll retreat from my bong cloud i guess we should uh no let's go ahead and be aggressive okay fun time is running out that was actually a mildly annoying move well actually it doesn't matter he doesn't have to take it let's just say he goes here and we can uh let's do this this and that okay white wins there we go so it looks like the checkmate detection works i'm pretty satisfied to be honest the only thing missing again is castling would be lovely to castle i don't think it's really possible to play a game of chess without like having the ability to castle so that definitely needs to be in there but other than that you've done a pretty good job uh i mean oh one thing we actually didn't test out is on poisson but i'm sure that that probably won't work if you can't even castle but let's uh let's reboot this okay so for this we're just going to advance something and then let's try this and oh so you did on poisson but you did not do castling wow okay anyway let's just dive into the code so the first thing i'm actually kind of uh oh quell data source files ah yes you mentioned you're from germany cool those of you don't know i went to school in germany for a while so i know german kind of decently but i've never kind of interacted with computers well at least programming wise in german so this is kind of interesting to look at because means like like a kind of a spring like a spring of water that's how i've kind of used that word before so it's interesting seeing it as literally like source here we go this is this is what i'm talking about that that is what i think of when i look at your source code all right anyway so uh mainlobe.h mainly brown i love this i love how clean this is let's get rid of that bright light i love how how clean this is um it's nice you know you have a static static function here which again you know like not too bad um you know when you have something like this class main loop static void run from what you mentioned how like everything it looks like you know bishop king knight all these things are separate classes you know i can see that you're very object oriented like literally you're you're oriented around object you're used to using objects but in class possibly don't have to do that and for something like this right again i would recommend that maybe it would be better to just have like a static void main loop doesn't have to be static at all um maybe maybe chuck that into like a namespace called chess or whatever the name of your program is but otherwise i would just have something like this and then implement it in the thing static is really only if we want to have it like within one translation unit which we don't really or i mean i guess if i'm explaining i should explain it properly static in this context kind of means that it's going to be local to a particular translation unit which is basically a cpp file so if we include this header file in multiple files they're basically going to have their own version of this main loop function or if you just include it in like a cpp file then this will not be a global function it will only be visible inside that one kind of cpp file but anyway obviously since we are just doing the definition in the cpp file and the declaration here we can just leave it at that i would do something like that instead right and then obviously this would look something like chess main loop right or you could even call it chess run or whatever you're kind of just moving the main function over there it's not it's not a huge deal or anything but i guess i would just encourage you to think about like the fact that you don't really need classes why like why is this a problem well again it's really not a problem but you could do interesting things like this right which doesn't really make sense and i mean you can technically write code like that as well even though it's static so i think in general it could be a cool exercise for you to kind of uh try and sway a little bit from the path of the objects anyway let's take a look at this uh kind of main run loop first of all i really am digging how clean the code is like honestly like there's no fluff anyway it's just like main run then we have a run function and then here we have everything you know sometimes i really like code like this for simple applications like this chess game why because instead of having like an init function and then like in it this other thing i mean we do have a handler in it over here but like my point is instead of having a billion functions and then it being really hard to navigate everything's really really clear and that's always something we like to aim for okay sdl handler we make a new instance of this and then initialize it again i have a feeling straight away that this should not be a heap allocation but let's take a look at this i mean there are quite a few things here it doesn't really matter though okay so public so const and screen width and screen height 640 by 640 fixed would be nice if it was resizable and then we have a window we'll be rendering to while you're even using the m underscore if you're using like m underscore and all that stuff i'd probably make sure these variables are private i'm not sure if you use them outside of this uh you might i guess yeah i mean use m event here the whole kind of way that i like to use that m underscore convention is for basically private member variables so if something's public i'll usually just slap it in like that um just so that it's kind of clear but private members i like to prefix with this m underscore variable again just a personal opinion of mine uh nothing actually physically wrong with this it's just more of a code style window we will be rendering to service contained by the window renderer event like mouse etc this is kind of nicely uh annotated initialize the field i'm not sure what that means i guess you may initialize these things the fields um returns true if everything worked okay clean up um destructor so you know this is one of those things where i would probably do the init in the constructor and then the cleanup in the destructor because you can kind of take advantage of objects right this is a good good time to take advantage of objects speaking of taking advantage of objects one thing you could also do because i'm sure that you call handler clean up at the end is you could get rid of this not call cleanup at the end but instead just make this a stack variable like this and you're going to be taking advantage of r a i i resource acquisition is initialization which basically just means that your constructor is going to initialize stuff and your destructor is going to de-initialize stuff and that's that process is going to be automatic because it's going to revolve around the whole way that stacks work inside c plus plus i think i have a video about like stack uh stack stuff like stop using stack scopes scope lifetime something like that i'll have that linked up there definitely check that out you did mention that you've seen the simplest series so maybe you've already seen that video it goes into more depth about what i'm talking about but anyway the way that we would set that up here is by basically creating a constructor as well as that destructor that we've already got what does the destructor even do nothing right so this is going to be perfect and then we would just call cleanup from here and then call in it from well i'm kind of did that the other the other way around but we would call clean up from the destructor init from the constructor that means that again we don't need this and we don't need the cleanup that i already removed right and then basically this handler arrow becomes handler dot then we'll just replace that everywhere in this file uh and then we're basically good to go this game seems to take in a pointer so we can just give it the memory address of this let's double check to make sure that all of that worked but it should and it looks like we are good to go so this is a great example of where it's important to not allocate memory on the heap all the time not only because you then manually have to delete it which by the way now that i think about it you were never doing right so uh you didn't have like a delete handler at the very end right which you need to do i'm assuming you also haven't done that for game so these are all kind of memory leaks they're not really big deals in this case just because uh you know this basically lasts for the lifetime of your whole application so i don't know maybe you're like super cool and you've deliberately not deleted that i would delete it anyway just in case the destructor is running certain code but also just just to be kind of clean right so in this case you would come over here and then you would type in delete game because if you've manually allocated something with new then you have to destroy it with delete but again you know instead of doing that you could use smart pointers by just writing something like scd unique pointer which is which kind of uses the same concept as this it just wraps this kind of allocation in a class that will be automatically destroyed at the end of the scope right because the unique pointer itself is allocated on the stack um and then you would just uh do std make unique and then pass along the arguments like so right so by doing that what it means is everything else remains the same but it just means that the destructor gets automatically called you don't have to hold delete and by destructor i mean like the unique pointer class itself will call delete on the um object that is inside it right so game has a new kind of game object inside it when this unique pointer is destroyed when it goes out of scope it has some code in its destructor that is basically this right so it will in fact call that for you okay moving on quit so critical is false and that's basically while quit is false uh i personally don't like to write code like this usually you'll see code written like this a lot more often right so instead of actually doing equal circles false or equals equals true you just write quit again no real difference it's just more of a stylistic thing that most people like to do so i would rewrite that to that um and then what do we do sel poll event and uh oh okay that's cool this is nice so this game is entirely event driven meaning this isn't like your typical like let's render frames as quickly as we can or at vsync meaning like 60 times per second or whatever your monitor's refresh rate is instead of doing all of that this is basically entirely event driven meaning that the game does nothing until you like click on something and then it re-renders the screen so this is really cool actually that should mean that it's extremely like efficient however i don't think it is the reason being that you're using sdl poll event in a while loop in a well this is the big while loop in in a while loop constantly right without sleeping or doing anything so what this actually means most likely if i'm not misunderstanding this is that you probably have like a hundred percent cpu usage just in that loop so let's go ahead and open up task manager and see what it says about our chess app okay power usage very high right so the power usage is very high uh cpu usage is about five percent now i have a lot of cpus like as in i have a lot of cores i've got 16 cores 32 threads but you can see i mean the cpu scheduler is moving this around uh actually let's go to let's make it just stick to one core so if we go to details and then just set the affinity uh let's just clear this let's go cpu seven okay so now it's only allowed to run on that one core so if we go to cpu seven which is this one over here you can see how that is just completely maxing out right so power usage is very high and we are maxing out our cpu core that is running this we have a thread fully occupied doing nothing but checking for events and then of course if i was to close this then suddenly this core is going to be able to relax okay so how do we fix this well might be more simple than you think if we change polyvent to weight event which i happen to know is another sdl kind of function that will actually sleep and release the thread while it's waiting for events and we run this again that one little little change if we go back here let's take a look at what this is saying now and check this out very low because nothing's happening right and then if i should still be able to move and everything should still be fine but again very low 0.1 percent and then if we come over into our cpus let's just make sure that that affinity is still set okay let's go ahead and set it again i mean it doesn't really matter core seven you guys kind of get the point then you can see that this is our core seven it's now doing probably other stuff because we are in fact doing pretty much nothing and we're using like zero percent cpu usage right so that is a much better way to go than using 100 usage doing absolutely nothing but point being my my main takeaway from this is that it's cool that you've done it in a loop because i know that a lot of people would just kind of implement this as a game and again i guess there's not too much wrong with that because it probably wouldn't take more than like a millisecond per frame anyway and then it would just v-sync it would just wait it would just kind of sleep but doing it kind of event driven like this is just cool because we haven't looked at anything like this before okay moving on so if handler event type is quit then we we set quit to true um i mean it's probably not worth doing anything else here i mean i quit quit probably would be the last event to be set so it's not a big deal but i would just kind of break maybe or like i don't know continue just in case you have some other code or something just because it kind of doesn't make much sense to me to to kind of say that quit is true oh this is this while loop yeah so i then i in that case i would definitely break right because although it doesn't make much sense to me to kind of say that okay quit it's true but then let's continue checking like all this other stuff right speaking of which this event type as far as i'm aware is only uh capable of being a single type at a time i don't think this is like well you're comparing it with equals anyway so i would probably make these else ifs just so you're not checking each and every single kind of case even if it's this one or i would just use a switch statement over like handler.m event dot type right and then you can kind of implement all of your case statements um like this with like a little break over here and then it would look something like this instead and you can add scripts around them if you have any problems with like variable names or anything like that so i would probably do something like this again you know a huge deal but if you do go with if statements because you prefer them or whatever i would just definitely use elsif for all of these just to make sure that you're not rechecking event states when you don't need to with the point being that it can only be one type at a time okay let's uh add that back in moving on so handling event type mouse button down and in that case we uh okay so divided by 80. i'm assuming 880 640 80 yes so 640 divided by eight is 80 and there are eight okay so we have an eight by eight grid i didn't actually notice that it had zero padding yeah it has zero padding right so this entire game area 640 by 640 each cell because they go from they they fill up the entire bounds here of the window they're 80 pixels each um and so what we're doing here is we're just basically converting this mouse uh button kind of x position so where we clicked we're just dividing it by 80 to get the coordinate of where we are in the world right so in other words if this is like 200 pixels across or whatever if we divide that by 80 we're gonna get like what like two because it'll be like two and a half and then we'll round down to two so two is zero one two we know that we're in this cell so this is simply working out the kind of x and y coordinate in this kind of grid cell coordinate system right uh and then it's called start probably because we're able to drag it and then that way we know where we drag it by also doing end inside this if mouse button up so when we release the mouse so this will render the possible moves um if clicked on is not null pointer where we are right clicked on right so then what this will do is then use these coordinates to retrieve the piece that is there so clicked on is now a piece right which is like a pawn or whatever that is what we're dragging and then if it's not null which again you can check just by doing that right this is also very popular in c plus plus other languages don't really support this but because pointers are just numbers you can check them as if they were like bullying expressions so if the pointer has a value then it's going to be true otherwise if a pointer is null pointer it's zero which means it's false right so this is just a trick that we not only a trick but it's something that we do pretty often instead of having to write null pointer but i totally understand if you want to write null pointer just because it does make a bit more sense to some people okay so if clicked on uh get team equals game get turn this prevents us i assume from moving the wrong team's pieces so we can't have two like black moves in a row or two white moves in a row and then if that's both true then we render possible moves and then this is going to calculate all of the possible moves and then re-render probably like the whole board oh no this will just sorry this is just showing the possible moves so this will actually um kind of fill in the squares that we're allowed to move into once it calculates all of these possible moves let's have a quick look at this code so okay so we're using a tuple here so vector of tuples in in okay so immediately i'm going to say i don't like this i don't like to use tuples why because they make no sense so this is pretty obvious move type but int and end i mean i can probably deduce this is like the coordinate of where you're moving but the thing is if you had used a struct instead so for example you might have had something like possible move as a struct and then you can have like x coordinate y coordinate and you know piece uh move type you know move type or whatever right this obviously makes more sense because you just have one of these it's clear what these actually are because they have names performance wise does not matter at all same thing right it's just more kind of from the code side to be a little bit better and then when you use them see the problem is you're using std get so instead of doing that guess what we would be doing we would be doing a value dot and then what we want so like x coord instead of std get one which who on earth knows what that means its value dot y coord right so hopefully you can see how much better and more clear this code becomes now now it is also possible to use structured bindings with tuples so if you do actually really want to use tuples and not make a struct out of it which again i would i would make a struct but what you can do is actually use structured bindings which basically just means that what we can do is stick in some uh names for this here i don't know why i got rid of const but uh you know like x chord by quart and uh move type we can actually stick that in over here right getting rid of this value of course and then we just basically are expanding these but also giving them names so this just becomes that right and then this becomes that so at the very least you can do something like this if you are using c plus 17 or newer because that is a relatively new feature but again i would just use structs in which case everything's fine even if you're using c plus whatever c plus plus 1900 okay anyway i'm gonna undo all of this so that we can still compile the code without me changing everything um but other than that i don't really have any problems with this uh you know you're being a little bit inconsistent with your um curly brackets here you know if you're using if statements and functions as next line you should do the same for for loops not a big deal obviously just something i noticed uh set render and draw color and then doing all of this stuff so it's candle screen width divided by eight again getting the coordinate um and or rather creating the rectangle uh from the coordinates so we're taking the chord again you see what i mean like this is so hard to read like this is the x coordinate std get zero value that is the x coordinate this is just terrible like don't it would make much more sense if you had x chord or something like that here or at least x c even though that's a bit cryptic maybe x coordinate you know just x that would make much more sense so that's kind of what i'm trying to communicate here otherwise the function doesn't really have a big issue so you're getting the screen width dividing it by 8 which gives us 80 and then we're multiplying the coordinate bytes so we're going back from the kind of coordinate space which is what these two values are into screen space or into like pixel space so that we can actually figure out what the bounds of this physical rectangle is made out of pixels should be and then of course this is just going to be um like a width and height of 80 by 80. so i'm interested in what like because the remember the um the pieces were blurry right so let's see what size the pieces are let's see if i can find them here so we've got an image here i can't open the files the png file man okay so i found the pieces and it looks like they are 60 by 60 and you're rendering them as 80 by 80. so what i would do is just find some high resolution art right have some high resolution pieces because otherwise it looks bad or you can like reduce the resolution of the game which is probably not good as well i'm like crying from these allergies so let's try and find uh where was this sdl like handler i think yeah so if we make this like 480 by 480 is that right six times 66 60 times eight yep so if we rerun that now then yeah so that looks really crisp okay so um get some high resolution images high resolution textures 60 by 60 pixels is tiny like what are you doing get some high resolution pictures okay so that's the render possible moves i mean we didn't really finish this function but it's pretty simple all we're really doing is uh rendering like filled rectangles here and then we are rendering what's this rendering so this is going to oh so it is going through the entire board and rendering it again okay sure so we are in fact re-rendering i guess everything uh we rendered this rectangle which looks to be like the possible move and then we go through all of these and we render and fill which is the piece so we render the pieces on top i guess sure say my name say my name no peace here okay um if handler is not pointing sure again this is probably a good place to use asserts instead of just this because that would help you through development instead of having to watch the console like a hawk to see if something's gone wrong you would get a nice kind of uh like a break point basically appear here that you could just use to debug the current state of the application otherwise yeah um and this is another kind of see first second like you don't want to be doing that like a pair position of the piece make a struct chord like point or something that just has two integers in it x and y and use that that way you can use x and y instead of having to be all weird and use um you know first and second x and y would make much more sense right so structs are your friend okay so pulling back into into the main kind of loop here um when we do mouse up it's going to undo render possible moves okay right so that's going to i guess render the board but without the possible moves that's a bit weird wouldn't you just render like when you just render the whole board normally because again this looks like it's getting the possible moves so what exactly happens when we do mouse up ah this doesn't work because yeah that's another thing right so all of your logic all of your like you know the mouse dragging code for example all relies on the cell being 80 by 80 which you know isn't a huge problem but you should use a variable for this right just like you used a variable for like the width and height of um of the screen right so you can see screen width and you know i mean eight you can just keep using i guess because it's not like that's going to change probably unless you have some weird variant of chess maybe with more kind of um uh tiles or cells right but like i don't like the divided by 80. so you should probably have something like cell width whoops cell width which will be like you know 80 uh and that way you can easily change that to 60 or even better you can just do spring with divided by eight right because you know that's going to be that size so i would definitely make a constant out of that because otherwise you know if you do change the resolution like this then it's going to be an issue but furthermore it's not going to work if you actually make the window resizable like i think you should okay so back to blur town if i drag this here then yeah it moves this here but again i'm not sure why you don't because you definitely like you obviously have something that renders the board so why why would you not just why do you have to undo that kind of rendering i don't really get it where even am i so undo render possible moves because after this if it's a valid move oh well i guess maybe let's just quickly see what happens here maybe this doesn't happen when i think it does no it does so what if what happens if we comment it out okay right so the whole board doesn't get refreshed we missed this so it must not in fact be the oh because it's the background i see so you render all the pieces on top but the background you basically never render okay that makes sense um i'm not sure if i i'd probably just re-render the whole background to be honest and not have to worry about undoing this but i can see what you're doing now you're you're replacing that kind of uh like greenish turquoise color or whatever with like white or with the dark square whatever it should be for the particular possible moves and then you're rendering the pieces on top of them so see this is another thing i would do i would bring this out into a function like render pieces or something right um that will just go through and do that that way you don't have to duplicate the code multiple times like you've got it here and then you've got the exact same code here and then i'm assuming maybe in some other places as well in general of course we try to reduce code duplication as much as possible so it's easier to maintain and change code as time goes on because you don't need to hunt it down everywhere and see what it is okay so x um and we've gone through that clicked on it so if we've clicked on something if it's not not null pointer um then we figure that out again yeah these states this can just be combined right so you can just move this down here it's not like xn does needs to be calculated before that or whatever that would just kind of streamline this a little bit more if clicked on get so if it's if it's the same team you have a lot of if clicked on get team equals get turn um like literally you have this everywhere if you see yourself kind of using this multiple times i would definitely like take it out um you know is valid move or whatever um i would definitely like take it out and actually uh kind of you know into a variable so you can use it multiple times easily without having to write that same code multiple times but even better since you use it in really a lot of a lot of places i would probably bring it out into a function called like is valid move for team or i don't know is valid move and just kind of use that instead because that would just kind of save you from you know checking to see if the current team if it's the current teams go is current is teams go or something like that i don't know i mean what are you checking game get turned and clicked on get team right so is team's turn i guess would be good all right um yeah and then what do we do so game is valid move so how do we how do we calculate these valid moves so peace get possible moves okay so this is where we actually are getting into the hierarchy of our pieces so we have a base class called peace um and then that has like a particular team that it's on it's got a piece type uh so one thing i would do with these enums is i would it looks like you have a lot of these kind of like empty or none you have these like zero states but they're at the end of the list uh what i would do is actually make them first because this gives them the value two which doesn't make sense to me like i think that none should be zero uh and then you should go up right like that right and same with this like you should have empty as like zero a lot of times i actually explicitly write zero just so that it's clear you don't need to it will be zero anyway unless you start it with some other value like negative one or something it's just kind of like equating i guess uh empty or none with false and because false is also zero so that's why i like to do that um and then you've got again you know these possible moves which as i mentioned should be potentially a struct you're returning this by value which i wouldn't do either because it's a member variable here i would mark this as const and then have this be a const reference that way you are not copying a vector and dynamically allocating memory you want to kind of avoid those memory allocations where possible uh and again you know this is like an event driven game so realistically you'll probably never see any performance issues with stuff like this but it's still good practice like for the future uh team get team sure set new position uh okay again i wouldn't make that a pad make it a struct we have a constructor for all these pieces right so i'm assuming you're calling this like however many pieces there are what are how many pieces are there i think like six per team right like 12 different paces i think so okay copy constructor sure copy constructor why do you have copy constructor are you copying pieces for um this should uh i would probably just mark this as a default unless you're doing because it doesn't look like you're doing anything special right and then in that case you can just get rid of this because it looks like you're literally just saying this i mean i guess you're you're not copying the texture though you're just setting the texture now i'm not sure if that's an error but for some reason you're not and you're sending this to false so usually usually unless you have a good reason for this usually obviously copy constructors would copy um so maybe that is special i'm not sure destructor rendered this piece prints name of peace calculate possible move so i'm interested in this okay so let's go into like a let's go into something uh basic like a pawn so what does that do okay so in the pawns constructor we load the file so i'm hoping you don't call this eight times or rather 16 times eight for each side um but we basically uh load okay so pd pdt for black so i guess dark and then light light theme for um for white we take in this handler so this is kind of um this kind of uh object-oriented design i'm not really a big fan of either because you're you're taking this handler which uh you looks like you then don't use anywhere so i'm not sure why you even need it in the first place but it's kind of creating this like uh you know it's almost like a circular kind of dependency i mean it's not gonna cause really any problems it's more of a design issue right because the handler should be kind of well then again it's the sdl handler it's not like you're taking in the game right i actually kind of thought it was like the game so you wouldn't want to take that in because you don't necessarily want the pieces to be aware of like the game the game like the meta game and everything the meta game should contain the pieces and it should be the one giving them instructions um but this is a sdl handler which is specifically i guess like the rendering stuff right uh troll rectangle undo piece render load image all right it needs it for loading image up here which is again fine but i wouldn't assign it to a member variable because it i don't think you need it um i mean other pieces i guess might use it but pawn certainly doesn't seem to be okay calculate possible moves so i guess this will go through and actually figure out like which moves are valid so let's see how we do this so it's checking like the current position plus the delta and the delta is probably right mdy direction the pawn moves oh okay so this is not like uh okay i see this is like a fixed kind of delta so this is just immediately a quick check to see um whether or not like we are actually within the bounds of the board because zero is like the first tile and seven is the max tile so this is actually an interesting check because this is the thing that will promote us uh that's one thing we didn't check actually right because i guess the idea is if that plus d y is in fact a zero then we've reached the end of the board and we should get a queen that's one other thing i didn't check let me check that quickly okay so let's try and uh get rid of this we'll just uh step aside and let this oh let's check and let this uh win oh look at this this is a pleasant surprise nice yeah that's pretty cool okay so that works as well which is nice so this is just again calculating the possible moves but i guess the move type here obviously becomes a new piece move because that's a promotion for the pawn um so that's a quick little check but again like it's only possible to do if there's nothing in the way here right so this is us checking to see this next position that we've checked is in fact the promotion kind of the base uh rank kind of position so that's kind of an easy uh i guess well this is these are pawns in general so they're kind of easy this is just a normal move i guess if we're um if that's not the case again we're just checking to see if it's uh if it's empty or not uh but then i guess this is where it gets interesting one thing i want to mention about this code so far is that it would have really benefited from some annotating right you used comments a lot before in places that maybe are like a little bit meaningless but then over here you know where like check for promotion or whatever you don't have anything like this so you really need to you really should use comments more like this right because this code is a little bit it takes it takes a second to understand so this is like the whole has moved thing which i'm assuming uh well actually looks like see this is public so it's some it's hard to check but i'm assuming the house move thing is the thing that gives you the two kind of the ability to move two squares forward if you want on your first move anyway i'm not gonna spend time doing all this here's a little on poisson kind of uh uh section here which again is a little weird because uh like this has a bull in it so if true ampersand is possible in the inter direction so again this is set by kind of external forces which um you know makes it a little bit harder to kind of sort through but i guess that's probably necessary because it's not really up to you whether or not you can do that so you can see over here inside the game if there's just uh i guess some kind of pawn move that's happened then we can set that to true in negative one direction anyway like a lot of chess is obviously just really specific rules and i don't think that i need to kind of go through all of this and explain or try and explain to you how it's implemented it's all pretty like standard it's all pretty much done by hand right just kind of filling in valid moves like if we look at the knight for example you can see that we basically check for a grid like grid kind of adjacent squares in a grid around it right so that we can kind of just see if there are um empty squares there or i guess squares with the opponent's pieces that we can actually move into there's kind of two rounds of this as well one which focuses on like the x and the y because this only iterates through the for loop a total of like four times i think and this does another four but in the other directions then we get impossible moves here so this doesn't actually return them it just calculates them and then we get the possible moves here and then we can retrieve them uh by calling like get possible moves or something up here this kind of code is always tedious to do um i can see that you've done it like using for loops which is fine um i probably wouldn't have done it this way just because if you take a look at these for loops they're pretty useless in the sense that what they're doing is they're checking two states you're basically checking d y is minus two and then d y is two because you're adding four right so this is checking minus two and positive two and then this is checking minus one and positive one so what i would do instead is i would actually create some kind of like array of certain moves again i would use like a x y point class so let me just uh you know break this down here so we have x y we have an array of points and these are the possible squares or whatever right and you would need to obviously check this with the bounds of the board as well but you could do something like this and then you can just define these things right so i'm not sure so i guess in this case we would have four but then this also has four right so in total the knight has eight kind of squares that it can move to so maybe i would do something like this and then you can just list them here minus two two two two i mean this was for the y but i think you kind of get the point minus two two and then for the y again for the y we're supposed to have minus one and one so we can do minus one one and then we duplicate this and then this time we can have something like that right and that's four of the possible moves and then all you have to do is kind of iterate through that so possible squares and then let's just pretend we did actually have four let's just add that there um and then let's just go through all of these moves possible squares and then that's kind of that's your for loop done right and then you can combine them and just do them all in one because it looks i mean looking at this code the way that you're calculating this like it's all identical right lots of code duplication there as well so listing something like this out would be helpful and then if you were really cool with your comments what you could also do is attempt to actually draw out a board that would explain like what these points are so this is just me being extra cool here but you could basically say that like the knight originates here and then what we're doing we're going minus two and then minus one so we can move here and then same but like plus one right and then same but positive two right so this kind of fills in these moves that the knight can make but then also you would have like the other four that i mentioned we would also need to implement which are these four right and then this kind of is a nice nice visualization of what in fact we're doing here and if you were even cooler you could actually label these so that you could easily kind of validate them and i mean i don't know like none of this is not this is necessary but it might be kind of cool to just look at you know kind of visualize what you're actually doing i don't know maybe i should make like a little chess game in hazel or something that'd actually be a cool project for the game engine series kind of hazel 2d but anyway uh that's pretty much all of the code um again all these move things you know textures rendering uh this just draws a rectangle with that texture again that's where that kind of distortion is happening because you have the 60 by 60 texture then you're drawing it to a destination rectangle that is 640 divided by 8 which is 80. right so you're just kind of stretching it out a little bit you're getting all that blurriness um but other than that i think you've done a pretty decent job at everything the code is relatively clean uh there's a lot of again use of like heap allocated objects where they don't just they just don't need to be right i would like to see a bit more you know stack allocated stuff again you don't really need unique pointer here that was just an example i would make it like that probably um and then wait actually where do you make all of the pieces i haven't really seen that part yet have i while i'm looking uh for that i also noticed this so calculated possible moves again takes in piece pointers and then an eight by eight two dimensional array of them so this is kind of confusing i probably wouldn't do that i'd have like uh probably like an scd array of like 64 of these i like one dimensional arrays more just because they're less kind of indirect when you're dealing with them uh but you know this might be easier i guess um but again you know if you're dealing with one-dimensional versus two-dimensional arrays it's still there's an easy kind of way to convert that right you just do like x plus y times width so it's it's super simple um but anyway let's let's try and find those pieces being created so over here in init is where you uh create all the sdl stuff again looks pretty simple and here it is the game class okay so yeah this is like a lot of stuff i mean you could have you could have definitely used some for loops here um because you know you're creating identical pawns for example here i mean obviously for some things you can't use for loops but i would do that this is also once you've created the pieces i'm glad that you kind of store the pieces somewhat permanently so you're not like recreating them all the time that's nice not that it's a bad thing to recreate them but again if they're heap allocated objects like that as well as long as they're like fixed i guess it's not too bad if you're doing that in initialization but yeah um you know going through all these then manually assigning them to like all the different cells a little bit tedious a little bit annoying but more or less um oh actually wait okay so you're creating individual pawns yeah so this is again like a situation where you know we don't need eight pawns for each side right we really just need one pawn and then we need to like render it many times and also keep track of eight instances of it but we don't actually need eight pawns so if i was to do this i'd do it a bit differently especially because every time you create a pawn each every time you create eight of these white pawns you're loading the image repeatedly and you're storing like a reference of it here right that's completely unnecessary as well they all look the same you should kind of take eight instances of them um you know when you're rendering we'll actually have one instance of it and then just render it eight times for example but then obviously you need to have some kind of lightweight kind of piece object or something that is just like you know this is the piece this is like what team it belongs to this is its coordinates and then uh well that's it right the the type of the piece and then then that's it that's all you need so i definitely wouldn't wouldn't set it up like this uh and then you calculate all moves this so i guess these are like the starting moves or something wow but then yeah you're going through okay so this is all of the moves possible for the current pieces on the board okay so you kind of do that like up front i guess but yeah that's uh that's pretty much it hope you guys enjoyed this code review if you did please don't forget to hit the like button below don't forget that you can send in your code to china review gmail.com there will be like some instructions in the description below so definitely check that out i never know what i'm gonna like review until i just open up that email address and i just get you know all these messages there and then i just pick so it's always a blast always a fun time and never really know what's coming next if you guys also have some like opinions about this code definitely share them in the comments section below but other than that thank you guys for watching don't forget to check out skillshare using the link in the description below and i will see you next time goodbye [Music] you

Channel: The Cherno

Views: 92,017

Keywords: thecherno, thechernoproject, cherno, c++, programming, gamedev, game development, learn c++, c++ tutorial, chess, c++ chess, chess in c++, code review, c++ code review, chess game, video game, making games

Id: WKs685H6uOQ

Length: 51min 5sec (3065 seconds)

Published: Fri Nov 05 2021