C++: Issues with manipulating a PPM file

Question

I'm working on a program that can perform various effects and manipulations on a PPM file. However for testing reasons, it uses cin rather than an input file. It is supposed to be able to perform multiple effects at once, but I am having trouble even getting one right. I'll run a removeBlue() on a line that will work, then try again with different values and it will remove red or green instead. That sort of thing. There's a lot of code, so I'll try to include only what is necessary.

#include <vector>
#include <stdlib.h>
#include <cstdlib>
#include <string>
#include <vector>
#include <fstream>
#include <sstream>
#include <iostream>
using namespace std;

class SimpleImageEffect
{
public:
    virtual void processImage(vector<Point> &points) = 0;
};

class RemoveRed : public SimpleImageEffect
{
public:

    virtual void processImage(vector<Point> &points)
    {
        for (Point& p : points)
        {
            p.setRed(0);
        }
    }
};
//Just an example of one of the effect classes. 
//The code in them is correct, so I won't include the others unless needed.

vector<Point> parse_line(string line)
{
    istringstream scanner{line};
    vector<Point> result{};
    int red = -1;
    int green = -1;
    int blue = -1;
    int counter = 0;

    while(scanner.good())
    {
        if (counter == 0)
        {
            counter++;
            scanner >> red;
        }
        else if (counter == 1)
        {
            counter++;
            scanner >> green;
        }
        else if (counter == 2)
        {
            scanner >> blue;
            Point p{ red, green, blue };
            result.push_back(p);
            counter = 0;
        }
    }
    return result;
}

void readFromCin()
{
    string line = "";

    vector<string> lines_in_file{};
    int i, effect_choice;
    SimpleImageEffect *effect = nullptr;

    getline(cin, line);

    while (line.length() > 0)
    {
        lines_in_file.push_back(line);
        getline(cin, line);
    }

    for (int i = 0; i < lines_in_file.size(); i++)
    {
        if (lines_in_file[i] != "P3")
        {
            effect_choice = strToInt(lines_in_file[i]);
        }

        else if (lines_in_file[i] == "P3")
        {
            cout << lines_in_file[i] << endl;
            cout << lines_in_file[i+1] << endl;
            cout << lines_in_file[i+2] << endl;
        }

        vector<Point> points = parse_line(lines_in_file[i]);

        if (effect_choice == 1) effect = new RemoveRed;
        if (effect_choice == 2) effect = new RemoveGreen;
        if (effect_choice == 3) effect = new RemoveBlue;
        if (effect_choice == 4) effect = new NegateRed;
        if (effect_choice == 5) effect = new NegateGreen;
        if (effect_choice == 6) effect = new NegateBlue;
        if (effect_choice == 7) effect = new AddNoise;
        if (effect_choice == 8) effect = new HighContrast;
        if (effect_choice == 9) effect = new ConvertToGrayscale;

        effect->processImage(points);

        for (auto p : points)
        {
            cout << p;
            cout << endl;
        }
    }
}

int main(int argc, char** argv)
{
    string menu_choice;
    getline(cin, menu_choice);
    if (menu_choice == "1")
    {
        readFromFile();
    }
    else
    {
        readFromCin();
    }
    return 0;
}

So for example, running it with an input of

2
1
P3
1 1
255
50 50 50

will return

P3
1 1
255
0 50 50

but if I run it with

2
3
P3
1 2
255
50 50 50
1 2 3

it returns

P3
1 2
255
0 50 50
0 2 3

I have absolutely no idea what's causing the issue, so any help at all would be greatly appreciated. Thanks.

Answer 1

Your algorithm logic structure smells a lot, this is what I see:

• read all non empty lines into lines_in_file (looks good to me)
• for EVERY line (problematic, requires additional logic in inner loop):
  • if not "P3", try to parse [EVERY] line as integer and set effect_choice (it's not clear from your code, what happens on lines where several integers are provided, but judging from your problem description the first integer is successfully parsed by strToInt function)
  • if "P3", the current line and next two are copied to output
  • [EVERY] line is parsed as vector of triplets of numbers
  • effect is set by new effect for actual value of effect_choice (for EVERY line, also you don't delete the effect at end, so you are leaking memory in per-line counts. Also your current effects look like they may be implemented as static functions of "process" function type, so you don't need to allocate each of them, just store the particular memory address of requested function. And you call it processImage, while you are processing only line, not whole image.
  • effect is run for current line triplets
  • the line triplets are outputted
• loop to next line (!)

So for example for input:

2
3
P3
1 2
255
50 50 50
1 2 3

I believe (can't run it, as you didn't provide lot of code) this happens:

lines are read, and per particular line this happens:

line "2": effect_choice = 2, effect = RemoveGreen, zero triplets parsed into points, RemoveGreen::processImage() run over empty vector, empty vector printed (ie nothing).

line "3": effect_choice = 3, effect = RemoveBlue, zero triplets parsed into points, RemoveBlue::processImage() run over empty vector, empty vector printed.

line "P3": Lines: {"P3", "1 2", "255"} are printed, zero triplets parsed into points, RemoveGreen::processImage() run over empty vector, empty vector printed.

line "1 2": effect_choice = 1, effect = RemoveRed, zero triplets parsed into points, RemoveRed::processImage() run over empty vector, empty vector printed.

line "255": effect_choice = 255, zero triplets parsed into points, RemoveRed::processImage() run over empty vector, empty vector printed.

line "50 50 50": effect_choice = 50, one triplet {50, 50, 50} parsed into points, RemoveRed::processImage() run over it, modified triplet outputs {0, 50, 50}.

line "1 2 3": effect_choice = 1, effect = RemoveRed, one triplet {1, 2, 3} parsed into points, RemoveRed::processImage() run over it, modified triplet outputs {0, 2, 3}.

All of this should be clearly visible in debugger, while stepping over the code, so you probably are not debugging it, which gets downvoting the question from me, and you will pay in tremendous pain over time, as debugging without debugger is lot more difficult.

Also writing code without thinking about algorithm and code architecture makes the need of debugging lot more likely, so you wasted even more time here, by starting by writing the code.

You should have first design some algorithm and code architecture (what data are processed, how, when new memory is needed, how it will be freed, where the code need to loop, where it need to skip over, or run only once, etc).

Write only overview of how it will work into single-line comments, then split too generic comments into simpler steps until they can be implemented by few lines of C++ code, and move/modify them around until you feel the wanted algorithm will be implemented with minimal "cruft" added (most of the comments does, what is really requested, like "set red in point to zero", and any processing/preparations/moving/etc is minimized only to cases where you can't avoid it by smarter design). (for example in your current code you can read through the header of the file without looping, and start looping only after the pixel data pours in)

Then write the code, start probably with some empty function definition so you can already "run" it in debugger and verify the emptiness works, then implement the comment (or small group of them) which you feel is clear enough to be implemented and can be tested easily (no big dependency on yet-to-implement parts). Debug + test new code. If it works, try to clean up the source to remove anything not really needed, work-in-progress variable names, etc... Then verify it works in final version.

And do it again for another comment (group of), until the implementation is done.

Using unit-testing makes the write-short-code, test+debug, clean-up-source rounds even easier, especially in cases like this, where I/O are pure data, so it's easy to feed specialized test input data into test, and verify the expected output data were produced.