PPM File Modifier

Question

I've been working in C++ for the better part of this year and am looking to improve my style and efficiency as much as possible. One thing I've been told is to work on commenting code to make it clearer, so if you have any suggestions about where I could use a comment to clarify what is going on I would be very glad to hear them.

A guide to how PPM is structured can be found here for those unfamiliar with the file type.

Pixel.h

#ifndef __PIXEL_H_INCLUDED__
#define __PIXEL_H_INCLUDED__

#include <fstream>

struct Pixel {                                                                  
    public:                                                                       
        int red;                                                                    
        int green;                                                                  
        int blue;                                                                   
        Pixel(): Pixel(0, 0, 0) {};                                                 
        Pixel(int _r, int _g, int _b): red(_r), green(_g), blue(_b) {};
};

std::ifstream& operator >> (std::ifstream& fin, Pixel& temp) {
    fin >> temp.red;
    fin >> temp.green;
    fin >> temp.blue;
    return fin;
}

std::ofstream& operator << (std::ofstream& fout, Pixel& temp) {                            
    fout << temp.red << " ";                                                      
    fout << temp.green << " ";                                                    
    fout << temp.blue;                                                            
    return fout;                                                                  
}

#endif

PortablePixelMap.h

#ifndef __PICTURE_H_INCLUDED__
#define __PICTURE_H_INCLUDED__

#include <vector>
#include <fstream>
#include <string>

class PortablePixelMap {                                                                 
    private:                                                                      
        std::vector<std::vector<Pixel> > picture_data;                                         
        std::string magicNumber;                                                         
        int width;                                                                  
        int height;                                                                 
        int intensity;                                                              
    public:                                                                       
        bool ReadPPMFile(std::ifstream& fin);                                            
        bool WritePPMFile(std::ofstream& fout);                                          
        void FlipY();                                                               
        void FlipX();                                                               
        void InvertPixelIntensity();                                                
};

#endif

main.cpp

#include <iostream>
#include <cmath>
#include "Pixel.h"
#include "PortablePixelMap.h"

int main(int argc, char** argv) {
    if (argc < 3) {
        std::cout << "Usage: inputfile outputfile [XYI]\n";
        return -1;
    }


    std::ifstream fin;
    fin.open(argv[1]);

    PortablePixelMap picture;

    bool able_to_read_ppm_file = picture.ReadPPMFile(fin);
    if(!able_to_read_ppm_file) {
        std::cout << "Error: unable to read PPM file " << argv[1] << "\n";
        return -1;
    }

    if (argc == 4) {
        if (*argv[3] == 'X') {
            picture.FlipX();
        } else if (*argv[3] == 'Y') {
            picture.FlipY();
        } else if (*argv[3] == 'I') {
            picture.InvertPixelIntensity();
        } else {
            std::cout << "Error: " << argv[3] << " is an invalid command. Use either X, Y, or I.\n";
            return -1;
        }
    }

    std::ofstream fout;
    fout.open(argv[2]);
    bool able_to_write_ppm_file = picture.WritePPMFile(fout);
    if (!able_to_write_ppm_file) {
        std::cout << "Error: unable to write uninitialized PPM file " << argv[2] << "\n";
        return -1;
    }
    fout.close();
}

bool PortablePixelMap::ReadPPMFile(std::ifstream& fin) {
    if (fin.fail()) {
        return false;
    };
    fin >> magicNumber;
    if (magicNumber != "P3") {
        return false;
    }
    fin >> width >> height;
    fin >> intensity;
    for (int i = 0; i < height; i++) {
        std::vector<Pixel> pixel_vector;
        for (int j = 0; j < width; j++) {
            Pixel temp;
            fin >> temp;
            pixel_vector.push_back(temp);
        }
        picture_data.push_back(pixel_vector);
    }
    return true;
}

bool PortablePixelMap::WritePPMFile(std::ofstream& fout) {
    if (fout.fail()) {
        return false;
    }
    fout << magicNumber << "\n";
    fout << width << " " << height << "\n";
    if (width == 0 || height == 0) {
        return false;
    }
    fout << intensity << "\n";
    for (int i = 0; i < height; i++) {
        for (int j = 0; j < width; j++) {
            fout << picture_data[i][j]  << "\n";
        }
    }
    return true;
}

void PortablePixelMap::FlipX() {
    for (int i = 0; i < height/2; i++) {
        std::vector<Pixel> temp = picture_data[i];
        picture_data[i] = picture_data[picture_data.size()-i-1];
        picture_data[picture_data.size()-i-1] = temp;
    }
}

void PortablePixelMap::FlipY() {
    for (unsigned int i = 0; i < picture_data.size(); i++) {
        std::vector<Pixel> temp = picture_data[i];
        for (unsigned int j = 0; j < temp.size(); j++) {
            temp[j] = picture_data[i][width-j-1];
        }
        picture_data[i] = temp;
    }
}

void PortablePixelMap::InvertPixelIntensity() {
    for (unsigned int i = 0; i < picture_data.size(); i++) {
        std::vector<Pixel> temp = picture_data[i];
        for (unsigned int j = 0; j < temp.size(); j++) {
            temp[j].red = abs(temp[j].red - intensity);
            temp[j].green = abs(temp[j].green - intensity);
            temp[j].blue = abs(temp[j].blue - intensity);
        }
        picture_data[i] = temp;
    }
}

Answer 1

Include guards

• The include guards must not start with an underscore.
• Their names should be prefixed with your library name, e.g. PPM_PIXEL_H.
• Their names must correspond to the name of the file that defines them.

Efficiency

• Use std::swap in the FlipX method.

Names

• Consider renaming the FlipX and FlipY methods. I would have named them the exact opposite, hence you should look at other libraries or image editing programs how they name these operations.
• Don't name function parameters temp.

Comments

• The code is so clearly structured that it doesn't need any comments.

Code organization

• The operator<< functions don't belong in the header, since they are only used by functions in main.cpp.
• The I/O functions from main.cpp should be extracted to ppm_io.cpp, since they are declared in the header file.

Answer 2

Reserved Names

Never start any identifers with _ and dont create identifiers with double underscore __. Both are reserved for system use. See What are the rules about using an underscore in a C++ identifier?

Thus these are illegal.

#ifndef __PIXEL_H_INCLUDED__
#define __PIXEL_H_INCLUDED__

Note: When defining include guards makeing them unique is hard. So I use both the namespace and file name to make sure they are unique. Especially since I use a namespace based on a domain I own.

#ifndef YOUR_NAMESPACE_PIXEL_H
#define YOUR_NAMESPACE_PIXEL_H

Namespace

You put all your code into the global namespace. This is not a good idea especially with such common names as Pixel. Create your own namespace and put your code into this.

Parameters on constructors.

Watch those leading underscores.

        Pixel(int _r, int _g, int _b): red(_r), green(_g), blue(_b) {};

Though this time you are fine. Do you know the exact rules about underscores in identifiers. No? The problem is that very few people do. So try and make it easy for them by not using a leading underscore.

Also I like giving the same name to the parameter as the member they are going to initialize.

    // The following is perfectly valid.
    Pixel(int red, int green, int blue)
        : red(red)                      // when used in the initializer
        , green(green)                  // list each name can only identify
        , blue(blue)                    // the expected object.
    {}                  // Note no ';'

The rule about only initializing one variable per line still applies even if this is done in the constructor initializer list. You are trying to make the code easy to read (for the person coming behind you to maintain the code).

Input/Output operators.

Personally I declare these as friends of the class (and place them in the class). This shows the functions are tightly bound to the class and part of the user interface of the class.

class Pixel
{
   ... STUFF

    friend std::ifstream& operator >> (std::ifstream& fin,  Pixel& temp);
    friend std::ofstream& operator << (std::ofstream& fout, Pixel const& temp);
         // Note output operator parameter is const               ^^^^^


};

SImplify the operators.

    fin >> temp.red;
    fin >> temp.green;
    fin >> temp.blue;
    return fin;

    // This can be written as:

    return fin >> temp.red >> temp.green >> temp.blue;

And

    fout << temp.red << " ";                                                      
    fout << temp.green << " ";                                                    
    fout << temp.blue;                                                            
    return fout;                                                                  

    // Can be written as:
    return fout << temp.red << " " temp.green << " " temp.blue; 

Source/Header filer

For every header file X.h there is usually a source file X.cpp that contains the definitions that are not in the header file (Note: if all the definitions are in the header file you can eliminate X.cpp).

I don't like that you have put definitions from PortablePixelMap.h in the fie main.cpp. In a larger project that would make them harder to find.

Declare and open in one line:

    std::ifstream fin;
    fin.open(argv[1]);

    // Why not use the constructor?
    std::ifstream fin(argv[1]);

Read Vs Input operator.

I don't mind Read functions. But I definitely prefer input operator >>. You wrote an input operator for Pixel why not for PortablePixelMap?

    bool able_to_read_ppm_file = picture.ReadPPMFile(fin);
    if(!able_to_read_ppm_file) {

Assuming you had. The same functionality is available here:

    if (picture.ReadPPMFile(fin)) {

    // or

    if (fin >> picture)

Checking Input.

You check your input parameters after you have done a lot of hard work (loading the file). Check your parameter's first and fail fast. That way your application does not succ up resources from the system needlessly.

    if (argc == 4) {
        if (*argv[3] == 'X') {
            picture.FlipX();
        } else if (*argv[3] == 'Y') {
            picture.FlipY();
        } else if (*argv[3] == 'I') {
            picture.InvertPixelIntensity();
        } else {
            std::cout << "Error: " << argv[3] << " is an invalid command. Use either X, Y, or I.\n";
            return -1;
        }
    }

Don't manually close a file

The destructor will do that correctly for you. If you manually close the file you have to take extra pre-cautions to handle potential errors that could happen during closing.

Now you can care about these errors and take the appropriate actions when errors happen. But if you are not going to bother then just let the destructor do it.

    fout.close();

Read and the Strong Guarantee

There are schools of thought that either an "operation succeeded" or an "operation fails and the object remains unchanged". This is the strong guarantee.

I like this philosophy. If a read operation fails I would prefer the object that was being read into remain unchanged (ie that the state is only change if the operation succeeds completely).

Another acceptable potentially philosophy is to allow the state of change but it must be consistent. This is the basic guarantee.

Your code provides the basic guarantees. Though there is a bug (I will show that later).

  PortablePixelMap  x;
  x.ReadPPMFile("Pic1");  // Assumes this works perfectly.
                          // It loads a picture of 15 (width) 20 (height)

  x.ReadPPMFile("Pic2");  // This load fails.
                          // It sets the magic number and width (2).
                          // But the rest of the file is blank.
                          //
                          // The height remains 20 and no picture data
                          // is loaded. Thus leaving your object with
                          // what looks like real data in it but is
                          // is actually just junk.

Bug in the read().

If you call read on a PortablePixelMap twice. The second (and subsequent) are appended on the end.

    fin >> width >> height;
    fin >> intensity;

    // You need to call clear here:
    // To remove the data from the previous picture from the object.
    picture_data.clear();

    for (int i = 0; i < height; i++) {
        std::vector<Pixel> pixel_vector;
        for (int j = 0; j < width; j++) {
            Pixel temp;
            fin >> temp;
            pixel_vector.push_back(temp);
        }
        picture_data.push_back(pixel_vector);
    }

Const correctness

Methods that do not change the state of the object should be marked as const.

bool PortablePixelMap::WritePPMFile(std::ofstream& fout) const {
      // Notice this:                                    ^^^^^

This is because in C++ we often pass objects around as const reference. When you only have a const reference to an object you may only call const member functions on that object.

Ue Standard Functions:

The standard library has a whole bunch of standard functions. including std::swap() that could replace the main body of Flip?().

void PortablePixelMap::FlipX() {
    for (int i = 0; i < height/2; i++) {
        std::vector<Pixel> temp = picture_data[i];
        picture_data[i] = picture_data[picture_data.size()-i-1];
        picture_data[picture_data.size()-i-1] = temp;
    }
}

Note: The standard version of swap() is much more efficient than your version as it uses move semantics to move rather than copy the objects.

Move Semantics

C++11 introduced move semantics. When you have finished using an object but want to move its content using move semantics to move the value can be much more effecient than copying the value.

seethe extra std::move() added below to move values.

void PortablePixelMap::FlipY() {
    for (unsigned int i = 0; i < picture_data.size(); i++) {
        std::vector<Pixel> temp = std::move(picture_data[i]);
        for (unsigned int j = 0; j < temp.size(); j++) {
            temp[j] = std::move(picture_data[i][width-j-1]);
        }
        picture_data[i] = std::move(temp);
    }
}

Note: I would still use std::swap() here. I was just demonstrating move semantics.

Why is Invert() not a member function of Pixel?

Personally I would refactor the central part of this function into a member function on pixel.

void PortablePixelMap::InvertPixelIntensity() {
    for (unsigned int i = 0; i < picture_data.size(); i++) {
        std::vector<Pixel> temp = picture_data[i];
        for (unsigned int j = 0; j < temp.size(); j++) {
            temp[j].red = abs(temp[j].red - intensity);
            temp[j].green = abs(temp[j].green - intensity);
            temp[j].blue = abs(temp[j].blue - intensity);
        }
        picture_data[i] = temp;
    }
}

Also you are needlessly making a copy of each vector<Pixel> into temp then copying it back into the picture_data. Since these are vectors you are copying this is very expensive.

Make temp a reference. This makes it another name for an object so no copying is required.

 std::vector<Pixel> &   temp = picture_data[i];
 //                ^^^  Notice this. Add it then it becomes a reference.

 // You will no longer need this line:
 // picture_data[i] = temp;

Answer 3

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

Separate interface from implementation

The interface goes into a header file and the implementation (that is, everything that actually emits bytes including all functions and data) should be in a separate .cpp file. The reason is that you might have multiple source files including the .h file but only one instance of the corresponding .cpp file. In other words, split your existing Pixel.h file into a .h file and a .cpp file, and move the PortablePixelMap member functions from main.cpp into PortablePixelMap.cpp file.

Use all required #includes

Each interface (header) file should contain only and exactly the other include files that are required to understand the interface. In this case, the PortablePixelMap class relies on the Pixel class, so the PortablePixelMap.h file should have this line:

#include "Pixel.h"

Use a switch instead of long if ...else chain

The argument matching logic is much easier to see if a swtich statement is used instead of the long if...else chain. The default case can then be used solely for the error case. It won't make a huge difference, but it makes the logic easier to see and maintain.

Prefer generic streams to file streams

The interfaces in the current classes are all defined in terms of std::ifstream and std::ofstream objects, but they could just as easily be written instead to use the more generic std::istream and std::ostream objects instead and doing so allows for greater flexibility, such as the ability to read and write to a std::stringstream instead of a file. It's usually advantageous (and often no additional effort) to write interfaces to the more generic stream objects.

Use const where practical

When a Pixel is emitted to an output stream, the Pixel is not altered, so it should be declared const as:

std::ostream& operator << (std::ostream& fout, const Pixel& temp);

Prefer class to struct

The Pixel struct could be a class instead and the data members made private, thus allowing only member functions (or friend functions, as with the next suggestion) to access values. Then you could also add a member function like this:

void invert(int maxval) { 
    red = maxval - red;
    green = maxval - green;
    blue = maxval - blue;
}

Make inserter and extractor operators friends

Once the data members are private, we can allow selective access by inserter and extractor functions by declaring them as friend functions:

friend std::istream& operator >> (std::istream& fin, Pixel& temp);
friend std::ostream& operator << (std::ostream& fout, const Pixel& temp);

Reconsider the interface

At the moment, the only way to initialize a PortablePixelMap object is to read a file. This suggests that there should be a constructor which takes a std::istream& as an argument. Further, rather than returning a boolean value, the reading function could simply set std::ios_base::failbit. This flag is implicitly set if, say, the int extractions fail, but this causes that bit to be explicitly set if the formatting was wrong. So one way to write such a constructor would be this:

PortablePixelMap(std::istream &in) { 
    if (!ReadPPMFile(in)) {
        in.setstate(std::ios_base::failbit); 
    }
}

This allows the relevant section of main to be rewritten like this:

std::ifstream fin{argv[1]};
PortablePixelMap picture{fin};
if(!fin) {
    std::cout << "Error: unable to read PPM file " << argv[1] << "\n";
    return -1;
}

In this version, fin is created and opened immediately, and the picture variable is constructed using the new constructor. If either operation sets the failbit, the error message will be printed. A similar technique can be used to define an inserter as a friend of the PortablePixelMap class:

friend std::ostream& operator<<(std::ostream &out, const PortablePixelMap &ppm) {
    if (!ppm.WritePPMFile(out)) {
        out.setstate(std::ios_base::failbit);
    }
    return out;
}

And now the last part of main can be much simplified to look like this:

std::ofstream fout{argv[2]};
fout << picture;
if (!fout) {
    std::cout << "Error: unable to write PPM file " << argv[2] << "\n";
    return -1;
}

Consider using range-for syntax

The current code contains this member function:

void PortablePixelMap::InvertPixelIntensity() {
    for (unsigned int i = 0; i < picture_data.size(); i++) {
        std::vector<Pixel> temp = picture_data[i];
        for (unsigned int j = 0; j < temp.size(); j++) {
            temp[j].red = abs(temp[j].red - intensity);
            temp[j].green = abs(temp[j].green - intensity);
            temp[j].blue = abs(temp[j].blue - intensity);
        }
        picture_data[i] = temp;
    }
}

With the Pixel::invert function described above and the use of range-for syntax, this can be simplified to this:

void PortablePixelMap::InvertPixelIntensity() {
    for (auto &row : picture_data) {
        for (auto &item : row) {
            item.invert(intensity);
        }
    }
}

Use standard algorithms where practical

The FlipX function is currently like this:

void PortablePixelMap::FlipX() {
    for (int i = 0; i < height/2; i++) {
        std::vector<Pixel> temp = picture_data[i];
        picture_data[i] = picture_data[picture_data.size()-i-1];
        picture_data[picture_data.size()-i-1] = temp;
    }
}

This is not really optimal for a number of reasons, including the fact that temp is probably not a good name for the variable and that constructing and destructing all of those copies is much less efficient than it needs to be. I'd recommend rewriting it to use a the std::reverse function:

void PortablePixelMap::FlipX() {
    std::reverse(picture_data.begin(), picture_data.end());
}

Similarly, FlipY becomes:

void PortablePixelMap::FlipY() {
    for (auto &row : picture_data) {
        std::reverse(row.begin(), row.end());
    }
}

Read the specifications carefully

There are a few things that are in the ppm specification that aren't correctly handled by this code. First, the code may contain comments which are like this:

# this is a comment

Second, no line should be more than 70 characters. Both of these should be easy to fix.

Don't use leading underscores in names

Anything with a leading underscore is a reserved name in C++ (and in C). See this question for details. So this means that your Pixel constructor parameters should be renamed as should your include guard names.

Consider using a namespace

To avoid the possibility of name clashes with other possible libraries, you might consider putting your classes into their own separate namespace.