4
\$\begingroup\$

I have a function that reads numbers from a file and constructs a Vector3. Can anyone think of how I can make improvements to this since I am new to C++?

The file has a list of numbers like so:

0.200153
-6.30333
-2.02161
etc...

The function look like this:

//uses 3 vectors to store x,y,z then combines them at the end to make the full PxVec3

void ModelLoader::LoadVertex(std::string fileLocation)
{
    //clears vector so it doesnt break when loading multiple models
    vertexArray.clear();

    std::ifstream file2;
    file2.open(fileLocation);

    if (!file2.is_open())
        std::cout << "Failed to load vertex data" << std::endl;

    //individual x,y,z values for the vertex arrays
    std::vector<float> x;
    std::vector<float> y;
    std::vector<float> z;

    int counter = 0;

    //grabs each line add places them into a vector to be used later
    while (!file2.eof())
    {
        if (counter == 3)
            counter = 0;

        std::string num;
        file2 >> num;

        switch (counter)
        {
        case 0:
            x.push_back(stof(num));  //converts string to float
            break;
        case 1:
            y.push_back(stof(num));
            break;
        case 2:
            z.push_back(stof(num));
            break;
        default:
            break;
        }
        counter++;
    }
    file2.close();

    //adds x,y,z values to the vector to complete to full PxVec3
    for (int i = 0; i < x.size(); i++)
        vertexArray.push_back(PhysicsEngine::PxVec3(x[i], y[i], z[i]));
}
\$\endgroup\$
1
  • \$\begingroup\$ Is there a way to read the entire file into an array all at once and then iterate through that? \$\endgroup\$ Mar 14, 2018 at 4:53

2 Answers 2

4
\$\begingroup\$

Interface

The name LoadVertex(…) suggests that it's going to parse the string, rather than parse the file with the given filename. I suggest LoadVertexFile(…) for clarity.

The parameter should be a const std::string&, to avoid unnecessarily copying the string.

You test whether file2.is_open(), but the code proceeds anyway if it isn't open. The error message is printed to std::cout; I would recommend std::cerr, if you must print anything at all. (Throwing an exception would be better.) Furthermore, there is no way to programmatically tell whether the whole method succeeded. What if the number of entries in the file is not a multiple of 3?

Implementation

I don't see any point in building three vectors x, y, and z, then transposing the results to populate vertexArray. All you need is three temporary floats. You don't need a counter. Just read three entries at a time into those three floats.

You don't need to read a std::string and convert it using stof(). The iostream interface knows how to read floats.

You don't need to .close() the file explicitly; the ifstream's destructor takes care of that for you.

Suggested solution

bool ModelLoader::LoadVertexFile(const std::string& fileLocation)
{
    std::ifstream file(fileLocation);
    if (!file) {
        std::cerr << "Failed to load vertex data\n";
        return false;
    }

    float x, y, z;
    vertexArray.clear();
    while (file >> x >> y >> z)
    {
        vertexArray.push_back(PhysicsEngine::PxVec3(x, y, z));
    }
    return true;
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ You could use emplace_back to avoid naming it explicitly. \$\endgroup\$ Mar 14, 2018 at 8:18
4
\$\begingroup\$

There are a number of things that could be improved here.

Don't loop on eof()

It's almost always incorrect to loop on eof() while reading a file. The reason is that the eof indication is only set when an attempt is made to read something from the file when we're already at the end. In this case, it means we have already executed the line file2 >> num; and then called something like this x.push_back(stof(num));. The problem is that if we're at the end of the file and we attempt to read one more x value, we are now passing an empty string to stof. This is guaranteed to throw a std::invalid_argument exception. See this question for more details on why using eof is usually wrong.

Use better naming

It's not clear from the question whether ModelLoader is a class or a namespace. If it's a namespace, then it appears that vertexArray is a global variable, which is a problem itself. If it's a class, and vertexArray is a member variable, then I'd suggest reconsidering both names. If the vertexArray is part of the model, as it appears, then I'd suggest that the class should be Model and the member function should be load. Note that I use the common convention of having class names in CamelCase and members in lowercase. A consistently applied naming convention helps readers understand the code.

Use const where practical

The LoadVertex() function does not need to modify the passed string and doesn't need to make a copy, but here's how it's declared:

void LoadVertex(std::string fileLocation)

Instead, we can both avoid making a copy and signal that the passed string is not modified by doing this instead:

void LoadVertex(const std::string &fileLocation)

Return something useful from a function

It's a frequent newbie mistake to make all functions void. Better practice is to return something useful from the function. In this case the function includes this mysterious comment:

//clears vector so it doesnt break when loading multiple models
vertexArray.clear();

This strongly suggests to me that the vertexArray probably shouldn't be a part of the class at all. Instead, it probably makes more sense to create the vertexArray and then return it from the function as:

std::vector<PhysicsEngine::PxVec3> LoadVertex(const std::string &fileLocation);

Initialize variables on declaration

The best practice is to initialize varibles as soon as they're created. In C++, this most often means initializing them in the same line as the declaration. So instead of this:

std::ifstream file2;
file2.open(fileLocation);

write this:

std::ifstream file2{fileLocation};

Note, too, that I use the C++11 uniform initializer syntax (with the {}) to make it clear to both the compiler and the reader that this is an initialization and not a function call. See Stroustrup's description for more details on that.

Avoid adding unnecessary objects

The three vectors for the three coordinates are not really needed at all. Instead, one could simply read three floats and create the PxVec3 objects directly. Here is one way to do that:

namespace ModelLoader {
std::vector<PhysicsEngine::PxVec3> LoadVertex(const std::string &fileLocation)
{
    std::vector<PhysicsEngine::PxVec3> vertexArray;
    std::ifstream file2{fileLocation};
    float x, y, z;
    while (file2 >> x >> y >> z) {
        vertexArray.emplace_back(x, y, z);
    }
    return vertexArray;
}
}

However, I'd actually suggest the following approach instead.

Create an object reader

Instead of having an external class explicitly construct a PhysicsEngine::PxVec3 class instance, it makes more sense to me to have a custom object reader defined within that class and then call that. Here's what I mean, using a minimal PxVec3 class:

namespace PhysicsEngine {
class PxVec3 {
public:
    PxVec3(float x=0, float y=0, float z=0) :
        x{x}, y{y}, z{z}
    {}
    // a custom object reader
    friend std::istream &operator>>(std::istream &in, PxVec3 &vec) {
        return in >> vec.x >> vec.y >> vec.z;
    }
private:
    float x, y, z;
};
}

Now we can rewrite the loader slightly differently:

namespace ModelLoader {
std::vector<PhysicsEngine::PxVec3> LoadVertex(const std::string &fileLocation)
{
    std::vector<PhysicsEngine::PxVec3> vertexArray;
    std::ifstream file2{fileLocation};
    PhysicsEngine::PxVec3 point;
    while (file2 >> point) {
        vertexArray.emplace_back(point);
    }
    return vertexArray;
}
}

Note that now, if we want to use a double instead of a float within the PxVec3 class, no change is required to this code. This is the advantage of information hiding, that is, keeping class implementation details private to the class. However, there's an even simpler way to do this, which uses the standard library:

std::ifstream in{filename};
std::vector<PhysicsEngine::PxVec3> model{
        std::istream_iterator<PhysicsEngine::PxVec3>{in},
        std::istream_iterator<PhysicsEngine::PxVec3>{}
};

This declares model as a vector and then uses std::istream_iterator and the previously defined object reader to very elegantly initialize the vector.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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