4
\$\begingroup\$

I've written a program that runs through a matrix of numbers, ranging from 0 to 2, and stores the results in a 2D vector.

I'm using the values in the 2D vector as the profile for a tile map for a game engine.

I want this to be optimal, and as this is my first attempt at it, I doubt that it is but it does work as is.

Any suggestions for improvement or obvious errors I've made?

The entire project can be found here: link

Thanks in advance.

    // NOTE (Devan): Opening the file.
    std::ifstream mapFile ( fileToOpen );
    if ( !mapFile.is_open () )
    {
        printf ( "Error: Could not open file: %s\n", fileToOpen );
        return;
    }

    // NOTE (Devan): Setting the dimensions of the map. The width and height are taken from the first two entries of the text file.
    unsigned int width = 0;
    unsigned int height = 0; 
    mapFile >> width;
    mapFile >> height;

    int current = 0;
    for ( unsigned int y = 0; y < height; y++ )
    {
        // NOTE (Devan): Forming a new vector to be stored within the outer vector forming the rows of the map. 
        std::vector<int> innerVec;

        for ( unsigned int x = 0; x < width; x++ )
        {
            if ( mapFile.eof () )
            {
                printf ( "Reached end of file %s too early. Check width and height parameters.\n", fileToOpen );
                return;
            }

            // NOTE(Devan): Capturing the value stored at the current location within the text file.
            mapFile >> current;

            // NOTE (Devan): Checking if the input is within the tile type boundaries. Current Types: ( 0 : 2 )
            if( current >= 0 && current <= 2 )
            {
                innerVec.push_back ( current );
            }
            // NOTE (Devan): If the input is outside of the boundaries simply push 0 into the inner vector.
            else
            {
                innerVec.push_back (0);
            }
        }
        tileMap.push_back ( innerVec );
    }

    mapFile.close ();
\$\endgroup\$
  • \$\begingroup\$ It would be helpful to reviewers if you included a complete compilable program showing its use in context. \$\endgroup\$ – Edward Oct 23 '16 at 20:51
  • \$\begingroup\$ Thanks for the suggestion. Below is a link to my GitHub account where the entire project can be downloaded. link \$\endgroup\$ – dserres Oct 23 '16 at 21:22
2
\$\begingroup\$

Here are some things that may help you improve your code.

Provide complete code to reviewers

This is not so much a change to the code as a change in how you present it to other people. Without the full context of the code and an example of how to use it, it takes more effort for other people to understand your code. This affects not only code reviews, but also maintenance of the code in the future, by you or by others. One good way to address that is by the use of comments. Another good technique is to include test code showing how your code is intended to be used.

Use the required #includes

The code uses printf which means that it should #include <cstdio>. It was not difficult to infer, but it helps reviewers if the code is complete.

Separate data manipulation from output

Instead of printing things from within the program fragment, it would be better to use the std::exception mechanism to allow the caller to react appropriately to the problems it encounters. This makes the design cleaner.

Use an object instead

The tileMap is apparently a specialization of a std::vector<std::vector<int>>. Since the each of the rows are actually the same length (and apparently not resizeable), I'd recommend making this an actual std::vector<int> instead and writing inserter/extractor code for the class. That way, all of the special stuff (such as clamping the values to a particular range) can be implemented with the relevant data structure. As a simplified example (without error checking):

class TileMap : public std::vector<int>
{
public:
    friend std::istream& operator>>(std::istream &in, TileMap &tiles) {
        in >> tiles.width >> tiles.height;
        int count = tiles.width * tiles.height;
        tiles.reserve(count);
        int item;
        while (count && in >> item) {
            if (item < 0 || item > 2) {
                item = 0;
            }
            tiles.push_back(item);
            --count;
        }
        return in;
    }
    friend std::ostream& operator<<(std::ostream &out, const TileMap &tiles) {
        unsigned line = tiles.width;
        for (const auto &item: tiles) {
            out << item << " ";
            if (--line == 0) {
                out << '\n';
                line = tiles.width;
            }
        }
        return out;
    }
private:
    unsigned width;
    unsigned height;
};

If you want to do things with the TileMap, implement them as member functions.

\$\endgroup\$
2
\$\begingroup\$

Your code has a bug. When the very last value cannot be read, it won't detect it. Example:

2 2
1 2
3 invalid

Instead of checking for eof, check whether mapFile >> current returns true.

\$\endgroup\$
1
\$\begingroup\$

So some small remarks:

  1. You can directly check against the ifstream

    if (!mapFile)
    
  2. With c++ you should stick to std::cin and std::cout instead of scanf and printf

  3. You do not have to set default values for variables you are reading anyway

    unsigned int width;
    unsigned int height; 
    mapFile >> width;
    mapFile >> height;
    

    Also you might want to check whether the provided values are actually reasonable

    if (width < 0) { throw std::runtime_error("Invalid width provided!"); }

    if (height < 0) { throw std::runtime_error("Invalid height provided!"); }

  4. unsigned int is the same as unsigned

  5. Declare variables, where you need them. For example int current = 0; is not needed outside the loop.

  6. You already know the size of the matrix so reserve the memory to avoid reallocations

    std::vector<std::vector<int>> tileMap;
    tileMap.reserve(height);
    
    std::vector<int> innerVec;
    innerVec.reserve(width);
    
  7. If your user provides false input, you should tell him so and not silently assume some default value.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.