2
\$\begingroup\$

I would really appriciate if someone could review my code and give me feedback. This was my first multi file project.

Main:

#include <iostream>
#include <vector>
#include "Pad.h"
#include <conio.h>
#include <Windows.h>
#include <iostream>
#include "Ball.h"
#include"consoleFunctions.h"

int main()
{
    Map map{};
    Ball ball{};
    FirstPad firstPad{map};
    SecondPad secondPad{ map };
    WhoScored whoScored{ none };
    map.print();
    int ballCounter{};
    int secondPadCounter{};
    while (firstPad.getScore() < 10 && secondPad.getScore() < 10) { // game finishes when one player has 10 points
        
        map.print();
        std::cout << "Score: " << firstPad.getScore() << ":" << secondPad.getScore();
        if (_kbhit()) {
            firstPad.move(map);
        }
    
        if (ballCounter == 13) { //slowing down ball
        ball.move(map, whoScored);
        ballCounter = 0;
         }
        if (secondPadCounter == 11) { //slowing down computer paddle movement
            secondPad.move(ball, map);
            secondPadCounter = 0;
        }
        if (whoScored == leftPad) { 
            firstPad.increaseScore(); 
            
        }
        else if (whoScored == rightPad) {
            secondPad.increaseScore(); 
          
        }
        whoScored = none;
        ++ballCounter;
        ++secondPadCounter;
   }
    set_cursor( MAP_WIDTH / 2, MAP_HEIGHT / 2);
    if (firstPad.getScore() == 10) std::cout << "LEFT PAD WINS";
    else std::cout << "RIGHT PAD WINS";
    std::cin.ignore();
  
}

Map.h

#pragma once
#include <vector>
#include <iostream>
#include"EnumsAndStructs.h"

class Map {
private:    
    std::vector<std::vector<Objects>> m_map; //matrix which holds map   
public:
    Map();
    void print();
    std::vector<std::vector<Objects>>& getMap() { return m_map; }
    
};

Map.cpp

#include "map.h"
#include<Windows.h>
#include"consoleFunctions.h"
#include<sstream>
extern const int MAP_HEIGHT{ 25 };
extern const int MAP_WIDTH{ 100 };

Map::Map() {
    //resizing array
    m_map.resize(MAP_HEIGHT);
    for (int i{ 0 }; i < MAP_HEIGHT; ++i) {
        m_map[i].resize(MAP_WIDTH);
    }
    //setting map for begining 
    for (int i{ 0 }; i < MAP_HEIGHT; ++i) {
        for (int j{ 0 }; j < MAP_WIDTH; ++j) {
            if (j == 0 || j == MAP_WIDTH - 1) {
            m_map[i][j] = Objects::VERTICAL_BORDER;
            }
            else if (i == 0 || i == MAP_HEIGHT - 1) {
                m_map[i][j] = Objects::HORISONTAL_BORDER;
            }
            
            else {
                m_map[i][j] = Objects::NOTHING;
            }
        }
        m_map[MAP_HEIGHT / 2][MAP_WIDTH / 2] = Objects::BALL;
    }
}
void Map::print() {
    set_cursor();
    cursor_off();
    std::ostringstream ss;
    for (int i{ 0 }; i < MAP_HEIGHT; ++i) {
        for (int j{ 0 }; j < MAP_WIDTH; ++j) {
            if (m_map[i][j] == Objects::HORISONTAL_BORDER) {
                ss << '-';
            }
            else if (m_map[i][j] == Objects::VERTICAL_BORDER) {
                ss << static_cast<char>(0XB3);
            }
            else if (m_map[i][j] == Objects::PAD) {
                ss << static_cast<char>(0XB3);
            }
            else if(m_map[i][j] == Objects::BALL){
                ss << '0';
            }
            else {
                ss << ' ';
            }
        }
        ss << '\n';
    }
    std::cout << ss.str();
}

Pad.h

#pragma once
#include <vector>
#include "EnumsAndStructs.h"
#include "Ball.h"


extern const int MAP_HEIGHT;
extern const int MAP_WIDTH;
class Ball;
class Map;
class Pad {
protected:
    int m_score{};
    const int m_height{ 3 };
    Coordinates m_coordinates{};
    void setPadInitially(Map& map); //initially sets pads on map, used only in constructor
public:
    const Coordinates& getCoordinates() const { return m_coordinates; }
    int getHeight() const;
    int getScore() const { return m_score; }
    void increaseScore() { ++m_score; }

};
class FirstPad : public Pad {
private:
    Direction m_direction{ Direction::NONE };
    void takeDirection();
public:
    FirstPad(Map& map); 
    void move(Map& map); 
};
class SecondPad : public Pad {
public:
    SecondPad(Map& map);
    void move(const Ball& ball, Map& map);
};

Pad.cpp

#include "Pad.h"
#include <conio.h>

const int firstPadX{ 5 };
const int secondtPadX{ MAP_WIDTH - 7 };
constexpr auto KEY_UP = 72;
constexpr auto KEY_DOWN = 80;
// class Pad
int Pad::getHeight() const { return m_height; }
void Pad::setPadInitially(Map& map) {
    int j{};
    for (int i{ m_coordinates.m_y }; j < m_height; ++i) {
        map.getMap()[i][m_coordinates.m_x] = Objects::PAD;
        ++j;
    }
}
//class FirstPad
FirstPad::FirstPad(Map& map)
{
    m_coordinates.m_x = firstPadX;
    m_coordinates.m_y = (MAP_HEIGHT / 2) - (m_height / 2);
    int j{};
    setPadInitially(map);
}
void FirstPad::takeDirection() {
    
    auto input{ _getch() };
    switch (input) {
    case KEY_DOWN: m_direction = Direction::DOWN;
        break;
    case KEY_UP: m_direction = Direction::UP;
        break;
    default: m_direction = Direction::NONE;
    }
}
void FirstPad::move(Map& map) {
    //chagning y coordinate as pad goes up and down
    takeDirection();
    if (m_direction == Direction::UP) {
        if (m_coordinates.m_y > 1) {
            --m_coordinates.m_y;
            map.getMap()[m_coordinates.m_y][m_coordinates.m_x] = Objects::PAD;
            map.getMap()[m_coordinates.m_y + m_height][m_coordinates.m_x] = Objects::NOTHING;
        }
    }
    else if (m_direction == Direction::DOWN) {
        if (m_coordinates.m_y + m_height < MAP_HEIGHT - 1) {
            ++m_coordinates.m_y;
            map.getMap()[m_coordinates.m_y + m_height - 1][m_coordinates.m_x] = Objects::PAD;
            map.getMap()[m_coordinates.m_y - 1][m_coordinates.m_x] = Objects::NOTHING;
        }
    }
    
}
// class SecondPad
SecondPad::SecondPad(Map& map) {
    m_coordinates.m_x = secondtPadX;
    m_coordinates.m_y = (MAP_HEIGHT / 2) - (m_height / 2);
    setPadInitially(map);
    
}
void SecondPad::move(const Ball& ball, Map& map) {
    // Coumputer controls this pad, basically follows ball
    int padMiddleY = m_coordinates.m_y + m_height / 2;
    if (padMiddleY < ball.getCoordinates().m_y && m_coordinates.m_y < MAP_HEIGHT - m_height - 1) {
        ++m_coordinates.m_y;
        map.getMap()[m_coordinates.m_y + m_height - 1][m_coordinates.m_x] = Objects::PAD;
        map.getMap()[m_coordinates.m_y - 1][m_coordinates.m_x] = Objects::NOTHING;
    }
    else if (padMiddleY > ball.getCoordinates().m_y && m_coordinates.m_y > 1) {
        --m_coordinates.m_y;
        map.getMap()[m_coordinates.m_y][m_coordinates.m_x] = Objects::PAD;
        map.getMap()[m_coordinates.m_y + m_height][m_coordinates.m_x] = Objects::NOTHING;
    }
}

Ball.h

#pragma once
#include "EnumsAndStructs.h"
#include "Map.h"
extern const int MAP_HEIGHT;
extern const int MAP_WIDTH;
class Map;
class Ball
{
private:
    Coordinates m_coordinates{};
    Direction m_direction{Direction::RIGHT_DOWN};

    
    void handleColision(Map& map, WhoScored& whoScored);
public:
    Ball()
        : m_coordinates{MAP_WIDTH/ 2, MAP_HEIGHT / 2} // puts ball in the middle
    {}
    void move(Map& map, WhoScored& whoScored);
    void reflectDirection();
    const Coordinates& getCoordinates() const { return m_coordinates; }
};

Ball.cpp

#include "Ball.h"

void Ball::reflectDirection() {
    switch (m_direction) {
    case Direction::RIGHT_UP: {
        m_direction = Direction::LEFT_UP;
        return;
    }
    case Direction::RIGHT_DOWN: {
        m_direction = Direction::LEFT_DOWN;
        return;
    }
    case Direction::LEFT_UP: {
        m_direction = Direction::RIGHT_UP;
        return;
    }
    case Direction::LEFT_DOWN: {
        m_direction = Direction::RIGHT_DOWN;
        return;
    }
    }
    return;
}
void Ball::handleColision(Map& map, WhoScored& whoScored) {
//handling bouncing on horisiontal walls and pads and checking who scored on verticals
    if (map.getMap()[m_coordinates.m_y][m_coordinates.m_x] == Objects::HORISONTAL_BORDER) {
        if (m_direction == Direction::RIGHT_UP) { m_direction = Direction::RIGHT_DOWN; }
        else if (m_direction == Direction::RIGHT_DOWN) { m_direction = Direction::RIGHT_UP; }
        else if (m_direction == Direction::LEFT_UP) { m_direction = Direction::LEFT_DOWN; }
        else if (m_direction == Direction::LEFT_DOWN) { m_direction = Direction::LEFT_UP; }
    }
    else if (map.getMap()[m_coordinates.m_y][m_coordinates.m_x] == Objects::VERTICAL_BORDER) {
        if (m_coordinates.m_x < MAP_WIDTH / 2) { //if it is left side                   
            whoScored = rightPad;
        }
        else whoScored = leftPad;
        m_coordinates.m_y = MAP_HEIGHT / 2;
        m_coordinates.m_x = MAP_WIDTH / 2;
        map.getMap()[m_coordinates.m_y][m_coordinates.m_x] = Objects::BALL;
    }
    else if (map.getMap()[m_coordinates.m_y][m_coordinates.m_x] == Objects::PAD) {
        reflectDirection();
    }
}
void Ball::move(Map& map, WhoScored& whoScored) {
    //ball will only move on 45 degrees, pretty simple
    //if balls next position is bounacable object call handleColision
    //whoScored is not needed in this function but in handleColision
    if (m_direction == Direction::RIGHT_DOWN) {
        if (map.getMap()[m_coordinates.m_y][m_coordinates.m_x] != Objects::VERTICAL_BORDER && //without this ifs our objects will disappear as ball bounces of them
            map.getMap()[m_coordinates.m_y][m_coordinates.m_x] != Objects::HORISONTAL_BORDER &&
            map.getMap()[m_coordinates.m_y][m_coordinates.m_x] != Objects::PAD) {

            map.getMap()[m_coordinates.m_y][m_coordinates.m_x] = Objects::NOTHING;
        }
        ++m_coordinates.m_y;
        ++m_coordinates.m_x;
        if (map.getMap()[m_coordinates.m_y][m_coordinates.m_x] != Objects::NOTHING) {
            handleColision(map,whoScored);
            
        }
        else map.getMap()[m_coordinates.m_y][m_coordinates.m_x] = Objects::BALL;
    }
    else if (m_direction == Direction::RIGHT_UP) {
        if (map.getMap()[m_coordinates.m_y][m_coordinates.m_x] != Objects::VERTICAL_BORDER &&
            map.getMap()[m_coordinates.m_y][m_coordinates.m_x] != Objects::HORISONTAL_BORDER &&
            map.getMap()[m_coordinates.m_y][m_coordinates.m_x] != Objects::PAD) {
            map.getMap()[m_coordinates.m_y][m_coordinates.m_x] = Objects::NOTHING;
        }
        --m_coordinates.m_y;
        ++m_coordinates.m_x;
        if (map.getMap()[m_coordinates.m_y][m_coordinates.m_x] != Objects::NOTHING) {
            handleColision(map, whoScored);

        }
        else map.getMap()[m_coordinates.m_y][m_coordinates.m_x] = Objects::BALL;
    }
    else if (m_direction == Direction::LEFT_DOWN) {
        if (map.getMap()[m_coordinates.m_y][m_coordinates.m_x] != Objects::VERTICAL_BORDER &&
            map.getMap()[m_coordinates.m_y][m_coordinates.m_x] != Objects::HORISONTAL_BORDER &&
            map.getMap()[m_coordinates.m_y][m_coordinates.m_x] != Objects::PAD) {
            map.getMap()[m_coordinates.m_y][m_coordinates.m_x] = Objects::NOTHING;
        }
        ++m_coordinates.m_y;
        --m_coordinates.m_x;
        if (map.getMap()[m_coordinates.m_y][m_coordinates.m_x] != Objects::NOTHING) {
            handleColision(map,whoScored);
            
        }
        else map.getMap()[m_coordinates.m_y][m_coordinates.m_x] = Objects::BALL;
    }
    else if (m_direction == Direction::LEFT_UP) {
        if (map.getMap()[m_coordinates.m_y][m_coordinates.m_x] != Objects::VERTICAL_BORDER &&
            map.getMap()[m_coordinates.m_y][m_coordinates.m_x] != Objects::HORISONTAL_BORDER &&
            map.getMap()[m_coordinates.m_y][m_coordinates.m_x] != Objects::PAD) {
            map.getMap()[m_coordinates.m_y][m_coordinates.m_x] = Objects::NOTHING;
        }
        --m_coordinates.m_y;
        --m_coordinates.m_x;
        if (map.getMap()[m_coordinates.m_y][m_coordinates.m_x] != Objects::NOTHING) {
            handleColision(map, whoScored);
            
        }
        else map.getMap()[m_coordinates.m_y][m_coordinates.m_x] = Objects::BALL;
    }
}

EnumsAndStructs.h

#pragma once
enum class Objects {
    NOTHING,
    PAD,
    VERTICAL_BORDER,
    HORISONTAL_BORDER,
    BALL
};
enum class Direction {
    NONE,
    UP,
    DOWN,
    RIGHT,
    LEFT,
    RIGHT_UP,
    RIGHT_DOWN,
    LEFT_UP,
    LEFT_DOWN,
};
struct Coordinates {
    int m_x;
    int m_y;
};
enum WhoScored {
    none,
    leftPad,
    rightPad,
};

consoleFunctions.h

#pragma once
void cursor_off();//stops blinking cursor
void set_cursor(int x = 0, int y = 0);

consoleFucntions.cpp

#include<Windows.h>
void cursor_off()
{
    CONSOLE_CURSOR_INFO console_cursor;
    console_cursor.bVisible = 0;
    console_cursor.dwSize = 1;
    SetConsoleCursorInfo(GetStdHandle(STD_OUTPUT_HANDLE), &console_cursor);
}

void set_cursor(int x = 0, int y = 0)
{
    HANDLE handle;
    COORD coordinates;
    handle = GetStdHandle(STD_OUTPUT_HANDLE);
    coordinates.X = x;
    coordinates.Y = y;
    SetConsoleCursorPosition(handle, coordinates);
}
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

This looks pretty good for a first project of this size!

Consider using a curses library

You are using <conio.h> and <Windows.h>, but those are of course Windows-specific header files. You can make your program more portable by using a curses library to draw the screen and to handle the keyboard input.

There are several curses implementations that also work on Windows, the most popular one of those is probably PDCurses.

Use std::array<> if your map is going to have a fixed size

If you know the size of your map at compile time, you can use std::array instead std::vector. This is more efficient, especially if you are nesting them. Use constexpr to declare the map size, and then you can use those constants to declare the arrays:

class Map {
    static constexpr std::size_t HEIGHT = 25;
    static constexpr std::size_t WIDTH = 100;
    std::array<std::array<Objects, WIDTH>, HEIGHT> m_map;

public:
    ...
    auto& getMap() { return m_map; }
};

Consider not using m_map

Your m_map, when implemented using std::arrays, uses 10 kilobytes of memory. With std::vectors, it even needs a little more. But is it really necessary to store the map this way? You know the walls are always at the edge of the map region, and then the only two other things to worry about are the position of the paddles and the ball. Since each paddle can only move up or down, you only need four integers in total to describe the state of the map: the y-coordinate of each paddle, and the x- and y-coordinates of the ball.

Of course this means rewriting some of the code, but I don't think it will actually be more complicated than it already is now.

Remove FirstPad and SecondPad

You should not need to create different classes for each of the pads. Instead of having SecondPad::move() contain the computer player logic, move that logic out, and just add a move() function to Pad that takes a Direction as an argument:

class Pad {
    ...

public:
    Pad();
    void move(Map& map, Direction dir);
    ...
};

If anything, make a Player and Computer class that then each control one Pad object.

Unnecessary use of stringstreams

I don't see the point in Map::print() first printing everything to ss, and then printing the contents of ss to std::cout. Why not print everything directly to std::cout?

Avoid repeating yourself

There is a lot of code duplication in your program that could have been avoided. For example, in Ball::move():

void Ball::move(Map& map, WhoScored& whoScored) {
    // Remove the ball from its current position
    auto& cur_object = map.getObject(m_coordinates);

    if (cur_object == Objects::BALL) {
        cur_object = Objects::NOTHING;
    }

    // Update the position of the ball
    switch (m_direction) {
    case Direction::RIGHT_DOWN: ++m_coordinates.m_y; ++m_coordinates.m_x; break;
    case Direction::RIGHT_UP:   --m_coordinates.m_y; ++m_coordinates.m_x; break;
    case Direction::LEFT_DOWN:  ++m_coordinates.m_y; --m_coordinates.m_x; break;
    case Direction::LEFT_UP:    --m_coordinates.m_y; --m_coordinates.m_x; break;
    }

    // Handle collisions if necessary
    auto& next_object = map.getObject(m_coordinates);

    if (next_object != Objects::NOTHING) {
        handleCollision(map, whoScored);
    }
        else next_object = Objects::BALL;
    }
}

The above also introduces a new member function for Map to get a reference to the object at a given position:

Objects& Map::getObject(Coordinates pos) {
    return m_map[pos.y][pos.x];
}

You could even go further. Consider creating an array of relative positions for each of the directions:

static constexpr Coordinates directions[] = {
    /* NOTHING */   {0, 0};
    /* UP */        {0, -1};
    ...
    /* LEFT_DOWN */ {-1, 1};
};

This way, the switch statement in the above code can be replaced as follows:

// Update the position of the ball
m_coordinates.x += directions[m_direction].x;
m_coordinates.y += directions[m_direction].y;

Avoid magic numbers

There are several magic numbers in your code. Whenever you have some number, like the number of points you need to win, don't just write that number in the code, create a constant for it. This has several benefits: because the number now has a name, it is more self-documenting, and if you ever need to change the number, you only need to do it in one place. So:

static constexpr int winning_score = 10;
static constexpr int ball_move_interval = 13;
static constexpr int computer_move_interval = 11;
...
while (firstPad.getScore() < winning_score && secondPad.getScore() < 10) {
    ...
    if (ballCounter == ball_move_interval) {
        ball.move(map, whoScores);
        ballCounter = 0;
    }
    ...
}

For the vertical lines, you use static_cast<char>(0XB3). I recommend you stick with ASCII characters, and just write '|' (the pipe symbol), but note that if you really want to use a high-ASCII symbol here, you could have written either '│' (literally the character with code 0xB3 from codepage 437) or '\xb3'. The latter still looks like a magical number, so in that case I would still create a named constant for it:

static constexpr char vertical_bar = '\xb3';
\$\endgroup\$
4
  • \$\begingroup\$ 1. Are you sure curses are avaliable for windows? I thoguht they were only for unix based systems. 2.Actually my plan was to change it to std::array but I forgot. Thanks for reminding 3.What do you mean walla are always edges? I suppose you meant edges of console window. Walls are determined by width and height of map I want and they are smaller than console window. Maybe I could resize console window to fit width and height? It would certantly be much better that way but I am not sure how would I handle colisions. I will try refactor code that way and see what I can do. \$\endgroup\$
    – Ivica
    Oct 9, 2022 at 22:28
  • \$\begingroup\$ 4. Main reason for making separatate classes was because of different positions on map. This was best solution that came to my mind. Your way seems much cleaner. 5. Beleive me it was nessesery. Initially I used only couts but program was slow as hell. Then some guy on discord had advised me to use sstream and it made program 100 times faster. It was unplayable before. 6. I do my best to follow DRY but I dont have enough experience to write code clean like your solution. I guess it comes with time. 7. Yeah I know that, I was just happy that it works so I didnt bother putting in const. \$\endgroup\$
    – Ivica
    Oct 9, 2022 at 22:38
  • \$\begingroup\$ Thanks for detailed review btw \$\endgroup\$
    – Ivica
    Oct 9, 2022 at 22:39
  • \$\begingroup\$ 1) Yes, there are curses implementations that also work on Windows. I've added a link to my answer. 3) The edge of the map is walls. 5) I did not know that, maybe someone can shed some light on why that would be so inefficient. 6) Indeed this becomes more natural with experience. Writing clean code is hard, and it's not easy to spot the problems in your own code, which is why code reviews done by others are so helpful :) \$\endgroup\$
    – G. Sliepen
    Oct 9, 2022 at 23:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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