Single Responsibility
#Single Responsibility
OverallOverall, I see one really big thing that I think needs to be addressed before anything else can be addressed. Namely, your class is doing way too many things. A class should have one single purpose. It should avoid relying on other classes as much as reasonably possible. This is known as loose coupling. A class that figures out positions and angles of a ship should not have to know anything about windows or mouse buttons. As such, I'd change the prototype for void angleShipToMouse
to something like this:
void angleShipToMouse(const sf::Vector2f& mousePosition)
I'd then remove the logic that gets the mouse button state and checks whether a button is pressed and whether the position is within the window bounds and put those somewhere else in your program.
Do as few transformations of data as necessary
#Do as few transformations of data as necessary SeveralSeveral places in your code you're converting between degrees and radians. There are several problems with doing this:
- It becomes difficult to figure out or remember which units a variable is in, particularly if it's named something generic like
angle
orrotation
. - It's inefficient in both the space the program takes up and the time it takes to execute. It might not matter here, but maybe it will later.
- It can introduce inaccuracies. Again, they may not be noticeable in this code, but they could be multiplied later.
In general, all code other than code which displays values to users is probably going to want to operate in radians. All math functions in the standard library use radians. You should probably be using radians internally and only converting to degrees for display.
Also, when you do want to convert between degrees and radians, you should write a named function to do it and call that function rather than always writing the values inline. It's easy to mistype one of the values and someone not familiar with the transformation might not immediately understand it looking at the code. If you had functions named degreesToRadians()
and radiansToDegrees()
, it becomes immediately obvious to anyone reading the code what you're doing.
Avoid magic numbers
#Avoid magic numbers
InIn moveCircle()
you have the value 400
twice in the first line. What does it represent? I'm guessing that the window or view this is in is 800x800 pixels, so (400, 400) is the center? If so, you should at least have a constant defined for that. I recommend something like:
const sf::Vector2f kWindowCenter(400, 400);
Or better yet, pass in the point you want to rotate around! What happens if you want to let the user resize the window (or view)? What if you want to rotate around a different point?
#Single Responsibility
Overall, I see one really big thing that I think needs to be addressed before anything else can be addressed. Namely, your class is doing way too many things. A class should have one single purpose. It should avoid relying on other classes as much as reasonably possible. This is known as loose coupling. A class that figures out positions and angles of a ship should not have to know anything about windows or mouse buttons. As such, I'd change the prototype for void angleShipToMouse
to something like this:
void angleShipToMouse(const sf::Vector2f& mousePosition)
I'd then remove the logic that gets the mouse button state and checks whether a button is pressed and whether the position is within the window bounds and put those somewhere else in your program.
#Do as few transformations of data as necessary Several places in your code you're converting between degrees and radians. There are several problems with doing this:
- It becomes difficult to figure out or remember which units a variable is in, particularly if it's named something generic like
angle
orrotation
. - It's inefficient in both the space the program takes up and the time it takes to execute. It might not matter here, but maybe it will later.
- It can introduce inaccuracies. Again, they may not be noticeable in this code, but they could be multiplied later.
In general, all code other than code which displays values to users is probably going to want to operate in radians. All math functions in the standard library use radians. You should probably be using radians internally and only converting to degrees for display.
Also, when you do want to convert between degrees and radians, you should write a named function to do it and call that function rather than always writing the values inline. It's easy to mistype one of the values and someone not familiar with the transformation might not immediately understand it looking at the code. If you had functions named degreesToRadians()
and radiansToDegrees()
, it becomes immediately obvious to anyone reading the code what you're doing.
#Avoid magic numbers
In moveCircle()
you have the value 400
twice in the first line. What does it represent? I'm guessing that the window or view this is in is 800x800 pixels, so (400, 400) is the center? If so, you should at least have a constant defined for that. I recommend something like:
const sf::Vector2f kWindowCenter(400, 400);
Or better yet, pass in the point you want to rotate around! What happens if you want to let the user resize the window (or view)? What if you want to rotate around a different point?
Single Responsibility
Overall, I see one really big thing that I think needs to be addressed before anything else can be addressed. Namely, your class is doing way too many things. A class should have one single purpose. It should avoid relying on other classes as much as reasonably possible. This is known as loose coupling. A class that figures out positions and angles of a ship should not have to know anything about windows or mouse buttons. As such, I'd change the prototype for void angleShipToMouse
to something like this:
void angleShipToMouse(const sf::Vector2f& mousePosition)
I'd then remove the logic that gets the mouse button state and checks whether a button is pressed and whether the position is within the window bounds and put those somewhere else in your program.
Do as few transformations of data as necessary
Several places in your code you're converting between degrees and radians. There are several problems with doing this:
- It becomes difficult to figure out or remember which units a variable is in, particularly if it's named something generic like
angle
orrotation
. - It's inefficient in both the space the program takes up and the time it takes to execute. It might not matter here, but maybe it will later.
- It can introduce inaccuracies. Again, they may not be noticeable in this code, but they could be multiplied later.
In general, all code other than code which displays values to users is probably going to want to operate in radians. All math functions in the standard library use radians. You should probably be using radians internally and only converting to degrees for display.
Also, when you do want to convert between degrees and radians, you should write a named function to do it and call that function rather than always writing the values inline. It's easy to mistype one of the values and someone not familiar with the transformation might not immediately understand it looking at the code. If you had functions named degreesToRadians()
and radiansToDegrees()
, it becomes immediately obvious to anyone reading the code what you're doing.
Avoid magic numbers
In moveCircle()
you have the value 400
twice in the first line. What does it represent? I'm guessing that the window or view this is in is 800x800 pixels, so (400, 400) is the center? If so, you should at least have a constant defined for that. I recommend something like:
const sf::Vector2f kWindowCenter(400, 400);
Or better yet, pass in the point you want to rotate around! What happens if you want to let the user resize the window (or view)? What if you want to rotate around a different point?
#Single Responsibility
Overall, I see one really big thing that I think needs to be addressed before anything else can be addressed. Namely, your class is doing way too many things. A class should have one single purpose. It should avoid relying on other classes as much as reasonably possible. This is known as loose coupling. A class that figures out positions and angles of a ship should not have to know anything about windows or mouse buttons. As such, I'd change the prototype for void angleShipToMouse
to something like this:
void angleShipToMouse(const sf::Vector2f& mousePosition)
I'd then remove the logic that gets the mouse button state and checks whether a button is pressed and whether the position is within the window bounds and put those somewhere else in your program.
#Do as few transformations of data as necessary Several places in your code you're converting between degrees and radians. There are several problems with doing this:
- It becomes difficult to figure out or remember which units a variable is in, particularly if it's named something generic like
angle
orrotation
. - It's inefficient in both the space the program takes up and the time it takes to execute. It might not matter here, but maybe it will later.
- It can introduce inaccuracies. Again, they may not be noticeable in this code, but they could be multiplied later.
In general, all code other than code which displays values to users is probably going to want to operate in radians. All math functions in the standard library use radians. You should probably be using radians internally and only converting to degrees for display.
Also, when you do want to convert between degrees and radians, you should write a named function to do it and call that function rather than always writing the values inline. It's easy to mistype one of the values and someone not familiar with the transformation might not immediately understand it looking at the code. If you had functions named degreesToRadians()
and radiansToDegrees()
, it becomes immediately obvious to anyone reading the code what you're doing.
#Avoid magic numbers
In moveCircle()
you have the value 400
twice in the first line. What does it represent? I'm guessing that the window or view this is in is 800x800 pixels, so (400, 400) is the center? If so, you should at least have a constant defined for that. I recommend something like:
const sf::Vector2f kWindowCenter(400, 400);
Or better yet, pass in the point you want to rotate around! What happens if you want to let the user resize the window (or view)? What if you want to rotate around a different point?