0

Right now I am creating a vector in a header file. Mind, I'm not just declaring it.

std::vector<int> attacked_nodes = std::vector<int> ();

It works fine, but it's giving me some doubts. This is not in the context of a proper class, it's just a variable used by many functions.

I'd like to know if this is

  • correct (will it backfire?)
  • acceptable practice (it's probably not good)

I've searched around, yes.

NOTICE: this is probably a bad coding practice, I get it. What i want to know is if it actually incorrect?

I'd prefer not to use new to create a pointer, maybe I could use new and save a reference? It's still inelegant, but may work.

Thanks.

Community
  • 1
  • 1
Agostino
  • 2,723
  • 9
  • 48
  • 65
  • 1
    Completely unclear, what you're asking about actually. What are your particular doubts? The points you left are just too vague. – πάντα ῥεῖ Mar 05 '15 at 21:32
  • Defining a free variable in a header file is a terrible idea. Why do you need to do this? – Oliver Charlesworth Mar 05 '15 at 21:34
  • @πάνταῥεῖ I am initializing a vector in a header, and saving it as a reference. Is that leading to undefined behavior or is it bad coding practice? – Agostino Mar 05 '15 at 21:34
  • Your first problem is design, **it's just a variable used by many functions** is it that you are not allowed to pass parameters to functions? – Iharob Al Asimi Mar 05 '15 at 21:34
  • It's just done this way, and I need to know if there's a *really* good reason to change it... – Agostino Mar 05 '15 at 21:35
  • 1
    You're prone to violating the one-definition rule... – Kerrek SB Mar 05 '15 at 21:35
  • @DieterLücking Can I add a commit notice that it's actually an erroneous thing? – Agostino Mar 05 '15 at 21:37
  • **It's just done this way**, you have more problems to solve then, a really good reason to change it is that there must be a problem in your design that made you need such an anti-pattern. – Iharob Al Asimi Mar 05 '15 at 21:58
  • **NOTICE: this is probably a bad coding practice, I get it ...**, it's probably not invalid if you are careful not to compile the definition of the variable twice, but it is indeed incorrect. – Iharob Al Asimi Mar 05 '15 at 21:59
  • If only one file includes this header, the definition could be moved into the file including it and made `static`. That doesn't solve the general issues with objects of static storage duration or objects with file scope, but it can easily done now. – mafso Mar 05 '15 at 22:06
  • Just got 1000 views. Spare an upvote? :D – Agostino Jul 04 '18 at 13:08

2 Answers2

6

It's valid C++, but it's not good design. You're defining a global object in a header file. There's two problems with this: the "global" bit, and the "in a header file" bit.

Let's start with the header file issue. C++ has a rule called the one-definition rule, part of which requires that you don't have multiple definitions of an object across your entire program. If you include this header file in multiple implementation files (remember, it's like copying and pasting the contents of the header into your source files), you will have multiple definitions and therefore break the one definition rule.

Now the issue with global state is that it hides dependencies. You mention that you have many functions that use this object, yet it will not be clear from the call site of these functions that they use that object. Your functions might return different results even if you call them with exactly the same parameters, simply because this global state may have changed. This is poor interface design and makes functions considerably harder to test (and trust).

Joseph Mansfield
  • 108,238
  • 20
  • 242
  • 324
  • Thank you for posting an actual answer. If I'm going to change working code I need a clear, justified reason, that goes beyond the "looks bad" generic justification. +1 to you. – Agostino Mar 05 '15 at 21:47
  • It is not technically incorrect as such, but there is a danger that two different modules use the same header, either getting two copies of the vector (if it's declared static) or a conflict with declaring the same vector twice (two definitions of the same variable is not valid). – Mats Petersson Mar 05 '15 at 22:06
2

You asked:

I'd like to know if this is correct (will it backfire?)

This is not correct if you #include the .h file in more than one translation unit. Yes, it will backfire. Create a simple project with:

a.h

std::vector<int> attacked_nodes = std::vector<int> ();

a.cpp

#include "a.h"
void foo() {}

main.cpp

#include "a.h"

int main() {}

You will see a linker error about attached_nodes being defined more than once.

You also said:

This is not in the context of a proper class, it's just a variable used by many functions.

When you have global state that is accessed by many functions, it's best to provide access to the data using a functional interface.

.h file:

std::vector<int>& get_attacked_nodes();

In the implementation:

static std::vector<int> attacked_nodes = std::vector<int> ();
std::vector<int>& get_attacked_nodes()
{
   return attacked_nodes;
}
R Sahu
  • 204,454
  • 14
  • 159
  • 270