LCD Display Programming challenge solution

Question

I have submitted here my solution to the LCD Diplay programming challege(detailed here) Please review my code and let me know what you think.

// ConsoleApplication3.cpp : Defines the entry point for the console application.
//

//#include "stdafx.h"
#include <iostream>
#include <string>

using namespace std;

/*We use this array to store the graphical LCD 
representation of each number each digit is stored in an array of size 5 that 
represents each of 5 rows of the LCD output. */
const string lcd_digits_images[10][5] = { { " - ","| |","   ","| |"," - " },  
{ "   ","  |","   ","  |","   " },
{ " - ","  |"," - ","|  "," - " },
{ " - ","  |"," - ","  |"," - " },
{ "   ","| |"," - ","  |","   " },
{ " - ","|  "," - ","  |"," - " },
{ " - ","|  "," - ","| |"," - " },
{ " - ","  |","   ","  |","   " },
{ " - ","| |"," - ","| |"," - " },
{ " - ","| |"," - ","  |"," - " } };

/*draws any section that could occupy a '-' in the LCD representation of a number. 
These are sectons 1, 3 and 5
digits_to_draw represents the digits to be drawn.
rowNo is the section number. 
size represents the size parameter mentioned in the spec. 
number_of_digits is the number of digits that for the number to be drawn.*/
void draw_horizontal_row(int* digits_to_draw, int rowNo, int size, int number_of_digits ){
    for (int i = 0; i < number_of_digits; i++) {
        if (lcd_digits_images[digits_to_draw[i]][rowNo].compare(" - ") == 0) {
            cout << " ";
            for (int j = 0; j < size; j++) {
                cout << "-";
            }
            cout << " ";
            if (i < number_of_digits - 1) {
                cout << " ";
            }           
        }
        else {
            for (int i = 0; i < size + 2; i++) {
                cout << " ";
            }
            if (i < number_of_digits - 1) {
                cout << " "; //A space follows each digit drawn except the last one. 
            }
        }

    }

    cout << endl; //After drawing the digits, we add a blank line to seperate our output. 

}


/*draws any section that could occupy a '|' in the LCD representation of a number.
These are sectons 2 and 4 of the 
digits_to_draw represents the digits to be drawn.
rowNo is the section number.
size represents the size parameter mentioned in the spec.
number_of_digits is the number of digits that for the number to be drawn.*/
void draw_vertical_row(int* digits_to_draw, int rowNo, int size, int number_of_digits) {
    for (int i = 0; i < size; i++) {
        for (int i = 0; i < number_of_digits; i++) {
            if (lcd_digits_images[digits_to_draw[i]][rowNo].compare("  |") == 0) {
                for (int i = 0; i < size+1; i++) {
                    cout << " ";
                }
                cout << "|";

            }
            else if (lcd_digits_images[digits_to_draw[i]][rowNo].compare("|  ") == 0) {
                cout << "|";
                for (int i = 0; i < size + 1; i++) {
                    cout << " ";
                }
            }
            else {
                cout << "|";
                for (int i = 0; i < size; i++) {
                    cout << " ";
                }
                cout << "|";
            }

            if (i < number_of_digits - 1) {
                cout << " ";//A space follows each digit drawn except the last one. 
            }

        }

        cout << endl; //After drawing the digits, we add a blank line to seperate our output. 
    }
}


int main()
{
    int size, number;
    string number_to_draw;
    while (cin >> size >> number_to_draw) {
        number = stoi(number_to_draw);
        if (number == 0 && size == 0) {
            return 0;
        }

        int* numbers = new int[number_to_draw.length()];

        for (int i = 0; i < number_to_draw.length(); i++) {
            numbers[i] = number_to_draw.at(i) - '0' ;
        }
        int rowNo = 0; 
        draw_horizontal_row(numbers, rowNo, size, number_to_draw.length());
        rowNo++;
        draw_vertical_row(numbers, rowNo, size, number_to_draw.length());
        rowNo++;
        draw_horizontal_row(numbers, rowNo, size, number_to_draw.length());
        rowNo++;
        draw_vertical_row(numbers, rowNo, size, number_to_draw.length());
        rowNo++;
        draw_horizontal_row(numbers, rowNo, size, number_to_draw.length());
        cout << endl; 
    }

}

Answer 1

I see some things that you might want to use to improve your code.

Don't abuse using namespace std

Especially in a very simple program like this, there's little reason to use that line. Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.

Use const where practical

The draw_vertical_row and draw_horizontal_row functions do not alter the passed digits_to_draw array and therefore that parameter should be declared const.

Enhance readability of data structures

At the moment, the first two data items in lcd_digits_images are these:

{ " - ","| |","   ","| |"," - " },  
{ "   ","  |","   ","  |","   " },

It's not very easy for a human to verify their correctness or to change them if needed. Consider instead formatting them like this:

{ " - ",
  "| |",
  "   ",
  "| |",
  " - " },  
{ "   ",
  "  |",
  "   ",
  "  |",
  "   " },

It's now a lot easier to see what they are.

Don't use std::endl if you don't really need it

The difference betweeen std::endl and '\n' is that '\n' just emits a newline character, while std::endl actually flushes the stream. This can be time-consuming in a program with a lot of I/O and is rarely actually needed. It's best to only use std::endl when you have some good reason to flush the stream and it's not very often needed for simple programs such as this one. Avoiding the habit of using std::endl when '\n' will do will pay dividends in the future as you write more complex programs with more I/O and where performance needs to be maximized.

Don't leak memory

Each time through the main loop, a new array numbers is allocated but that array is never freed. This is a memory leak which must be fixed. Either explicitly free the memory, or better, don't explicitly allocate it in the first place. See the next suggestion for that.

Avoid using new and delete directly

Rather than using new to allocate the numbers array, better would be either use something like a std::vector which could be automatically deleted as it goes out of scope or use the string directly as in the next suggestion.

Make better use of data

The data structure can be better used in this code. For example, there are two completely different routines for horizontal and vertical rows, but this could be easily inferred from the passed rowNo parameter (even lines are horizontal if we number from 0).

Secondly, in every case (both vertical and horizontal), of each three-character string, the first character is printed, then the second character repeated size times and then the third character followed by a space.

Thirdly, the only difference between horizontal and vertical lines is that vertical lines are repeated size times.

This all suggests a simple rewrite which uses your existing lcd_digits_images:

std::string expand(char ch, int row, int size) {
    int digit = ch - '0';
    if (digit < 0 || digit > 9 || row < 0 || row > 4) {
        return "";
    }
    std::string exp{lcd_digits_images[digit][row]};
    exp.insert(1, size-1, exp[1]);
    return exp;
}

void draw_row(const std::string &number_to_draw, int row, int size) {
    // only duplicate vertical (odd numbered) rows        
    for (int count = (row & 1) ? size : 1; count; --count) {
        for (const auto &ch: number_to_draw) {
            std::cout << expand(ch, row, size) << ' ';
        }
        std::cout << '\n';
    }
}

int main()
{
    int size, number;
    std::string number_to_draw;
    while (std::cin >> size >> number_to_draw) {
        number = stoi(number_to_draw);
        if (number == 0 && size == 0) {
            return 0;
        }
        for (int row = 0; row < 5; ++row) {
            draw_row(number_to_draw, row, size);
        }
    }
}

Use a class

Right now, with either the code above or your original code, there is a data structure and several closely related functions but nothing that really ties them all together. That could be done, however, using a class. An LCD class could contain one const static private data structure containing your existing digits array, and wrap up the functions as private functions and a single public function to transform a string of digits into the LCD-style output. Then the main could be rewritten to look like this:

int main()
{
    int size;
    std::string number_to_draw;
    while (std::cin >> size >> number_to_draw) {
        int number = stoi(number_to_draw);
        if (number == 0 && size == 0) {
            return 0;
        }
        LCD::printDigits(number_to_draw, std::cout, size);
    }
}

One way to do that would be with a class like this:

class LCD final {
public:
    LCD() = delete;
    static std::ostream &printDigits(const std::string &digitString, 
                            std::ostream &out = std::cout, int size = 1) {
        for (int row = 0; row < 5; ++row) {
            draw_row(digitString, row, out, size);
        }
        return out;
    }
private:
    static void draw_row(const std::string &number_to_draw, int row, 
                         std::ostream &out, int size) {
        // only duplicate vertical (odd numbered) rows        
        for (int count = (row & 1) ? size : 1; count; --count) {
            for (const auto &ch: number_to_draw) {
                out << expand(ch, row, size) << ' ';
            }
            out << '\n';
        }
    }
    static std::string expand(char ch, int row, int size) {
        int digit = ch - '0';
        if (digit < 0 || digit > 9 || row < 0 || row > 4) {
            return "";
        }
        std::string exp{lcd_digits_images[digit][row]};
        exp.insert(1, size-1, exp[1]);
        return exp;
    }
    static const std::string lcd_digits_images[10][5]; 
};

The data structure is as before:

const std::string LCD::lcd_digits_images[10][5]={ 
{ " - ",        // 0
  "| |",
  "   ",
  "| |",
  " - " },  

{ "   ",        // 1
  "  |",
  "   ",
  "  |",
  "   " },

// etc.

};

Answer 2

There is a problem with the space between characters and especially when entering "2 18" (see below):

2 18
     --
   | |  |
   | |  |
     --
   | |  |
   | |  |
     --

instead of:

2 18
      --
   | |  |
   | |  |
      --
   | |  |
   | |  |
      --

Problem solved: in the draw_horizontal_row() the index of the main loop is override by the else second loop.

Use the variable 'j' as follow:

else {
    for (int j = 0; j < size + 2; j++) {
        std::cout << " ";
    }
    if (i < number_of_digits - 1) {
        std::cout << " "; //A space follows each digit drawn except the last one. 
    }
}

Instead of:

else {
    for (int i = 0; i < size + 2; i++) { // the local 'i' override loop 'i'
        std::cout << " ";
    }
    if (i < number_of_digits - 1) {
        std::cout << " "; //A space follows each digit drawn except the last one. 
    }
}

Be also careful in draw_vertical_row() where 'i' is reuse in the 3 levels of loop. Use 3 different variable names ( i, j & k):

void draw_vertical_row(int* digits_to_draw, int rowNo, int size, int number_of_digits) {
    for (int i = 0; i < size; i++) {
        for (int j = 0; j < number_of_digits; j++) {
            if (lcd_digits_images[digits_to_draw[j]][rowNo].compare("  |") == 0) {
                for (int k = 0; k < size+1; k++) {
                    std::cout << " ";
                }
                std::cout << "|";

            }
            else if (lcd_digits_images[digits_to_draw[j]][rowNo].compare("|  ") == 0) {
                std::cout << "|";
                for (int k = 0; k < size + 1; k++) {
                    std::cout << " ";
                }
            }
            else {
                std::cout << "|";
                for (int k = 0; k < size; k++) {
                    std::cout << " ";
                }
                std::cout << "|";
            }

            if (j < number_of_digits - 1) {
                std::cout << " ";//A space follows each digit drawn except the last one. 
            }

        }

        std::cout << std::endl; //After drawing the digits, we add a blank line to seperate our output. 
    }
}