Sign in to follow this  
MajinMusashi

Unity C++ code correctness

Recommended Posts

MajinMusashi    182
UPDATED!!! CONTEXT: After visiting this thread in the General Programming forums (Master C++) and reading many of the threads at Guru of the Week, I've seen a light. PROBLEM: The light told me to rewrite all of my old code in a object-oriented standards-compliant fashion. Coincidently, in the same day I've read those articles, I was having some hard times figuring how to make the event handling mechanics of my OpenGL GUI to work properly (indeed, it was only bad design's fault). So, I began rewriting the most basic classes I use on all my projects ("rectangle" and "status"). Please, see the code below:
// Is it a good idea to put everything into the header (since this class is so small)?
/** @file	mi_rectangle.hpp
  * @author	Alex F. Orlando (aka Majin)
  * @date	2005/11/25 - 2005/11/27
  * @brief	Rectangle class header file.
  */

#ifndef MI_RECTANGLE_HPP
#define MI_RECTANGLE_HPP


namespace MI {


/** @class C_Rectangle
  * @brief 2D rectangle class used to delimit the coordinates of GUI elements
  *		(set their bounds), as well as to move them and check for colisions.
  *		The origin of the coordinates system is at bottom-left.	Negative values
  *		of width and height are not allowed, so some modifications may be
  *		necessary at run-time depending on the user input (for example, if the
  *		provided right coordinate is smaller than the left coordinate, these
  *		two values will be swapped.
  */
class C_Rectangle {
private:
	float left;		///< Left coordinate
	float right;	///< Right coordinate
	float bottom;	///< Bottom coordinate
	float top;		///< Top coordinate

	float width;	///< Width
	float height;	///< Height
public:
	/****************/
	/* Constructors */
	/****************/

	/** @brief Default constructor */
	C_Rectangle() : left( 0.0f ), right( 0.0f ), bottom( 0.0f ), top( 0.0f ) {}

	/** @brief Detailed constructor
	  * @param L Left coordinate
	  * @param R Right coordinate
	  * @param B Bottom coordinate
	  * @param T Top coordinate
	  */
	C_Rectangle( float L, float R, float B, float T ) {
		setCoordinates( L, R, B, T );
	}
assert
	/*******************/
	/* Acessor methods */
	/*******************/

	/** @brief Gets left coordinate
	  * @return Left coordinate
	  */
	float getLeft() const {
		return left;
	}

	/** @brief Sets left coordinate
	  * @param newLeft New left coordinate
	  */
	void setLeft( float newLeft ) {
		if ( newLeft <= right ) {
			left = newLeft;
		} else {
			left = right;
			right = newLeft;
		}
		width = right - left;
	}

	/** @brief Gets right coordinate
	  * @return Right coordinate
	  */
	float getRight() const {
		return right;
	}

	/** @brief Sets right coordinate
	  * @param newRight New right coordinate
	  */
	void setRight( float newRight ) {
		if ( newRight >= left ) {
			right = newRight;
		} else {
			right = left;
			left = newRight;
		}
		width = right - left;
	}

	/** @brief Gets bottom coordinate
	  * @return Bottom coordinate
	  */
	float getBottom() const {
		return bottom;
	}

	/** @brief Sets bottom coordinate
	  * @param newBottom New bottom coordinate
	  */
	void setBottom( float newBottom ) {
		if ( newBottom <= top ) {
			bottom = newBottom;
		} else {
			bottom = top;
			top = newBottom;
		}
		height = top - bottom;
	}

	/** @brief Gets top coordinate
	  * @return Top coordinate
	  */
	float getTop() const {
		return top;
	}
	
	/** @brief Sets top coordinate
	  * @param newTop New top coordinate
	  */
	void setTop( float newTop ) {
		if ( newTop >= bottom ) {
			top = newTop;
		} else {
			top = bottom;
			bottom = newTop;
		}
		height = top - bottom;
	}

	/** @brief Gets width
	  * @return Width
	  */
	float getWidth() const {
		return width;
	}
	
	/** @brief Sets width
	  * @param newWidth New width
	  */
	void setWidth( float newWidth ) {
		
		// Be sure that the new width is not negative
		if ( width >= 0.0f ) {
			width = newWidth;
			right = left + width;
		}
	}

	/** @brief Gets height
	  * @return Height
	  */
	float getHeight() const {
		return height;
	}
	
	/** @brief Sets height
	  * @param newHeight New height
	  */
	void setHeight( float newHeight ) {

		// Be sure that the new height is not negative	
		if ( newHeight >= 0.0f ) {
			height = newHeight;
			top = bottom + height;
		}
	}

	/*****************/
	/* Other methods */
	/*****************/
	
	/** @brief Sets left and bottom coordinates (automated right and top adjustment)
	  * @param newLeft New left coordinate
	  * @param newBottom New bottom coordinate
	  */
	void setCoordinates( float newLeft, float newBottom ) {
		left = newLeft;
		right = left + width;
		bottom = newBottom;
		top = bottom + height;
	}
	
	/** @brief Sets left, right, bottom and top coordinates (complete set)
	  * @param L Left coordinate
	  * @param R Right coordinate
	  * @param B Bottom coordinate
	  * @param T Top coordinate
	  */
	void setCoordinates( float L, float R, float B, float T ) {
		left = L;
		if ( R >= L ) {
			right = R;
		} else {
			left = R;
			right = L;
		}

		bottom = B;
		if ( T >= B ) {
			top = T;
		} else {
			bottom = T;
			top = B;
		}

		width = right - left;
		height = top - bottom;
	}
	
	/** @brief Translates the given distances
	  * @param x Horizontal distance
	  * @param y Vertical distance
	  */
	void translate( float x, float y ) {
		left += x;
		right = left + width;
		bottom += y;
		top = bottom + height;	
	}
	
	/** @brief Checks whether or not contains the point at the specified location
	  * @param x X coordinate
	  * @param y Y coordinate
	  * @return True if contains the point at the specified location
	  */
	bool containsPoint( float x, float y ) const {
		return (x > left) && (x < right) && (y > bottom) && (y < top);
	}
};


}	// End namespace


#endif


/** @file	mi_status.hpp
  * @author	Alex F. Orlando (aka Majin)
  * @date	2005/11/24 - 2005/11/27
  * @brief	Status class header file.
  */

#ifndef MI_STATUS_HPP
#define MI_STATUS_HPP

// Includes STL string header file
#include <string>


namespace MI {


/** @enum E_StatusCode
  * @brief Status code enumeration used to simplify status' access.
  */
enum E_StatusCode {
	SC_NONE,			///< No error found (success)
	SC_UNACCESSIBLE,	///< Unaccessible file
	SC_COULDNTREAD,		///< Couldn't read file
	SC_BADATTRIBUTES,	///< Bad attributes (width, height etc.)
	SC_BADPIXELDATA		///< Bad pixel data
};

/** @class C_Status
  * @brief Status class used to keep track of objects' status (in most of the
  *     cases, error codes and their descriptions). The user don't have direct
  *		access to the description variable since that is directly dependent of
  *		the current code, thus automatically defined by the class
  */
class C_Status {
	E_StatusCode code;
	std::string details;		///< Additional details
public:
	/****************/
	/* Constructors */
	/****************/

	/** @brief Default constructor */
	C_Status() {
		setCode( SC_NONE );
	}

	/** @brief Detailed constructor
	  * @param newCode New code
	  * @param newDetails New details description
	  */
	C_Status( E_StatusCode newCode, const std::string& newDetails ) {
		setCode( newCode );
		setDetails( newDetails );
	}

	/*******************/
	/* Acessor methods */
	/*******************/

	/** @brief Gets code
	  * @return Code
	  */
	E_StatusCode getCode() const {
		return code;
	}

	/** @brief Sets code
	  * @param newCode New code
	  */
	void setCode( E_StatusCode newCode ) {
		code = newCode;
	}

	/** @brief Gets description
	  * @return Description
	  */
	const std::string getDescription() const {
		std::string description;
		
		switch ( code ) {
			case SC_NONE:
				description = "No error found (success)";
				break;
			case SC_UNACCESSIBLE:
				description = "Unaccessible file";
				break;
			case SC_COULDNTREAD:
				description = "Couldn't read file";
				break;
			case SC_BADATTRIBUTES:
				description = "Bad attributes (width, height etc.)";
				break;
			case SC_BADPIXELDATA:
				description = "Bad pixel data";
				break;
			default:
				description = "Unknown error code";
		}

		return description;
	}

	/** @brief Gets details
	  * @return Details
	  */
	const std::string getDetails() const {
		return details;
	}

	/** @brief Sets details
	  * @param newDetails New details
	  */
	void setDetails( const std::string& newDetails ) {
		details = newDetails;
	}

	/*****************/
	/* Other methods */
	/*****************/

	/** @brief Gets most verbose description possible
	  * @return Most verbose description possible
	  */
	const std::string getVerbose() const {
		std::string temp = getDescription() + ": " + details;
		return temp;
	}

	/** @brief Sets most verbose description possible
	  * @param newCode New code
	  * @param newDetails New details
	  */
	void setVerbose( E_StatusCode newCode, const std::string& newDetails ) {
		setCode( newCode );
		setDetails( newDetails );
	}
};


}	// End namespace


#endif



QUESTION: Could you tell me if things are acceptable in the code above, and give me some advices on how to improve it? Everything from "temporary objects avoidance", "const-correctness", "code legibility", proper documentation" (using some of the DOxygen's acceptable comments syntax) and "object-oriented paradigm" (encapsulation etc.) will be accepted. Thanks a lot, guys! [Edited by - MajinMusashi on November 27, 2005 3:59:01 PM]

Share this post


Link to post
Share on other sites
SiCrane    11839
For the rectangle class:
* You may want to consider renaming the member variables somehow to indicate that they're member variables. A lot of people use a m_ prefix to indicate member variables, I personally use an _ suffix to indicate non-public member variables. Not a big deal for a rectangle class, but for more complex classes this kind of practice often comes in handy.
* Consider using member initialization lists instead of calling member functions to initialize the rectangle.
* Consider assert()ing on bad values instead of the current check and sets.
* Your operator==() is dangerous. Floating point comparisons with == often doesn't do what you wish. Consider comparing the absolute value of the difference of the coordinates against a tolerance value.
* You don't actually need to define the copy constructor or assignment operator; the defaults will work fine for this class.
* You don't actually need all six member variables; consider scrapping a couple.

For the status class:
* member variable names again
* You don't need to define copy constructor or assignment operator here either.
* External include guards are evil (and in this case non-portable); don't use them.
* The description member variable is pretty pointless; consider doing the description lookup when the description field is requested not during construction.
* setCode() and setDetails() seem somewhat dangerous to be part of the public interface. It would seem to make more sense for your status objects to be constructed with full knowledge of their state and then treated as value objects.
* Are two status objects really equal if their details differ?

Share this post


Link to post
Share on other sites
Conner McCloud    1135
A lot of the documentation in the rectangle example seems completely pointless. They provide no information beyond what's either already in the function name, or couldn't become part of the function name. There's no reason to define a copy constructor or an assignment operator for this class...the defaults will function as expected. The equality operator relies on floating point equality, which is less than useful.

Haven't really looked over the other example, but is there a reason the enumeration isn't a part of the class?

CM

Share this post


Link to post
Share on other sites
MajinMusashi    182
Quote:
Original post by SiCrane
For the rectangle class:
* You may want to consider renaming the member variables somehow to indicate that they're member variables. A lot of people use a m_ prefix to indicate member variables, I personally use an _ suffix to indicate non-public member variables. Not a big deal for a rectangle class, but for more complex classes this kind of practice often comes in handy.
This is a matter of choice, but I really don't like this approach (as the guy who wrote "How to Write Unmaintanable Code", hehe).
Quote:
* Consider using member initialization lists instead of calling member functions to initialize the rectangle.
I agree with this only in the default constructor (done!), because the detailed constructor's parameters must be validated by the "setCoordinates" method.
Quote:
* Consider assert()ing on bad values instead of the current check and sets.
Not agreed, since this check is useful in execution time i.e. in my OpenGL GUI when the user is resizing a button.
Quote:
* Your operator==() is dangerous. Floating point comparisons with == often doesn't do what you wish. Consider comparing the absolute value of the difference of the coordinates against a tolerance value.
Agreed (already had trouble with this)! In the past, I've used a tolerance value of 0.00001 (don't remember the source though). Do you think it's worth in the rectangle's case? Well, actually I don't :)
Quote:
* You don't actually need to define the copy constructor or assignment operator; the defaults will work fine for this class.
Agreed! Done!
Quote:
* You don't actually need all six member variables; consider scrapping a couple.
Do you mean to get rid of "width" and "height" and calculate them on demand?
Quote:
For the status class:
* member variable names again
Again, not agreed.
Quote:
* You don't need to define copy constructor or assignment operator here either.
Agreed! Done!
Quote:
* External include guards are evil (and in this case non-portable); don't use them.
Agreed (and done!) in the non-portability case (specially regarding MS VC++ and STL), but why that would be evil when using my own files?
Quote:
* The description member variable is pretty pointless; consider doing the description lookup when the description field is requested not during construction.
Agreed! Done!
Quote:
* setCode() and setDetails() seem somewhat dangerous to be part of the public interface. It would seem to make more sense for your status objects to be constructed with full knowledge of their state and then treated as value objects.
Could you exemplify?
Quote:
* Are two status objects really equal if their details differ?
Agreed! Removed this operation overloading (not that useful).

Share this post


Link to post
Share on other sites
MajinMusashi    182
Quote:
Original post by Conner McCloud
A lot of the documentation in the rectangle example seems completely pointless. They provide no information beyond what's either already in the function name, or couldn't become part of the function name.
Agreed, but what should I say about a variable named "width" that is not redundant?
Quote:
There's no reason to define a copy constructor or an assignment operator for this class...the defaults will function as expected. The equality operator relies on floating point equality, which is less than useful.
Agreed! Done!
Quote:
Haven't really looked over the other example, but is there a reason the enumeration isn't a part of the class? CM
No, there is not! Can you point me how to make it?

Share this post


Link to post
Share on other sites
Conner McCloud    1135
Quote:
Original post by MajinMusashi
Quote:
Original post by Conner McCloud
A lot of the documentation in the rectangle example seems completely pointless. They provide no information beyond what's either already in the function name, or couldn't become part of the function name.
Agreed, but what should I say about a variable named "width" that is not redundant?

I would say nothing at all. I also wouldn't comment on the getLeft, getRight, etc functions, except whatever is neccessary to ensure they get listed in the external documentation. The set function documentation would be expanded to explain what changes they make...ie, swapping left and right if necessary to ensure left <= right.
Quote:

Not agreed, since this check is useful in execution time i.e. in my OpenGL GUI when the user is resizing a button.

How important is it that left <= right, at all times? What happens if left > right?

Personally, I would define a rectangle as just two coordinates, with no spacial constraints on them at all. Then, your set functions become trivial [if not removed outright in favor of public data], with some slight extra complexity for things like drawing the rectangle. But you're in a better position than I to know how much extra complexity this would entail.
Quote:

Do you mean to get rid of "width" and "height" and calculate them on demand?

Or get rid of one of the coordinates, and calculate them using the width and height.

CM

Share this post


Link to post
Share on other sites
MajinMusashi    182
Quote:
I would say nothing at all. I also wouldn't comment on the getLeft, getRight, etc functions, except whatever is neccessary to ensure they get listed in the external documentation. The set function documentation would be expanded to explain what changes they make...ie, swapping left and right if necessary to ensure left <= right.

Well, it would be very strange if, in some parts of the DOxygen generated documentation, some methods were commented and some weren't. Agreed about the expanded explanation of the "set" methods.

Quote:
Or get rid of one of the coordinates, and calculate them using the width and height.

The coordinates are very importante to the drawing methods, so I think I'll get rid of "width" and "height".

Thanks a lot!
Still accepting sugestions :)

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this