Sign in to follow this  
TheComet

Globals

Recommended Posts

What is the best practice/what do you guys do to organize various global definitions (such as static consts or typedefs)?

 

I'm working on a small sokoban library for fun, and the "problem" is that I've placed static members and typedefs in classes where it makes the most sense, but they are also required in other classes, so you get this awkward cross-referencing. Consider the following Level class:

namespace Sokoban {

// handles everything to do with a sokoban level
class Level
{
public:

    static const std::string validTiles;
    static const std::string validUndoData;

    typedef Array2D<char> LevelArray_t;

    /* --SNIP-- */
};

// in the .cpp file:
const std::string validTiles = "#@+$*. _pPbB";
const std::string validUndoData = "udlrUDLR";

/* --SNIP-- */

} // namespace Sokoban

This is all fine and good, but it turns out that Level is not the only class requiring access to validTiles, validUndoData and LevelArray_t. These are also required by the various level file parser classes and Collection class, so you get things like the following:

namespace Sokoban {

class CollectionParserSOK
{
public:

    // parses a file and returns a collection containing all of the levels
    Collection parse( const std::string& fileName ){
        // cutting code here to get to the point, inBuf stores the current line read from the file
        for( std::size_t i = 0; i != inBuf.size(); ++i )
        {
            if( Level::validTiles.find_first_of( inBuf[i] ) != std::string::npos ) // <------ Is "Level::validTiles" clean?
            {
                // do stuff
            }
    }
};

} // namespace Sokoban

 

Or this:

namespace Sokoban {

class Collection
{
public:

    typedef Level::LevelArray_t::iterator LevelIterator_t;

};
} // namespace Sokoban

My gut is telling me that this could be done better, such as making a separate "Globals.hpp" header file and dumping definitions required in multiple classes (such as the above) in there.

 

Is that cleaner? Or am I making a big fuss over nothing?

Share this post


Link to post
Share on other sites

I'd make those private, and provide methods for them:

class Level
{
public:

    static bool IsValid(char tile) const;
private:

    static const std::string validTiles;
    static const std::string validUndoData;
};

// in cpp:
bool IsValid(char tile) const
{
     return validTiles.find_first_of(tile) != std::string::npos);
}

That way, you don't pollute your code with global state, and the implementation of the valid tiles is encapsulated properly. For example, I'm not entirely sure why you've choosen a string here, strings can be rather slow, I'd recommand changing to a std::vector<char>, std::set<char> (this is especially fast if you only need to know if a certain element exists, like here), or std::map<>. In your code, you potentially need to change every bit of code where you're using the array - using my method, you don't need to change anything. Rule of thumb, NEVER expose state, if possible.

 

Also, I'd consider if this really has to be static, therefor global - it still creates some hidden denpendencies. It might seem like a pain, but unless you really need potential access EVERYWHERE, its often better to create a concrete object, and pass it down to where its needed. This will help you enfore proper encapsulation also, since you now aren't tempted that much to put too much information into the global level class, and don't use it all over the place. But I quess it would be just as fine if you simply encapsulated the functionality instead of exposing the containers directly like I mentioned before, for starters.

Edited by Juliean

Share this post


Link to post
Share on other sites
I'd just make them global const char [] in namespace Sokoban. Give them a good name, and you're set. If that leads to collisions, put them in a nested namespace of it's own.

Keeping it as std::string is fine too if you're concerned about errors from doing things like adding these variables to string literals. Don't think too hard about this, it doesn't really matter. Edited by King Mir

Share this post


Link to post
Share on other sites
Don't think too hard about this, it doesn't really matter.
Edited by King Mir, Today, 05:49 PM.

Untortunately, i have to disagree as strongly as I can, well let me
just ask that question: when does it begin to matter? Especially with global state, spending three thoughs is way better than spending none, IMHO.

Share this post


Link to post
Share on other sites

Don't think too hard about this, it doesn't really matter.
Edited by King Mir, Today, 05:49 PM.

Untortunately, i have to disagree as strongly as I can, well let me
just ask that question: when does it begin to matter? Especially with global state, spending three thoughs is way better than spending none, IMHO.

I was specifically referring to whether the type should be string or char array. Char array is faster, but worrying a lot about performance shouldn't be done without profiling. std::string is safer, but in a small way that may not even come up. Edited by King Mir

Share this post


Link to post
Share on other sites

I was specifically referring to whether the type should be string or char array. Char array is faster, but worrying a lot about performance shouldn't be done without profiling. std::string is safer, but in a small way that may not even come up.

 

I do agree on that this wouldn't make much of a difference, however I stay by my opinion that he'd probably be better suted with a std::set<char> anyway, since what he wants is to see if an element exists, and a std::string isn't the predestinated data type for this. His code using find_first_of != npos does appear to be a tad bit hacky, IMHO. That is if good coding practice is the concern, again I agree that for performance only its not that important.

Edited by Juliean

Share this post


Link to post
Share on other sites
I don't think using a set would add clarity and needless to say for performance it's horrible. It also makes the global non const which adds more problems. Personally, I'd use a char array.

Share this post


Link to post
Share on other sites
You can have a const std::set as a global. Ex:
const char data[] = "abcd";
const std::set<char> set(data, data + 4);
I wouldn't do it myself, but there's nothing about a set that precludes being const.

Share this post


Link to post
Share on other sites
Like Juliean I'd probably hide the implementation as much as possible, but I'd probably move these guys to free functions in a namespace, with the const data in the source file.
// sokoban.hpp

namespace Sokoban {
    bool isValidTile(char c);
}

// sokoban.cpp

namespace {
    std::string valid = "#@+$*. _pPbB";
}

namespace Sokoban {
    bool isValidTile(char c) {
        return valid.find(c)!= std::string::npos;
    }
}
If there was an "obvious" class to associate this code with, I might include it in the class. For example, it may be worth having a minimal "Tile" class, wrapping a character, to add clarity to the code.

I wouldn't worry excessively about performance unless this code ends up being a bottleneck. In the original example we're loading data from a file, the I/O time will likely dominate the performance.

Share this post


Link to post
Share on other sites

You can have a const std::set as a global. Ex:

const char data[] = "abcd";
const std::set<char> set(data, data + 4);
I wouldn't do it myself, but there's nothing about a set that precludes being const.

Yeah, but this way you have a char array anyway.

I suppose I should have explicitly mentioned this possibility, however.

Share this post


Link to post
Share on other sites

Not that i matters much in that case, but the set finds something in O(log n) and the char array O(n), but the tree inside may make it slower.

I would just sort the char array and use binary_search, thats about the same programming work as using find for better complexity as result.

Maybe you could also think about structuring the code in a way to allow loading the valid chars from a (level-)file later.

Edited by wintertime

Share this post


Link to post
Share on other sites

Not that i matters much in that case, but the set finds something in O(log n) and the char array O(n), but the tree inside may make it slower.
I would just sort the char array and use binary_search, thats about the same programming work as using find for better complexity as result.
Maybe you could also think about structuring the code in a way to allow loading the valid chars from a (level-)file later.

You're talking about a 11 element set here. Big O analysis isn't the way to go, and linked data structures like tree sets have horrible cache performance.

A binary search would probably be slower than a linear search here too because of all the branching.

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