14 replies to this topic

[superman3275]

I've been hard at work this week programming breakout again, but this time I'm putting heavy emphasis on programming using object oriented techniques. So, I am having my Block/Ball/Paddle class all handle their Variables, while I have a GameLogic class that handles all the logic(Collision Detection/Updating the other classes/Etc) and a GameDraw class which Draws all my objects(Just draws them to the screen). Then I have a MainGame class which has an instance of GameLogic and GameDraw, an instance of the other classes I need, and it's responsibility is to run the game. I spent a lot of time Pre-Visualizing how I was going to set up my classes, and what their responsibility's would be. This project abides by the single responsibility principle, and the Open-Closed principle(I hope, I really have a weak understanding of it).
Here's the link to the .zip fie containing my project:
http://www.mediafire.com/?r7l6hfxf8dh8c37

Edited by superman3275, 13 October 2012 - 08:11 PM.

[superman3275]

And I didn't link the code! FacePalm!
I'll have that up really soon!
Here's the link to the .zip file containing my project:
http://www.mediafire.com/?r7l6hfxf8dh8c37

Edited by superman3275, 13 October 2012 - 08:12 PM.

[SiCrane]

First off, if you're going to share your code, don't include things like the contents of the Debug/ folder, the intellisense cache or other extraneous compiler generated files. Otherwise, there doesn't seem to be enough completed to comment on the overall design. There are a number of implementation details that seem non-kosher, however. From a quick glance through: you seem to have some vectors that you never resize, and it seems like you don't intend on resizing them. In that case you should consider using std::array instead of std::vector. Also you seem to use a lot of non-const references when you only need const references (e.g.: Block and Ball's constructors). You also have a number of functions that return copies of objects rather than references to objects. Ex: Block::GetImage() could probably be changed to returning a const reference. There also seems to be a quite a bit of redundant computation going on. For instance, why store ball position as a member variable if you can get it from the sprite object?

Also, GameMain has protected member variables. If a class has non-public variables that implies that the class has a class invariant. With private variables only the members of that class is responsible for maintaining the class invariants. However, if you have protected member variables then every derived class is also responsible for maintaining the class invariants. This is generally a lot more trouble than it's worth.

[superman3275]

What you said about GameMain I removed. For the redundant computation: Later on in my code, in GameLogic/GameDraw especially, I have some code that would be almost impossible to read unless I have those variables. They're to help my program make more sense later on. Also, GetImage() is never used, but that's a good idea. This is stuff I'll have to remember later on.

[SiCrane]

For the redundant computation: Later on in my code, in GameLogic/GameDraw especially, I have some code that would be almost impossible to read unless I have those variables.

You can make functions with the same names that grabs the values as needed which will give you equivalent readability. See also Don't Repeat Yourself (DRY).

Also, GetImage() is never used, but that's a good idea.

You might want to get out of the habit of writing functions that are never used. See You Aren't Going to Need It (YAGNI).

[AlexB.hpp]

GameLogic::CheckCollision() -- this if is freaking terrible. After 10 minutes reading I don't get it.
Why do you every time repeat this GetIsHit()? And also this freaking LogicBall->GetCollision().Intersects(LogicBoard[index]->GetTop())... It would be nice to refactore it to

void GameLogic::CheckCollision()
{
...
  if (logic_ball->GetCollisionMgr()->Intersects(logic_board[index]) && LogicBoard[index]->GetIsHit() == false) {
	...
	logic_ball->UpdateVelocity(); // this func knows that GetCollisionMgr()->Intersects(...) have cached info about intersection and know how to apply velocity
  }
...

I hope I've gave you some ideas about how to improve your code.

And also I suggest you to use pointers instead of references. It's a little bit flexible.

Edited by AlexB.hpp, 14 October 2012 - 03:51 PM.

[superman3275]

Your interface Manager idea is smart, and I might implement it later. Thank you for your input, just please be less harsh on people in the beginners section.
I dislike how you called my code "freaking terrible", I'm posting in the beginners section and your post made me get Effective C++, Code Complete, and C++ Primer Plus.

Edited by superman3275, 15 October 2012 - 02:35 PM.

[superman3275]

This code is officially dead, I'm going to try something simpler before attempting breakout again. My interface is awful (As the guy above pointed out), and I didn't plan my code at all. I'm going to try to become a better programmer by taking a smaller step, because I bit off more than I could chew.

[Black-Rook]

superman3275 You can use sites like http://www.codeplex.com/ to post your code, it's much easier for us to access.

I didn't look over all the code, but AlexB.hpp it would appear he called the GetIsHit() each time so it doesn't check a block that has been hit? I would suggest once you hit a "block" you end the for loop. Does index refer to the 19 blocks you can hit? Sorry if I'm reading the code wrong.

Also, using references is perfectly fine! I personally prefer to pass by reference.

superman3275 don't be so hard on yourself, I learned how to program from trial and error. If it wasn't' for the thousands of times I had to reprogram the same code, I wouldn't even be able to program half the stuff I do today. I do however recommend working by setting small goals to reach your big goals. I'm doing this on my RPG project posted in my Journal, and it's coming along perfectly.

Edited by Black-Rook, 14 October 2012 - 08:19 PM.

[superman3275]

That's smart! Black-Rook, the GetIsHit() function is exactly for that. IsHit makes it so that I don't draw blocks that have been hit and I don't check for collision on them either. I read a post about how you should set small goals and focus on getting stuff up on the screen to motivate yourself, and I think that sounds smart. As lazyfoo.net said, I'm entering the realm of pre visualization, where I have to actually think about my code before programming!?!?! It's unheard of. Thanks for your input

[Black-Rook]

I have a whiteboard, well a few whiteboards! I will often draw out what I want my code to do before I even write a line. You can do the same on a piece of paper as well, or even in MS Paint with a tablet, or mouse.

Also you should start to use these:

//

/*
*/

Regardless of who reads your code, you need to make it a practice to comment code.

Example:

// Add two values and return the sum
int Add(int a, int b)
{
	return (a + b);
}

Note: /* */ is for multiple lines, example:

/*
This
is
all
comment code!
*/

So you don't have to use // each line.

[Serapth]

AlexB.hpp, considering your new to the forum I'm not going to get mad at you, but who are you to be talking like you're an expert in C++? It doesn't matter if pointers are more flexible then references, I don't need pointers and I don't plan to use their harder to understand syntax. Also, you should probably realize I AM USING A LIBRARY CALLED SFML AND OMG I'M USING BOUNDING BOX COLLISION. That's why I can't "refactor" my code, and that's probably why you don't understand this:

LogicBall->GetCollision().Intersects(LogicBoard[index]->GetTop())

If you actually used SFML you would know what was going on. You also just signed up for the forum, so I suggest you stop telling people in the beginners
section that their code is:

this if is freaking terrible

I'm in the beginners section, do you expect "your" quality code.(That's considering your an actual developer, which I doubt you are.)

I'm saying that if Collision(A bounding box rectangle) intersects one of my blocks I set that block to being hit. If you knew what you were talking about you might understand that.

BUT,

Your interface Manager idea is smart, and I might implement it later. Thank you for your input, just please be less harsh on people in the beginners section.

Wow, reading this response officially makes we never want to help you, ever.

Generally when asking for criticism, it is bad form to jump down someones throat for giving it. You came across as a world class d-bag there.

For the record, the Manager is a bad idea, it adds a layer of unneeded, and most probably, not fully understood abstraction to a design. You are in the learning phases, adhere to the KISS principle of Keep It Simple Stupid. That said, even after you move beyond the learning phase, any class named "Manager" should be a code smell of sorts, setting off your spidey-sense that you are creating a bad design. No, Manager!=Bad, at least, not always, but it very often is.

Also, references should pretty much always be preferred over pointers, until you have a very good reason to act otherwise.

Edited by Serapth, 14 October 2012 - 09:02 PM.

[ultramailman]

Hello superman. I have read your code, here is my opinion.

Why not implement GameLogic, MainGame, and GameMain as normal functions? Assuming you are only going to have only one instance of each.

There are some magic numbers lying around, like these:

Board.resize(20);
for (int index = 0; index < 20; ++index)

What do those 20's mean? Do they mean the same thing? It would be good if you can assign them to constants, it will make life easier for you if you suddenly decide to change that number. It will also make the code easier to understand.

Also, more comments would be great, I think. Preferably a comment for every method/function and variable in the header file, and a comment for the very long expressions and statements. This will be help the people reading your code to understand them better, and also remind yourself what the functions/methods do if you forget in the future.

This code is officially dead, I'm going to try something simpler before attempting breakout again. My interface is awful (As the guy above pointed out), and I didn't plan my code at all. I'm going to try to become a better programmer by taking a smaller step, because I bit off more than I could chew.

I think you should continue with it. It is good way to get better at coding, as good as reading tuts and taking lessons. Start from coding the small objects, like Ball, and make them as close to bugfree as possible. Then work your way up to the logics and graphics stuff.

Edited by ultramailman, 14 October 2012 - 09:31 PM.

[AlexB.hpp]

AlexB.hpp, considering your new to the forum I'm not going to get mad at you, but who are you to be talking like you're an expert in C++? It doesn't matter if pointers are more flexible then references, I don't need pointers and I don't plan to use their harder to understand syntax. Also, you should probably realize I AM USING A LIBRARY CALLED SFML AND OMG I'M USING BOUNDING BOX COLLISION. That's why I can't "refactor" my code, and that's probably why you don't understand this:

LogicBall->GetCollision().Intersects(LogicBoard[index]->GetTop())

If you actually used SFML you would know what was going on. You also just signed up for the forum, so I suggest you stop telling people in the beginners
section that their code is:

this if is freaking terrible

I'm in the beginners section, do you expect "your" quality code.(That's considering your an actual developer, which I doubt you are.)

I'm saying that if Collision(A bounding box rectangle) intersects one of my blocks I set that block to being hit. If you knew what you were talking about you might understand that.

BUT,

Your interface Manager idea is smart, and I might implement it later. Thank you for your input, just please be less harsh on people in the beginners section.

Sozz dude, looks like I was a little bit mean.

I'm not an expert but I'm trying to get some experience too. So that's why I wanna give you a little review.
Would you agree if guys will give you some feedbacks on your code to improve it. Your code looks really difficult to understand.

Did you just begin or not, it doesn't matter, 'cause you HAVE to write good code.

Let me tell what I call good code. Good code - when you send it to another programmer with enough skill, he can understand and modify it after few hours of reading.
Sooo that's why I said that that some part of your code freak me out. But ofc your code can be OK.

My suggestion - keep dev it, 'cause you've got nice feedback on it.

[superman3275]

AlexB.hpp, considering your new to the forum I'm not going to get mad at you, but who are you to be talking like you're an expert in C++? It doesn't matter if pointers are more flexible then references, I don't need pointers and I don't plan to use their harder to understand syntax. Also, you should probably realize I AM USING A LIBRARY CALLED SFML AND OMG I'M USING BOUNDING BOX COLLISION. That's why I can't "refactor" my code, and that's probably why you don't understand this:

LogicBall->GetCollision().Intersects(LogicBoard[index]->GetTop())

If you actually used SFML you would know what was going on. You also just signed up for the forum, so I suggest you stop telling people in the beginners
section that their code is:

this if is freaking terrible

I'm in the beginners section, do you expect "your" quality code.(That's considering your an actual developer, which I doubt you are.)

I'm saying that if Collision(A bounding box rectangle) intersects one of my blocks I set that block to being hit. If you knew what you were talking about you might understand that.

BUT,

Your interface Manager idea is smart, and I might implement it later. Thank you for your input, just please be less harsh on people in the beginners section.

Sozz dude, looks like I was a little bit mean.

I'm not an expert but I'm trying to get some experience too. So that's why I wanna give you a little review.
Would you agree if guys will give you some feedbacks on your code to improve it. Your code looks really difficult to understand.

Did you just begin or not, it doesn't matter, 'cause you HAVE to write good code.

Let me tell what I call good code. Good code - when you send it to another programmer with enough skill, he can understand and modify it after few hours of reading.
Sooo that's why I said that that some part of your code freak me out. But ofc your code can be OK.

My suggestion - keep dev it, 'cause you've got nice feedback on it.

I'm sorry, it was late at night when I made that post. I guess I was annoyed because of all the time I had invested, and when you criticized me that harshly I just felt like all my work was for nothing.