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? What are the rules about using an underscore in a C++ identifier?
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?
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?
###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;