Style to program - Could this be good coding style?

HenriK (136)
Hi, this is my second topic here and here I´d like to discuss about C++ coding style. Below I have two files, header and source file I have created some time ago, containing one class. This is supposed to be part of the bigger project I´m working on, called GOOS (Game Object Oriented Script).

Header file(.hpp):
------------------------
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
 
#ifndef __GOOS_ENTITY_ACCI_POWER_HPP__
#define __GOOS_ENTITY_ACCI_POWER_HPP__
#include "GOOS_Definitions.hpp"
typedef class GOOS_Entity_Acci_Power{
		bool bPowerAmountDoesExtend, bPowerAmountLimited;
		ushort usPowerAmountExtensionMax, usPowerAmountExtensionNone, usPowerNum, usPowerUnit;
		// Private power actions
		inline GOOS_Command f_Check(bool); // false: power, true: power extension
		inline GOOS_Command f_CheckChangePower(ushort,auto short);
		inline GOOS_Command f_CheckChangePowerExtension(ushort,auto short);
	public:
		ushort usPowerAmount, usPowerAmountMax, usPowerhAmountExtension;
		// Power actions
		GOOS_Command f_ToggleLimitation();
		// Operator overloading
		void operator=(ushort); // power number assignment
		void operator=(GOOS_Entity_Acci_Power);
		void operator++(GOOS_Postfix); // power increasement
		void operator--(GOOS_Postfix); // power decreasement
		void operator+=(ushort); // power increasement
		void operator-=(ushort); // power decreasement
		void operator>=(ushort); // power extension increasement
		void operator<=(ushort); // power extension decreasement
		void operator~(); // delete/normalize changes made to power extension
		// Constructors & destructors
		explicit GOOS_Entity_Acci_Power();
		explicit GOOS_Entity_Acci_Power(ushort);
		~GOOS_Entity_Acci_Power();
}power;
#endif


Source file(.cpp):
-----------------------
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
 
#include "GOOS_Entity_Acci_Power.hpp"
// ---------------------
// Private power actions
// ---------------------
inline GOOS_Command GOOS_Entity_Acci_Power::f_Check(bool extension){
    // Check power value
	if(!extension){
        if(usPowerAmount > usPowerAmountMax) usPowerAmount = usPowerAmountMax;
        if(usPowerAmount <= 0) usPowerAmount = 0;
    }else{ // Check power extension value
        if(usPowerhAmountExtension > usPowerAmountExtensionMax) usPowerhAmountExtension = usPowerAmountExtensionMax;
        if(usPowerhAmountExtension <= 0) usPowerhAmountExtension = 0;
    }
}
inline GOOS_Command GOOS_Entity_Acci_Power::f_CheckChangePower(ushort oprt, auto short amount){
	if(bPowerAmountLimited && (usPowerAmount > 0 && usPowerAmount < usPowerAmountMax)){
		switch(oprt){
			case GOOS_OPERATOR_MINUS2X: --usPowerAmount;break;
			case GOOS_OPERATOR_MULTIPLYEQUAL: usPowerAmount -= amount;break;
			case GOOS_OPERATOR_PLUS2X: ++usPowerAmount;break;
			case GOOS_OPERATOR_PLUSEQUAL: usPowerAmount += amount;break;
			default:break;
		}this->f_Check(false);
	}
}
inline GOOS_Command GOOS_Entity_Acci_Power::f_CheckChangePowerExtension(ushort oprt, auto short amount){
	if(bPowerAmountDoesExtend && bPowerAmountLimited){
		if(usPowerhAmountExtension > 0 && usPowerhAmountExtension < usPowerAmountExtensionMax){
			switch(oprt){
				case GOOS_OPERATOR_GREATERTHANEQUAL: usPowerAmountMax += amount;break;
				case GOOS_OPERATOR_SMALLERTHANEQUAL: usPowerAmountMax -= amount;break;
				default:break;
			}this->f_Check(true);
		}
	}
}

// -------------
// Power actions
// -------------
GOOS_Command GOOS_Entity_Acci_Power::f_ToggleLimitation(){bPowerAmountLimited ? bPowerAmountLimited = false : bPowerAmountLimited = true;}

// --------------------
// Operator overloading
// --------------------
void GOOS_Entity_Acci_Power::operator=(ushort power_num){if(power_num >= 1) usPowerNum = power_num;}
void GOOS_Entity_Acci_Power::operator=(GOOS_Entity_Acci_Power another_power){usPowerAmount = another_power.usPowerAmount;}
void GOOS_Entity_Acci_Power::operator++(GOOS_Postfix){this->f_CheckChangePower(GOOS_OPERATOR_PLUS2X,0);}
void GOOS_Entity_Acci_Power::operator--(GOOS_Postfix){this->f_CheckChangePower(GOOS_OPERATOR_MINUS2X,0);}
void GOOS_Entity_Acci_Power::operator+=(ushort amount){this->f_CheckChangePower(GOOS_OPERATOR_PLUSEQUAL,amount);}
void GOOS_Entity_Acci_Power::operator-=(ushort amount){this->f_CheckChangePower(GOOS_OPERATOR_MINUSEQUAL,amount);}
void GOOS_Entity_Acci_Power::operator>=(ushort amount){this->f_CheckChangePowerExtension(GOOS_OPERATOR_GREATERTHANEQUAL,amount);}
void GOOS_Entity_Acci_Power::operator<=(ushort amount){this->f_CheckChangePowerExtension(GOOS_OPERATOR_SMALLERTHANEQUAL,amount);}
void GOOS_Entity_Acci_Power::operator~(){
    usPowerAmountMax = usPowerAmountExtensionNone;
    if(usPowerAmount > usPowerAmountExtensionNone) usPowerAmount = usPowerAmountMax;
}

// --------------------------
// Constructors & destructors
// --------------------------
GOOS_Entity_Acci_Power::GOOS_Entity_Acci_Power(): bPowerAmountDoesExtend(bPowerAmountLimited=true), usPowerAmount(usPowerAmountMax=usPowerAmountExtensionNone=500){}
GOOS_Entity_Acci_Power::GOOS_Entity_Acci_Power(ushort power_amount): bPowerAmountDoesExtend(bPowerAmountLimited=true), usPowerAmount(usPowerAmountMax=usPowerAmountExtensionNone=power_amount){
    power_amount = usPowerAmount = usPowerAmountMax;
    if(power_amount <= 0) bPowerAmountLimited = false;
}
GOOS_Entity_Acci_Power::~GOOS_Entity_Acci_Power(){}


So, the ultimate question is if this is good coding style or not.
firedraco (6247)
It looks fine, but the only things I'd change are the header guards so they don't start with _ and removing the useless Hungarian notation.

Probably doesn't matter, but IIRC names starting with an _ are reserved by the standard.
bartoli (271)
When i have composed names like your GOOS_Entity_Acci_Power, i tend to split that into multiple levels of namespaces, like
1
2
3
4
5
6
7
8
9
 
namespace GOOS{
 namespace Entity{
  namespace Acci{
    class Power{
     ...
   };
  };
 };
};

For symbols names inside the namespaces, you can then refer to the class by typing only Power,
and GOOS::Entity::Acci::Power from outside the namespace
All the other types and constants starting by GOOS would pass in namespace GOOS and see their name shortened and their scope better defined (IMO)
Duthomhas (13282)
His name choices are consistent and well-formed. There is no need to go restructuring everything to avoid them.

@HenriK
Your operators are not quite right, particularly ones that must return a value, like the comparison operators. See the Wikipedia page for examples of what the operator should return and the const-ness of non-integer argument types:
http://en.wikipedia.org/wiki/Operators_in_c_and_c%2B%2B

Hope this helps.
HenriK (136)
@firedraco

That is good suggestion, perhaps I´d better remove extra '_' -characters in front of the header definition thing. I´ll keep that in mind, but Hungarian notation I´d like to leave. That helps me to figure out the type of my variable more easily.

@bartoli

I´ve heard that many programmers use namespaces like you tend to do. However, when my project various different classes, one header file would be pretty big compared to source files including declarations of the classes and that alone may make code shorter but bit more difficult to maintain. Not sure about this, though, perhaps handy, nifty linking might do the trick anyway.

@Duoas

Thanks for the praises. Have to check your link, it may be helpful.

Thanks to all about your feedback. My problem with programming is that I´m not sure whether I´m doing everything right. My coding style may be lacking or my programs may work illogically, but that´s what happens, when you haven´t even turned 17 and haven´t got any advanced teaching (at school we have only talked about variables, operators, functions and arrays and that´s about it).

But Duoas, why these comparison operators have to return a value? All I want to do is change values of the class itself and results work fine even without the return.
Last edited on
Duthomhas (13282)
Wait.... what?

If I am comparing two integers, say -7 and 12, I want to know whether the comparison is true or false.

  -7 < 12   --> true
  -7 > 12   --> false

The "<" and ">" operators are functions that take two integers and return a boolean value.

How does it make sense for me to say 9 <= 4; and discover that the 9 is somehow changed?

If you overload the comparison operators, you should not change the way they work. Hence, I can write:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
 
struct point
  {
  int x, y;
  point( int x = 0, int y = 0 ): x( x ), y( y ) { }
  };

bool operator == ( const point& lhs, const point& rhs )
  {
  return (lhs.x == rhs.x) && (lhs.y == rhs.y);
  }

bool operator != ( const point& lhs, const point& rhs )
  {
  return !( lhs == rhs );
  }
1
2
3
4
5
 
point a( -7, 12 );
point b(  7, 12 );

if (a == b) cout << "equal (oh no!)\n";
else        cout << "not equal (good job!)\n";

The operators tell me something about the relative qualities of my two points, but it did not modify them. In fact, it didn't do anything at all except tell me something useful in the if statement, so that I can choose what to do next.

Hope this helps.
HenriK (136)
Perhaps I didn´t explain correctly what I ment. OK, these two comparision operator overloadings have actually totally different functionality than we normally have in C++ codes. They are used as increasement and decreasement of my power, and that´s about it. However, if they would work as they usually would, then I understand (at least for the most part) that it makes a perfect sense.

Conclusion:
Comparision operations accept two values to check and then return boolean value, whether the statement is true or false.

Please correct if I´ve understood things wrong. And thanks a lot!
LB (13399)
You can overload operators to behave however you want, just know that it may be confusing to do so for others reading your code. Also, you can overload pre-increment/decrement to be different from post-increment/decrement. ;)
Duthomhas (13282)
Just because C and C++ allow you to do a thing does not mean that you should.
OK, these two comparision operator overloadings have actually totally different functionality than we normally have in C++ codes.
And that there is the problem with your code. You are doing something the Wrong Way. If you want to do something different than compare two things, then why are you using the "compare things" form?

Make your code clear, not obtuse. You have already got functions to do what you want, anyway, and that are much more clear to understand. Why not just make them public? Or if you must hide the details with your magic constant, use an inline alias:

1
2
3
4
5
 
class GOOS_Entity_Acci_Power {
		...
	public:
		void PowerUp(ushort amount) {f_CheckChangePowerExtension(GOOS_OPERATOR_GREATERTHANEQUAL,amount);}
};

Using this code is so much easier to read and understand, and costs nothing.

While I'm at it, why are you using typedef like that? (Don't.)
HenriK (136)
Thanks L B and Duoas for your comments.

Typedef is used, because I wanted to have one function taking care of basic operations. I´ll try to consider your suggestion, Duoas or perhaps I´ll change <= and/or >= to << and/or >> -operators.
Last edited on
Duthomhas (13282)
Don't take this the wrong way, but you aren't listening.

It is bad programming style to abuse operators. Don't do it.

If you want to do Quux, don't call it Flobbitz, call it "Quux". Likewise, if you are doing Quux, don't call it "<=" or "<<" or whatever.

What does the typedef have to do with the number of functions you are using?
ne555 (10692)
1
2
3
4
5
 
class GOOS_Entity_Acci_Power{
		// Private power actions
		inline GOOS_Command f_Check(bool); // false: power, true: power extension
		inline GOOS_Command f_CheckChangePower(ushort,auto short);
		inline GOOS_Command f_CheckChangePowerExtension(ushort,auto short);
Where are those methods defined? I though that inline function must be in header files Edit: sorry, they are not public.

1
2
 
void operator++(GOOS_Postfix);
void operator--(GOOS_Postfix);
Those are the postfix increment/decrement. Their definition must be operator++(int); in order to difference them from the prefix, they don't receive a parameter. (it's just a syntactic rule)

Curious about what do you want to do with that typedef
Last edited on
Disch (13742)
+1 @ Duaos

Operator overloading abuse is a terrible idea. Don't do it.

Just use a normal function.
HenriK (136)
OK, so no operator overloading for power extension. Could this be good idea instead?

1
2
3
4
5
6
7
8
9
10
11
12
 
template<class T> void HGDK::Features::Possibilities::Health::Extend(bool power_decreasement, bool object, T amount){
	if(!state_no_this && extendable_this){
		if(object){
			if(power_decreasement) amount_extension -= amount.amount_extension;
			else amount_extension += amount.amount_extension;
		}
		else{
			if(power_decreasement) amount_extension -= static_cast<pos_small>(amount);
			else amount_extension += static_cast<pos_small>(amount);
		}
	}this->CheckThis(true);
}


Note that my GOOS is turned into HGDK and that it includes more namespaces inside of it as well, but that´s about it.
Last edited on
ne555 (10692)
Is the template necessary?
HenriK (136)
Yes, because I want this function to handle both objects and different types (int, float, double etc.). Or do you happen to have better solution?

Updated:
1
2
3
4
5
6
7
 
template<class MT> void Extend(bool power_decreasement, bool is_object, MT amount){
    if(!state_no_this && extendable_this){
                            if(is_object) power_decreasement ? amount_extension -= amount.amount_extension : amount_extension += amount.amount_extension;
                            else power_decreasement ? amount_extension -= static_cast<pos_small>
(amount) : amount_extension += static_cast<pos_small>(amount);
    }this->CheckThis(true);
}
Last edited on
ne555 (10692)
Since when an int has an amount_extension field?
Topic archived. No new replies allowed.