Graphical editor programming challenge

I have posted here my working and accepted solution to the graphical editor programming challenge (detailed here) for your review.

The program recognizes the following image editing commands:

• I M N: Creates a new table M × N (up to 250 × 250) with white (O) pixels.

• C: Clears the table to white.

• L X Y C: Colors the pixel with coordinates (X, Y) in colour C. (A "colour" is a single character.)

• V X Y1 Y2 C: Draws the vertical segment in the column X between the rows Y1 and Y2 inclusive in colour C.

• H X1 X2 Y C: Draws the horizontal segment in the row Y between the columns X1 and X2 inclusive in colour C.

• K X1 Y1 X2 Y2 C: Draws the filled rectangle in colour C. (X1, Y1) is the upper left corner, (X2, Y2) is the lower right corner of the rectangle.

• F X Y C: Fills the region with the colour C, starting with (X, Y) and including any neighbouring pixels with a common side and the same colour.

• S Name: Writes the picture in the file Name.

• X: Terminates the session.

```
// graphical_editor.cpp : Defines the entry point for the console application.
//

#include "stdafx.h"
#include //provides access to cout and cin
#include //Always import if piping std input to a string in .net
#include
#include

using std::cin;
using std::cout;
using std::string;
using std::vector;
using std::ofstream;

//This is where we store the pixels of the image
static vector> image_array;

//our definition of an X,Y coordinate pair.
typedef struct point {
int x_coordinate, y_coordinate;
};

void initialise_image();
void clear_image();
void save_image(string file_name);
int get_image_width();
int get_image_height();
void color_pixel(int x, int y, string color);
void color_point(point p, string color);
void color_vertical_line(int x, int y1, int y2, string color);
void color_horizontal_line(int x1, int x2, int y, string color);
void color_box(int x1, int x2, int y1, int y2, string color);
void flood_fill(point p, string color);
void add_matc

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

Isolate platform-specific code

If you must have stdafx.h, consider wrapping it so that the code is portable:

#ifdef WINDOWS
#include "stdafx.h"
#endif

In this case, with only a single file, there's no advantage to having it, so I'd recommend simply deleting that line.

Eliminate spurious typedef

The code currently contains this:

typedef struct point {
    int x_coordinate, y_coordinate;
};

However, in C++, the typedef is not needed since a struct definition creates a new type. Simply omit the word typedef here.

Use objects

Almost all of the functions in the code operate on a single global variable named image_array. This strongly suggests an object instead in which the image_array is a class and most of the functions are member functions instead of standalone C-style functions.

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

The command procesing is much easier to see and understand if a switch statement is used instead of the long if...else chain. The default case can then be used for unrecognized commands. On my machine, this also makes the code slightly faster.

Use the appropriate data type

The description of the problem says that a color is a single character, but it's defined as a std::string in most of the code. It seems to me that a more appropriate choice would be to define a Color type and use that. I'd probably do this:

using Color = char;

However, if there's some reason you must use a std::string for this, you could use this:

using Color = std::string;

The advantage here is that it is easy to understand the intended usage if the functions are declared using syntax like this:

void add_matching_neighbours(point p, Color original_color, Color new_color, vector &points_queue);

Eliminate unused variables

Unused variables are a sign of poor code quality, so eliminating them should be a priority. In this code, final_color gets set but is never actually used. My compiler also tells me that. Your compiler is probably also smart enough to tell you that, if you ask it to do so.

Choose easier names

The point struct is quite simple, but the names for the coordinates are x_coordinate and y_coordinate. While those are fine, descriptive names, I think they're overly long. I'd be inclined to simply name them x and y instead and save a lot of typing and shorten some really long lines that are currently within the code.

Use for instead of while where appropriate

Within the color_box function is this code:

int x = x1;
int y = y1;

while (x != x2 + x_adjustment) {
    while (y != y2 + y_adjustment) {
        color_pixel(x, y, color);
        y += y_adjustment;
    }
    x += x_adjustment;
    y = y1;
}

This can be much simplified and also easier to understand by using a for loop instead of while:

for (int x = x1; x != x2 + x_adjustment; x += x_adjustment) {
    for (int y = y1; y != y2 + y_adjustment; y += y_adjustment) {
        color_pixel(x, y, color);
    }
}

Name useful constants

In the existing code, the value "0" is used multiple times to represent the color "white". I'd recommend formalizing that equivalence by naming the constants.

static const Color white{'0'};

In this case, I've defined it as a single char per my previous suggestion.

Think carefully about memory allocation

The code for initialise_array contains theres three lines:

for (int i = 0; i ());
}

The creates each column as an empty vector, forcing the vector to resize (probably more than once) as items are added to the vector. Since we already know the size, we can eliminate a lot of reallocations by initializing the std::vector to the known correct size as we create it:

for (int i = 0; i (height+1));
}

Even better, we do the same thing to the outer vector and pass in an initialized vector:

image_array.reserve(width+1);
for (int i = 0; i (height+1, white));
}

Perform bounds checking

If we construct an image that is 64x32 using I 64 32 then attempt to create a filled box with the command L 65 20 +, the x dimension of 65 is out of bounds. Unfortunately, the program doesn't seem to notice that and attempts to use the out of bounds and crashes.

Don't repeat yourself

Here's one way to rewrite the add_matching_neighbors to reduce the repetition.

```
void Image::add_matching_neighbours(const point &p, Color original_color,
Color new_color, vector &points_queue) {
const point neighbours[4]{ {p.x - 1, p.y}, // left
{p.x + 1, p.y}, // right
{p.x, p.y + 1}, //upper
{p.x, p.y - 1}, //lower
};
for (const auto &neigh : neighbours) {
if (is_valid_point(neigh) && get_point_color(neigh) == original_color) {
points_queue.push_back(neigh);
color_point(neigh, new_colo

StackExchange Code Review Q#145639, answer score: 3