Terminal Graphical Visualizer, using a queue of different matrices

Question

I built a project in which I provide a string input or a whole matrix to configure a frame, create a bunch of different frames and push them into a queue and finally print them in order of the queue.

I would like some advice on how to improve the design of the frame class as I feel it became a spaghetti code. I'll be happy to also know what changes I can make for better performance and maybe variable name ideas because I feel like I didn't give my variables proper and understandable names.
hpp file:

#pragma once #include <vector> #include <string> #include <queue> #include <chrono> #include <unordered_map> #include <map> using frame_matrix = std::vector<std::vector<std::pair<char, std::string>>>; const std::unordered_map<std::string, std::string> colors = {{"red", "\033[31m"}, {"green", "\033[32m"}, {"blue", "\033[34m"}, {"yellow", "\033[33m"}, {"reset", "\033[0m"}}; class frame { private: std::string color; void initialize_frame(); public: frame_matrix current_frame; const static size_t AMOUNT_OF_INPUT_OPTIONS = 6; const static size_t FRAME_WIDTH = 30; const static size_t FRAME_HEIGHT = 20; const static char INPUT_DELIMITER = ','; const static char BACKGROUND = '#'; frame(); frame(const std::string& input); frame_matrix get_current_frame() const; void alter_frame(const std::string& input); void set_current_frame(const frame_matrix& new_current_frame); void print_frame(); bool parse_input(const std::string& input); bool is_valid_input(std::string height_start, std::string height_length, std::string range_start, std::string range_length, std::string printable_char,std::string color); }; class graphical_visualizer { private: std::queue<frame> frame_queue; public: graphical_visualizer(); std::queue<frame> get_frame_queue() const; void add_frame(frame frame); void print_sequence(const std::chrono::milliseconds millis); }; struct input { size_t height_start; size_t height_length; size_t width_start; size_t width_length; char symbol; std::string color; }; 

cpp file

#include "graphical_visualizer.hpp" #include <iostream> #include <sstream> #include <thread> frame::frame() { initialize_frame(); } frame::frame(const std::string& input) { initialize_frame(); alter_frame(input); } frame_matrix frame::get_current_frame() const { return current_frame; } void frame::alter_frame(const std::string& input) { parse_input(input); } void frame::set_current_frame(const frame_matrix& new_current_frame) { current_frame = new_current_frame; } void frame::print_frame() { for(const std::vector<std::pair<char, std::string>>& line : current_frame) { for(const std::pair<char, std::string>& current_char : line) { std::cout << current_char.second << current_char.first; std::cout << colors.at("reset"); } std::cout << std::endl; } } bool frame::parse_input(const std::string& input) { std::stringstream input_stream(input); std::string input_sections[AMOUNT_OF_INPUT_OPTIONS]; size_t input_sections_index = 0; while(std::getline(input_stream, input_sections[input_sections_index], INPUT_DELIMITER)) ++input_sections_index; if(!is_valid_input(input_sections[0], input_sections[1], input_sections[2], input_sections[3], input_sections[4], input_sections[5])) { return false; } int height_start = std::stoi(input_sections[0]); int height_length = std::stoi(input_sections[1]); int range_start = std::stoi(input_sections[2]); int range_length = std::stoi(input_sections[3]); std::string color = colors.at(input_sections[5]); char printable_char = input_sections[4][0]; //should always be a string of length 1 for(size_t i = height_start; i <= height_start + height_length - 1; ++i) { for(size_t j = range_start; j <= range_start + range_length - 1; ++j) { current_frame[i][j].first = printable_char; current_frame[i][j].second = color; } } return true; } void frame::initialize_frame() { for(size_t i = 0; i < FRAME_HEIGHT; ++i) { std::vector<std::pair<char, std::string>> line; for(size_t j = 0; j < FRAME_WIDTH; ++j) line.push_back({'#', colors.at("reset")}); current_frame.push_back(line); } } bool is_a_number(std::string number) { for(const char& ch : number) { if(!std::isdigit(ch)) return false; } return true; } bool frame::is_valid_input(std::string height_start, std::string height_length, std::string range_start, std::string range_length, std::string printable_char,std::string color) { std::unordered_map<std::string, std::string>::const_iterator pos = colors.find(color); if(pos == colors.end()) return false; if(!is_a_number(height_start) || !is_a_number(height_start) || !is_a_number(height_start) || !is_a_number(height_start)) { return false; } if(printable_char.size() != 1) return false; int height_start_int = std::stoi(height_start); int height_length_int = std::stoi(height_length); int range_start_int = std::stoi(range_start); int range_length_int = std::stoi(range_length); if(height_start_int < 0 || height_length_int + height_length_int - 1 >= FRAME_HEIGHT || range_start_int < 0 || range_start_int + range_length_int - 1 >= FRAME_WIDTH) return false; return true; } graphical_visualizer::graphical_visualizer() { frame_queue = {}; } std::queue<frame> graphical_visualizer::get_frame_queue() const { return frame_queue; } void graphical_visualizer::add_frame(frame frame) { frame_queue.push(frame); } void graphical_visualizer::print_sequence(const std::chrono::milliseconds millis) { std::queue<frame> local_temp_queue = frame_queue; while(!local_temp_queue.empty()) { local_temp_queue.front().print_frame(); local_temp_queue.pop(); std::this_thread::sleep_for(millis); system("clear"); } } 

Answer 1

Don't use std::endl

Just output \n for a newline. See here.

std::size_t

The C++ standard defines std::size_t, not size_t.

const placing

I personally prefer type const& over const type&. Types are read right to left, so putting the const always to the right of the thing that is constant makes the type more consistent. Witness this comment.

Use more using

using frame_matrix = std::vector<std::vector<std::pair<char, std::string>>>; // ... for(const std::vector<std::pair<char, std::string>>& line : current_frame) { for(const std::pair<char, std::string>& current_char : line) { // ... 

This is very verbose, you're repeating std::pair<char, std::string> over and over again. You could do instead:

using matrix_element = std::pair<char, std::string>>; using frame_matrix = std::vector<std::vector<matrix_element>>; // ... for(const std::vector< matrix_element >& line : current_frame) { for(const matrix_element& current_char : line) { // ... 

But of course better is to use auto:

 for(const auto& line : current_frame) { for(const auto& current_char : line) { // ... 

Prefer struct over std::pair

std::pair is convenient to put two pieces of information together, but what does << current_char.second << current_char.first mean? I'd prefer

struct frame_matrix { char character; std::string color_code; }; // ... std::cout << current_char.color_code << current_char.character; 

It's just more readable, and it doesn't add a whole lot of code.

The matrix

There are two things about the way you store your data that makes it quite inefficient. A std::vector<std::vector<>> is usually a bad idea (double indirection, memory fragmentation, etc.). It is useful if all inner vectors have a different length, but that is not the case for you. Instead, use a simpler std::vector<>, of length FRAME_HEIGHT * FRAME_WIDTH, and while iterating simply count out how many values you read. You could iterate as follows:

 std::size_t index = 0; for(std::size_t i = 0; i < FRAME_HEIGHT; ++i) { for(std::size_t j = 0; j < FRAME_WIDTH; ++j, ++index) { std::cout << current_frame[index].color_code << current_frame[index].character; std::cout << colors.at("reset"); } std::cout << '\n'; } 

When initializing the frame, you push_back() into an empty vector. Since you know what size the vector will have, you can reserve() the space first. This way the vector won't have to resize continuously, resizing is costly.

Using a plain vector instead of a vector of vectors makes initializing to an empty frame a lot easier as well. No loops!

Finally, the representation of the color in the frame could probably be simplified to make it more efficient. std::string is complex. All your color strings are the same size, so you don't need any of the complexity of std::string. One possible solution could be to using constant strings (char arrays) for your color codes, and put a pointer in your matrix:

struct frame_matrix { char character; char const* color_code; }; const std::unordered_map<std::string, char const*> colors = {{"red", "\033[31m"}, {"green", "\033[32m"}, {"blue", "\033[34m"}, {"yellow", "\033[33m"}, {"reset", "\033[0m"}}; 

This way you only store a pointer to the commonly used string, instead of copying it again and again.

Variable names

When you declare void add_frame(frame frame), you make things quite confusing. frame now means something different inside the function as it does outside. It is legal, but you're not making it easier on the reader. It is quite common in C++ to name classes with an uppercase letter. In this case your function declaration would be void add_frame(Frame frame), which is a lot less ambiguous.

Spaghetti code

I don't think this is spaghetti code, the structure is quite clear. One thing you could do to simplify the code is putting shorter function definitions in the header right where you declare them. This way the compiler can do more optimizations, and you reduce code duplication a bit.

class frame { private: std::string color; void initialize_frame(); public: frame_matrix current_frame; const static size_t AMOUNT_OF_INPUT_OPTIONS = 6; const static size_t FRAME_WIDTH = 30; const static size_t FRAME_HEIGHT = 20; const static char INPUT_DELIMITER = ','; const static char BACKGROUND = '#'; frame() { initialize_frame(); } frame(const std::string& input) { initialize_frame(); alter_frame(input); } frame_matrix get_current_frame() const { return current_frame; } void alter_frame(const std::string& input) { parse_input(input); } void set_current_frame(const frame_matrix& new_current_frame) { current_frame = new_current_frame; } void print_frame(); bool parse_input(const std::string& input); bool is_valid_input(std::string height_start, std::string height_length, std::string range_start, std::string range_length, std::string printable_char, std::string color); }; 

Things to think about: What is the purpose of alter_frame() if all it does is call parse_input()? You have one function with two names! And do you really want is_valid_input() to be part of the public API? Should this not be a private function?

By the way, is_valid_input() takes std::strings by value. This should be const references:

 bool is_valid_input( std::string const& height_start, std::string const& height_length, std::string const& range_start, std::string const& range_length, std::string const& printable_char, std::string const& color ); 

is_valid_input() also converts the inputs to numbers, the result of the conversion is discarded. You call is_valid_input() in parse_input(), which also converts these strings to numbers. It is good to separate out functionality, but maybe put more of the validation in parse_input() (just verify the limits for the integers), so that you don't need to duplicate code.