C++ pet-peeves

An interesting discussion broke out on comp.lang.c++.moderated recently. I thought I'd share the topic here and see what people thought about it. (I hope I'm not breaking forum rules by doing so.)

What are your C++ pet-peeves?

Here's what other people have listed:

Performance related pet-peeves

* people using const references to pass ints and bools

* consecutive streaming statements to the same stream, especially when they flush the stream unnecessarily:

Code:

---

cout << "a " << a << endl;
cout << "b " << b << endl;

---

* Initialized variables that are immediately overwritten:

Code:

---

bool x = false;
x = MyFuntion();

---

Unnecessary code pet-peeves

* Checking if a container is empty before iterating over it:

Code:

---

if ( !v.empty() )
{
  for ( v::iterator i ...)
    ...
  ;

}

---

* Using "v.size() == 0" or "v == std::vector<int>()" instead of simply "v.empty()"


Reliving C pet-peeves
* Always defining a structure this way:
[code]
typedef struct _StructName
{
// data members
}StructName;
[code]

* Using macro constants rather than C++ constants:

Code:

---

#define SOME_CONSTANT 5 // int const some_constant = 5;

---

* Testing bools against true/false

Code:

---

if (some_bool != false) // use: "if (some_bool)"

---

* Using C <string.h> functions instead of C++ string operations:

Code:

---

std::string s1;
std::string s2;

if(strcmp(s1.c_str(),s2.c_str())!=0)
{}

---

* Attempting to use C++ "features" when not needed just to make it look like you know C++

Code:

---

class IHaveASimpleCFunction
{
public:
  IHaveASimpleCFunction() {} // do nothing

  void operator()()
  {
    // do what needs to be done
  }

};

int main()
{
  IHaveASimpleCFunction i;
  i();
  return 0;
}

---

Management / people related pet-peeves

* People who say that "macros are evil" or that require that macros
cannot be used. Use templates where you can. But don't be afraid to
use macros if it makes sense. Andrei Alexandrescu and Matthew Wilson
both use macros in their books and libraries where it makes code
cleaner and easier to read.

* People / rules that make unnecessary requirements. For example:
code must be single entry, single exit.

* Companies that will not provide the tools to help the developers
do the best job possible. (Ex: "Memory leak detection software is too
expensive.", GCC/GDB is free, why do you need an integrated development
enviroment.)

* People who mandate rules just to save source-code space. Ex:
Function names can only be 10 characters long.

* People / companies who refuse to use source control systems.

* Developers who believe that testing departments exist to find all
the bugs and consequently do nothing to develop tests themselves.

* Companies that do not do code reviews

* Development teams where nobody has the authority to make a final
say. (This usually happens on a two-person co-development environment
where neither was designated as the lead developer).

* People / rules that require hungarian notation in C++.


Compiler-Specific pet-peeves

* "#pragma once" instead of header guards (what if you need to use a
non-MS compiler?)

* Using #include <not-a-system-header>

Quote:

---

It won't compile on many compilers.
When questioned, the usual answer is "because I think that header is important"

---

Generally frowned upon C++ practices

* "using someNamespace" in header files

* Classes that are not const-correct

* lack of const (and I mean lack of const in each and every single isntance where you're not required
not to use it).

* Unitialized variables

* throw new T();

* throw "Oops, there was an error";

* Relying on undefined behavior (or compiler-specific behavior for things defined as "undefined behavior" by the standard)


Documentation / comment pet-peeves

* cryptic comments

* incorrect / out-of-date comments

* unnecessary comments:

Code:

---

++i; // increment i

---

* Missing and/or incomplete documentation.
(It's truely a joy to add something to a program wich you basically have to reverse engineer to figure out where to fit in the new code.)

* Wrong documentation
(Even worse then missing documentation, at least with missing documentation you know you've to figure it out yourself.)

Code layout pet-peeves

This section lists pet-peeves that noone can seem to come to complete agreement upon, but that nonetheless greatly annoy people. You can never make everyone happy! =D

* for(;;) vs. while(1) vs. while(true)

* Checking pointers against NULL:

Code:

---

    if (p) {
    }

VS

    if (p != NULL) {
    }

---

* Using NULL at all:

Code:

---

    T* p = NULL;
VS
    T* p = 0;

---

* If / While code layout (where only a single statement is used):

Code:

---

    if (condition) statement;
    while (condition) statement;

VS

    if (condition)
        statement;

    while (condition)
        statement;

VS

    if (condition) {
        statement;
    }

    while (condition) {
        statement;
    }

---

* Typedef's for pointers: "typedef X* XPtr;"

Quote:

---

Actually I do this occasionally, especially when I may have to change to
a smart-pointer sometime in the future. All I need to do is change
typedef X *XPtr;
to
typedef shared_ptr<X> Xptr;

---

Quote:

---

This is useful in classes sometimes:

Code:

---

template <typename T1, typename T2, typename T3>
class X
{
public:
    typedef X<T1, T2, T3> class_type;

    void Foo(class_type* pX);
    void Foo(class_type& rX);
};

---

---

* Using parentheses after return statement: "return (5);"


Unsorted pet-peeves

* failure to take advantage of string literal concatenation:

Code:

---

cout << "ab" << "\n" <<
        "xyz";

cout << "ab\n"
        "xyz";

---

* Prepending all class names with "C". Contrary to popular belief,
"C" is not related to hungarian notation. This convention was picked
up by compiler writers before namespaces existed. Microsoft used "C".
Borland used "T". Microsoft never wanted or expected programmers to
adopt this convention. People originally just blindly picked this up
b/c Microsoft was doing it. People now blindly pick it up because
other people do it. Now that namespaces exist, new interfaces provided
by compiler vendors (including Microsoft) don't use the "C" convention
anymore. Neither should we.

* Classes which consist of entirely public static functions. Namespaces should be used instead.
NOTE: An exception would be template meta-programming, where this is often required.

* Using new / delete when its not necessary:

Code:

---

{
    T *p = new T();
    // ...
    delete p;

}

instead of just:

{
    T t;
    // ...

}

---

* the belief that dealing with exceptions means try/catch everywhere:

Code:

---

try
{
    // something that may throw
    // ...
}

catch (T t)
{
    // oops, there was an error, but I don't know what to do
    throw t;

}

---

Really nice. There are so many things. The discussion thread could go uncontrolled. Better would have been you compiled these stuff and added into the FAQ list or made an article out of it and then the moderators could hang them on top of the non-visual C++ forum as a sticky thread or something. Or may be some other way round. Thanks from my side for putting up such an exhaustive list. Its for sure going to help me.

Also, could I use my free-mail id to register to the comp.lang.c++.moderated group? How do we get connected to it. I tried once but it needed some configuration with my email account and I was not able to figure it out.

Yeah, this is a long list. I didn't think that this would fit well into the FAQ section as these are more just what get's people annoyed, not about firm rules.

I was also interested to see if there were other pet-peeves people had that didn't make the list. (And I was kind of hoping to see if any debates would open up about the code layout issues. I actually find these debates quite informative; from some I have actually learned what makes things clearer for the newbie and therefore less prone to maintenance errors.)

:D

Cheers!

To read comp.lang.c++.moderated, I just use google groups without subscribing:

http://groups-beta.google.com/group/....c++.moderated

------------------------------------------

It's funny you should post this. I have recently been seeing a lot of these as I work through the code of excellent programmers who do things a little different than me. Most of the pet-peeves I see at work are of the Reliving C variety. One that wasn't mentioned (I don't think) is declaring all the variables at the top of the function, and even worse "initializing" them on a separate line of code just below:

Code:

---

void Foo()
{
    BOOL bRet;
    int nValue;

    bRet = FALSE;
    nValue = GetCurrentValue();

    // ...
}

---

Aarggh!! :D

It takes all my will power not to go through every function and change these things, and even then sometimes I do it anyway. :p

Heh, there are some really good ones here. Nice stuff.

Quote:

---

* consecutive streaming statements to the same stream, especially when they flush the stream unnecessarily:

Code:

---

cout << "a " << a << endl;
cout << "b " << b << endl;

---

---

I am not sure I understand this one fully. Without a flush let's say, what's the problem with consecutive stream statements, as oppose to it being all on one line? There is no performance penalty (I think), and it helps readability. If you want to be picky, I guess you could do:

Code:

---

cout << "a " << a << "\n";
cout << "b " << b << endl;

---

or something, but I think endl at end is nicer. I know I broke one "rule" here, but I actually like the \n separate. Even with endl, the extra flush can't be that bad. I don't see much of a probem with this; considering usually you'd see this type of thing for debugging purpoces, and comment it out or delete it later.

Quote:

---

Companies that will not provide the tools to help the developers
do the best job possible. (Ex: "Memory leak detection software is too
expensive.", GCC/GDB is free, why do you need an integrated development
enviroment.)

---

Add to that Valgrind if you are working on a supported platform.

A few of my own:

* Doing

Code:

---

char *p = "flop";
char &c = *p;

---

Instead of

Code:

---

char* p = "flop";
char& c = *p;

---

* Using parenthesis on the same line:

Code:

---

class MyClass {
      //
};

---

Insted of:

Code:

---

class MyClass
{
      //
};

---

I can't read it.

* In general trying to fit too much code into given space, making it harder to read.

* Using TURE and FALSE, instead of true and false.

* Epending on a previously mentioned one, using anything BUT 0 (ie. 0l, 0L or NULL) for a null pointer.

You've got to add TABS vs. SPACES in code! All code should be indented with SPACES! Not everyone uses the same text editor.

;)

Viggy

Quote:

---

Originally Posted by MrViggy

You've got to add TABS vs. SPACES in code! All code should be indented with SPACES! Not everyone uses the same text editor.

;)

---

One of my biggest pet peeves is the code that uses spaces instead of tabs. On my team, we all do use the same editor! Trying to maneuver through the code is a lot faster when there are tabs than when there are spaces. :) Plus the auto-indent feature is extremely helpful, unless it is automatically indenting to a different place than the existing code uses.

I'm not disagreeing with you, though. In any environment where the code might be looked at in more than one type of editor, I agree that spaces are generally better. I just think it's funny that one of my pet peeves is nearly the opposite of yours. :D

Here's some of my peeves


Management / people related pet-peeves


*Companies that think something can be done in five minutes when in they don't understand the depth of what they asked for and it takes far longer. "What's taking you so long??!!"

*Companies that think software engineers can develop anything and do not care whether or not it's designed properly.

*Time sheets anyone?

*Software engineers with too much pride and try to cover up their mistakes. "It's not my **** code" mentality.

*Feature creep



Programming Peeves!


*Deep deep inheritance hierarchies.

*"I_can_do_it" functions a.k.a. "Fat Functions".
Functions that have way too much code in them and think they can do it all. It's also unfortunate that these functions tend to be a critical/important point in the software as well.

*Excessive coupling between software components.

*Bad cohesion

*Having to maintain and add features to a poorly written legacy system

*Excessive use of global variables

*Spaghetti code

*Poor/confusing naming conventions

To add to the company pet-peeves:

Managers who do not understand that you cannot bribe a programmer into fixing a program that is unfixable or add features to something that is unmaintainable.

For example, how many times have you heard some dolt who knows nothing about programming, but who happens to be the manager of some department say the following:

Quote:

---

For the right price, I can get this program fixed/completed/whatever by someone else

---

It's as if that impossible loop, or that incomprehensible function, or those scores of memory leaks will clean themselves up, or that impossible to implement "feature" will get done if you shove a few dollars in front of a programmer -- the more money, the more features and fixes automatically appear.

They believe that writing a program is the same as writing a long letter. I guess they believe this since programmers use a keyboard just like their secretary and clerks (mind you, not to put down secretaries and clerks).

Regards,

Paul McKenzie

Quote:

---

Doing

Code:
char *p = "flop";
char &c = *p;
Instead of

Code:
char* p = "flop";
char& c = *p;

---

or rather

Code:

---

const char * p = "flop";

---

and I like the space both sides of the operator.

Quote:

---

if (condition) statement;
while (condition) statement;

VS

if (condition)
statement;

while (condition)
statement;

VS

if (condition) {
statement;
}

while (condition) {
statement;
}

---

vs

Code:

---

if ( condition )
{
  statement();
}

while ( condition )
{
  statement();
}

---

My biggest pet peeve is one that I've seen in another team member's code:

Code:

---

unsigned i = 0;

---

This one makes my blood boil. Is it that hard to type "int"?

Quote:

---

Originally Posted by Bob Davis

My biggest pet peeve is one that I've seen in another team member's code:

Code:

---

unsigned i = 0;

---

This one makes my blood boil. Is it that hard to type "int"?

---

No but it is a pain typing unsigned, which is why I like to typedef unsigned int to uint or uint_t or uint32 if you want to specify a 32-bit unsigned int. It also looks tidier if a type has a single name.

I typedef const std::string & to str_cref as well.

Quote:

---

Originally Posted by NMTop40

No but it is a pain typing unsigned, which is why I like to typedef unsigned int to uint or uint_t or uint32 if you want to specify a 32-bit unsigned int. It also looks tidier if a type has a single name.

---

I would use uint32_t since that's defined in the C99 standard, is available on many C++ compilers already, and will most likely be adopted by the next C++ standard (it's already being included in TR1).

Quote:

---

Originally Posted by NMTop40

I typedef const std::string & to str_cref as well.

---

Now, this would annoy me if I saw this in public header files or documentation. str_cref is not a standard way of expressing "const std::string &", and consequently would send people searching for it's definition. If however, it happened in internal file, I could live with it.

When you say a "public header". if you mean the entry point to a library, I never use std::string an the interface to any library because it is non-portable. Your std::string may have a different representation to mine if we are using different versions of STL.

I have a wrapper class portable_basic_string and the typedef to portable_string with char as its type is in its own header. pstr_cref can also be put in its own header in which case you don't need to search around to see what it means.

Yep, that's what I meant. This brings up a good point though. If a library heavily uses the STL containers -- lists, vectors, etc..., not just std::string -- then making a portable public interface can be difficult. I wish there were standard portable versions of the STL containers.

Hmm, about that one

Code:

---

{
    T *p = new T();
    // ...
    delete p;

}

instead of just:

{
    T t;
    // ...

}

---

There may be a reason for preferring the 1st one in some cases. If the object is too large to be placed on the stack... What do you think?

One that recentlly caused me grief is completelly avoiding the usage of refferences. All the functions were like:

Code:

---

void Foo::func( TimeExpensiveClass obj)
{
    obj.getName();
}

vs

void Foo::func( const TimeExpensiveClass& obj)
{
    obj.getName();
}

---

I will publish my "portable_basic_string" sometime but it relies on a shared-pointer. My shared-pointer is based on boost but is slightly different. In boost's shared-pointer it calls d(p) with your deleter, whereas with mine it calls a virtual function destroy() and doesn't pass the pointer at all. The deleter is given the pointer on its construction, and deletes itself at the same time as it deletes the pointer, thus the deleter and the pointer have the same lifetime.

So my deleter for an array of characters looks like this:

Code:

---

template < typename T >
struct self_array_deleter : public deleter_base
{
private:
  const T* itsPointer;
public:
  void destroy()
  {
      delete [] itsPointer;
      delete this;
  }
  self_array_deleter( const T* pt ) : itsPointer( pt ) {}
};

---

The boost equivalent version would be:

Code:

---

template < typename T >
struct array_deleter
{
  void operator()( const T* ptr )
  {
    delete [] ptr;
    }
};

---

With boost you can use shared_array though, which should work here. It defaults to checked_array_deleter() (I think) if you don't specify otherwise.

So it's portable only if you are using the same version of boost. (But as I am not using boost in my situation everyone is forced to use the similar smart pointer I have).

(By the way, you can put down companies who don't use boost as a pet-peeve too. I generally rewrite the bits I want on similar lines to how boost have implemented them...).

As a junior developer this thread is great!

Thanks everyone.

polus quietly creeps out of the thread to change his ways - stop doing the if(TRUE == bResult) .. and int nVar = 0; nVar = GetVar().... :D

Quote:

---

Originally Posted by polus

As a junior developer this thread is great!

Thanks everyone.

polus quietly creeps out of the thread to change his ways - stop doing the if(TRUE == bResult) .. and int nVar = 0; nVar = GetVar().... :D

---

actually I wouldn't stop from using int nVar = 0; regardless what comes after it. It creates and maintains the good habbit of initializing all the variables as soon as they are defined. Perhaps, later on you will remove the nVar = GetVar(); but you will forget to add the initialization on the line above.

Quote:

---

Originally Posted by jlou

Most of the pet-peeves I see at work are of the Reliving C variety. One that wasn't mentioned (I don't think) is declaring all the variables at the top of the function, and even worse "initializing" them on a separate line of code just below:

Code:

---

void Foo()
{
    BOOL bRet;
    int nValue;

    bRet = FALSE;
    nValue = GetCurrentValue();

    // ...
}

---

Aarggh!! :D

It takes all my will power not to go through every function and change these things, and even then sometimes I do it anyway. :p

---

Yes, I'm with you on this. In general I find the combination of C style and C++ style in the same program very confusing and difficult to read/maintain.

Quote:

---

Originally Posted by KevinHall

Yeah, this is a long list. I didn't think that this would fit well into the FAQ section as these are more just what get's people annoyed, not about firm rules.

I was also interested to see if there were other pet-peeves people had that didn't make the list. (And I was kind of hoping to see if any debates would open up about the code layout issues. I actually find these debates quite informative; from some I have actually learned what makes things clearer for the newbie and therefore less prone to maintenance errors.)

:D

Cheers!

---

Quite a lot of stuff here I agree with (in the whole thread) but, as usual, not everything!

Also, some things are unavoidably subjective. The problem is in separating out the subjective from the objective. You get the same thing with coding standards. I find that, while I have preferences, there are some practices which I don’t like but which I’m prepared to tolerate and others that I won’t tolerate.

How about the general pet-peeves of:

- Picking up code with no documentation and no specs. Look at the code to see what it does.
- One big directory full of every header file.
which often leads to
- A lot of libraries (DLLs or shared-objects or even static-libs/archives) and you have to link with all of them just to make sure.

- If it ain't broke, don't fix it. i.e. if the program doesn't actually crash it works.
- We can't budget for you to spend 30 minutes modifying this code. Spend the time on codeguru instead.

Reading through this I see you guys are worried about the portability when using STL containers. Why is this? Are the interfaces they use not specified by the standard? Dont they all support the same set interfaces? Surely how they do their thing is irrelevant as long as the functions/algorithms do what the standard specifies. I thought as long as you used the standard functions of these containers and you made no assumptions about how the STL class in questions inner workings work then your code was portable. Am I wrong then?

The source code is portable. However, the data-layout of the STL containers may not be portable. This is a concern if you distribute a library that uses STL containers as part of the interface.

For example, if I write a library using MSVC 8.0 that exposes a function "void Foo(const std::string& str);", but someone uses Digital Mars's compiler to interface with it, then the user's application will pass a reference to Digital Mar's implementation of std::string rather than the expected MSVC 8.0 std::string. Any differences of internal layout will lead to undefined behavior.

Anyway, I hope this makes sense.

- Kevin Hall

Quote:

---

Originally Posted by KevinHall

For example, if I write a library using MSVC 8.0 that exposes a function "void Foo(const std::string& str);", but someone uses Digital Mars's compiler to interface with it, then the user's application will pass a reference to Digital Mar's implementation of std::string rather than the expected MSVC 8.0 std::string. Any differences of internal layout will lead to undefined behavior.

---

This is not just the case for STL. You could say the same thing about the FILE structure from stdio.h.

Regards,

Paul McKenzie

Very true.

Quote:

---

Originally Posted by panayotisk

Hmm, about that one

Code:

---

{
    T *p = new T();
    // ...
    delete p;

}

instead of just:

{
    T t;
    // ...

}

---

There may be a reason for preferring the 1st one in some cases. If the object is too large to be placed on the stack... What do you think?

---

If the object is too large to be placed on the stack then it should use new/new[] internally to allocate the large array/whatever makes the object so large (just like std::vector - I have yet to come across a scenario where you need to create a vector using new). It is not reasonable to rely on the programmer to use new when creating the object because it might not fit on the stack.

Quote:

---

The source code is portable. However, the data-layout of the STL containers may not be portable. This is a concern if you distribute a library that uses STL containers as part of the interface.

For example, if I write a library using MSVC 8.0 that exposes a function "void Foo(const std::string& str);", but someone uses Digital Mars's compiler to interface with it, then the user's application will pass a reference to Digital Mar's implementation of std::string rather than the expected MSVC 8.0 std::string. Any differences of internal layout will lead to undefined behavior.

---

Yeah guess your right. Never really thought about it that way as every library I have used has come with full source which you build yourself. I did say if you made no assumptions about the containers inner workings but if the library is already compiled then those assumptions have already been made.

My company's policy is to hide the internals (we're quite protective about the internal engineering IP) and consequently have to think about these things. We've been bitten most often by structure alignment issues because most of our API is C. But if we go to C++, the STL containers will be another thing we'll have to think of.

Favorite pet peeve? Most programmers are lousy at what they do, even after many years of experience. Fix that problem and the rest go away (as if you ever could).