4
\$\begingroup\$

As a senior in Computer Science, have a long way to go towards improving myself. I'd appreciate any and all criticism.

Problem Set

Write a program that can draw a scalable checkered grid (like a chessboard) to any dimension.

#include <iostream>
using std::cout;
using std::cin;


void printSmallSquares(int SquareSize, int width, bool cond) {
bool stars = cond;
    printf("%s", "\n*");
    for (int i = 0; i < width; i++) {
        for (int i = 0; i < SquareSize; i++)
        {
            printf("%s", (stars ? "*" : " "));  
        }
    stars = !stars;
}
printf("%s", "*");
}

int main() {
int Height = 8;
int Width = 16;
int SquareSize = 6;

for (int j = 0; j < Height; j++) {
    for (int i = 0; i < Width * SquareSize + 2; i++) {
        printf("%s", "*");
    }

    for (int i = 0; i < SquareSize; i++) {
        (j % 2 == 0 ? printSmallSquares(SquareSize, Width, true) : printSmallSquares(SquareSize, Width, false));
    }
    printf("%s", "\n");
}
for (int i = 0; i < Width * SquareSize + 2; i++) {
    printf("%s", "*");
}
cin >> Width;

}
\$\endgroup\$
  • \$\begingroup\$ Hello and Welcome to Code Review. It would be nice of you to include a short problem description in your question (links may be subject to digital rot in the long time). \$\endgroup\$ – Ben Steffan Feb 11 '18 at 20:55
  • \$\begingroup\$ Thank you, I have remedied my post to add a problem description. \$\endgroup\$ – SierraMyrn Feb 12 '18 at 0:50
4
\$\begingroup\$

There are some things to consider:

  1. You already have using std::cout, however you use printf

    for (int i = 0; i < Width * SquareSize + 2; i++) {
        std::cout << "*";
    } 
    
  2. Edit: See that std::cout takes a std::string, so you should include that header.
    C++ mainly works with std::string rather than plain char arrays. Now if you look at std::string there is this constructor string (size_t n, char c) So what you can do now is replace the loop by a constructor:

    std::cout << std::string(Width * SquareSize + 2, "*");
    
  3. You are passing the result of a boolean comparison as a boolean:

    for (int i = 0; i < SquareSize; i++) {
        (j % 2 == 0 ? printSmallSquares(SquareSize, Width, true) : printSmallSquares(SquareSize, Width, false));
    }
    

    Just pass the comparison directly:

    for (int i = 0; i < SquareSize; i++) {
        printSmallSquares(SquareSize, Width, j % 2 == 0);
    }
    
  4. Obviously you should use a appropriate class for your task:

    class DrawCheckedGrid {
    
        DrawCheckedGrid(const int heigth, const int width const int size)
            : Height(height)
            , Width(width)
            , SquareSize(size)
        {}
    private:
        int Height;
        int Width;
        int SquareSize;
    }
    

    Now that you have a class, you can preallocate all you need

    std::string stars = std::string(SquareSize, "*");
    std::string spaces = std::string(SquareSize, " ");
    std::string separator = std::string(Width * SquareSize + 2, "#") + "\n";
    

    Also you can simply create some helper methods that draw the grid rather than plumbing everything into main():

    void drawLines(bool starsFirst) const {
        for (int line = 0; line < Width; ++line) {
            drawLine(starsFirst);
        }
        std::cout << separator;
    }
    
    void drawLine(bool starsFirst) const {
        const int offset = starsFirst ? 0 : 1;
        for (int square= offset; square < Width + offset; ++square) {
             std::cout << (square % 2 == 0 ? stars : spaces);
        }
        std::cout << "\n";
    }
    
    void drawGrid() const {
        std::cout << separator;
        for (int i = 0; i < Height; ++i) {
            drawLines(i%2 == 0);
        } 
    }
    

    See that I have created multiple small helpers that do one thing at at a time, rather than one big function.

    class DrawCheckedGrid {
        DrawCheckedGrid(const int heigth, const int width const int size)
            : Height(height)
            , Width(width)
            , SquareSize(size)
        {        
            stars = std::string(SquareSize, "*");
            spaces = std::string(SquareSize, " ");
            separator = std::string(Width * SquareSize + 2, "#") + "\n";
        }
    
        void drawGrid() const {
            std::cout << separator;
            for (int i = 0; i < Height; ++i) {
                drawLines(i%2 == 0);
            } 
        }
    private:
    
        void drawLines(bool starsFirst) const {
            for (int line = 0; line < Width; ++line) {
                drawLine(starsFirst);
            }
            std::cout << separator;
        }
    
        void drawLine(bool starsFirst) const {
            const int offset = starsFirst ? 0 : 1;
            for (int square= offset; square < Width + offset; ++square) {
                 std::cout << (square % 2 == 0 ? stars : spaces);
            }
            std::cout << "\n";
        }
        int Height;
        int Width;
        int SquareSize;
        std::string stars;
        std::string spaces;
        std::string separator;
    }
    

    Here you can also see that all internal functions are private, so we do not leak our internals. You can also add functions that allow you to change the parameters of the grid and pass something to drawGrid().

\$\endgroup\$
  • \$\begingroup\$ About point 2: "See that std::cout takes a std::string, so you should include that header." Yes and no. There is an overload for std::string, but there is also an overload for char const*, which is what is being called here. Including string is only required when doing explicit string instantiations (like your proposed improvement does). You might want to rethink your wording there. \$\endgroup\$ – Ben Steffan Feb 12 '18 at 16:21
  • 1
    \$\begingroup\$ @BenSteffan thats obviously true \$\endgroup\$ – miscco Feb 12 '18 at 21:20
  • \$\begingroup\$ Another variation would be to create each horizontal row as a string, say odd and even and printing would be for (auto row{Height}; row; --row) { std::cout << (row & 1 ? odd : even); } \$\endgroup\$ – Edward Feb 13 '18 at 13:03
  • \$\begingroup\$ I think DrawCheckedGrid is an odd name for a class (though it would be fine for a function). By removing the Draw prefix it becomes clear that this is a class with dedicated methods. \$\endgroup\$ – Daniel Feb 14 '18 at 15:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.