[5SD023] Code Review
|
Hello team 4! My name is Alex and I’m from team 3, I have been reading thru your ”player object” class’s source code. And I will now give you my opinions how you could decouple the class even further. But before I started to talk about hte actual code I would like to write about you repository. First, I cloned the ”master” branch and tried to compile the code, but couldn’t since there was syntax errors in your latest commit. This is something that you never should do in a branch, since you will just give the other developers problems they shouldn’t be dealing with. I would understand if the application would crash in some cases, especially if the team only uses one branch. But syntax erros is a big no no. In my opinion. Next, my code review about the acutal source code, while I was looking for the file(s) where the ”player object” is declared and/or defined, I found a few files which you should probably be looking into. I found a few header files, which probably has been generated while you were merging 2 or more commits together. All started with the name ”player_” and all had the same revision number. Now, the ”player object” declaration, there is alot of variables, if possible maybe you could split this class into two. That way your player class won’t be responsible for as much as it’s now, it might just be a personal preference but I think I should share it. Also at the top you have forward almost all classes you need which I really think is a good choice, this way you aren’t coupled to as many physical files as you potentially could be. When I read thru the definitions, I found that you take a pointer to a sf::RenderWindow in your constructor, which I think isn’t that great since the window is used to render too. Also following the render window trail I can see why you did it this way. But I think it’s better that you send the mouse position from the ”check input” function to your newly instantiated object. Also reading your function names such as ”GetPowerUpSound” I think you could be a bit more descriptive, since this function doesn’t return a value maybe you coud call it ”PlayPowerUpSound”. And lastly remember to free all of the memory in the deconstructor. Otherwise good work, and I’m looking forward for your beta presentation! Sincerely Alex  |