# Beginning of simple rogue-like RPG (in console) in C++

I've started to learn programming recently and it's my first more or less "big" project. But coding this project for me is quite hard. I spent a lot of time to understand what I've done and my coding is developing really slowly. And I'm curious what's my problem? Is it my lack of experience or is it my ugly style of programming? If it's all about style please tell me about my mistakes.

What do I have now?

"@" - Main character

"#" - Wall

"." - Floor

You can move the character by numpad ("8" - to go up, "6" to go right and etc.) but controls are made by cin>> (for now I hope).

Main.cpp

#include "Map.h" // Map class
#include "MainChar.h" // Main charecter class
#include "Screen.h" // Screen Class
void WaitKey(char& key) //sooner or later I will move all functions in separate file
{
std::cin>>key;
}
int main()
{
//Creating objects
char key=' ';       //key that pressed
bool quit=false;    // flag for quit
Screen scr;         // it's my screen
Map map;            // it's my map
MainChar Mchar;
scr.cordx=39;       //x coordinate of camera
scr.cordy=39;       //y coordinate of camera

//Game Preaparation
scr.FillHelp();
map.Fill();
scr.RenderScr(map,Mchar);

while (!quit) // Main loop
{
// Game Logic
//Contolls
WaitKey(key);
switch (key)
{
case '6' :
if (scr.Scr[Mchar.y][Mchar.x + 1]=='#')
scr.log="There is a wall on my way";
else
scr.cordx++;
break;
case '4' :
if (scr.Scr[Mchar.y][Mchar.x - 1]=='#')
scr.log="There is a wall on my way";
else
scr.cordx--;
break;
case '8' :
if (scr.Scr[Mchar.y - 1][Mchar.x]=='#')
scr.log="There is a wall on my way";
else
scr.cordy--;
break;
case '2' :
if (scr.Scr[Mchar.y + 1][Mchar.x]=='#')
scr.log="There is a wall on my way";
else
scr.cordy++;
break;
}
//Rendering the screen
scr.RenderScr(map,Mchar);

}

return 0;
}


Map.h

#ifndef Map_h
#define Map_h
#endif

class Map
{
public:
void Fill();                   // Creating Map

public:
char Layout[120][120];         // The Walls on the map

};


Map.cpp

#include "Map.h"

void Map::Fill()                                        //Creating Map
{
for (int i=0;i<120;i++)
{
for (int j=0;j<120;j++)
{

if (i==40 || i==80 || j==40 || j==80)       // For now it's just the simpliest map
Layout[i][j]='#';                           // This symbol means Wall
else
Layout[i][j]='.';                           // This symbol means Floor
}
}
}


MainChar.h

#ifndef MainChar_h
#define MainChar_h
#endif

class MainChar
{
public:
MainChar();             //constructor of the class
char Symb;              //Symbol of a character
int x,y;                //Coordinates of a character

};


MainChar.cpp

#include "MainChar.h"
#include <stdlib.h>

MainChar::MainChar()                //constructor
: Symb {'@'} , x{10}, y{10}         // x=10; y=10 - center of the screen
{

}


Screen.h

#include <iostream>                     // call ios to use render
#include <stdlib.h>
#include <string.h>

#ifndef Map_h
#include "Map.h"
#endif

#ifndef MainChar_h
#include "MainChar.h"
#endif
class Screen
{
public:
void RenderScr(Map lay,MainChar mchar);         // "capturing" the screen
void FillHelp();                                // filling help

public:
std::string help[21];                           // It's description of controls
std::string log;                                // It's the log of events
char Scr[21][21];                               // it's the screen we are printing on or "camera view" if you can call it
int cordx,cordy;                                // coordinates of the screen
};


Screen.cpp

#include "Screen.h"// Calling our header file

void Screen::RenderScr(Map lay,MainChar mchar)                  // "capturing" the screen and printing it
{
system("cls");
for (int i=0;i<21;i++)                          // first loop for "capturing" the screen
{
for (int j=0;j<21;j++)                      // second loop for "capturing" the screen
{
Scr[i][j]=lay.Layout[i+cordy][j+cordx]; // "rendering" the walls
if (i==mchar.y && j==mchar.x)
Scr[i][j]=mchar.Symb;               // "rendering" the Main Character
//bla bla bla other objects
std::cout<<Scr[i][j];                   //printing the screen
}
std::cout<<'\t'<<help[i]<<'\n';             // printing help
}
std::cout<<log<<'\n';                           //printing the log
log="";

void Screen::FillHelp()                         //filling help
{
help[0]="\"Numpad\"... - Movement";         //help message № 1
help[1]="";                                 //help message № 2
for (int i=2; i<21; i++)                    //blanket messages
help[i]="";

}

• Is the indentation here the same as the indentation in your actual code or did something go wrong with copy-pasting it? – Mast Apr 23 '16 at 14:24
• It's the same (as far as I can see) – Hagartinger Apr 23 '16 at 14:35

## Fix the typo

It's probably a cut-and-paste error rather than a real error, but in the posted version of the code, the Screen::RenderScr() function is missing the closing brace.

## Use consistent formatting

The code as posted has inconsistent indentation which makes it hard to read and understand. Pick a style and apply it consistently.

## Don't use system("cls")

There are two reasons not to use system("cls") or system("pause"). The first is that it is not portable to other operating systems which you may or may not care about now. The second is that it's a security hole, which you absolutely must care about. Specifically, if some program is defined and named cls or pause, your program will execute that program instead of what you intend, and that other program could be anything. First, isolate these into a seperate functions cls() and pause() and then modify your code to call those functions instead of system. Then rewrite the contents of those functions to do what you want using C++. For example, if your terminal supports ANSI Escape sequences, you could use this:

void cls()
{
std::cout << "\x1b[2J";
}


## Eliminate "magic numbers"

This code is littered with "magic numbers," that is, unnamed constants such as 10, 21, etc. Generally it's better to avoid that and give such constants meaningful names. That way, if anything ever needs to be changed, you won't have to go hunting through the code for all instances of "21" and then trying to determine if this particular 21 means the width of the screen or some other constant that happens to have the same value.

## Use include guards

There should be an include guard in each .h file. That is, start the file with:

#ifndef SCREEN_H
#define SCREEN_H
// file contents go here
#endif // SCREEN_H


That way you don't have to (and shouldn't!) have constructs like these:

#ifndef Map_h
#include "Map.h"
#endif


## Use only necessary #includes

The #include <stdlib.h> line in MainChar.cpp is not necessary and should be removed. Generally speaking, you should seek only have necessary #includes in your code.

## Make data members private

All of the data members of the classes are currently public, which is not a good design. If you need accessors, you could use simple const ones accessor functions that allow for safe access to a classes internal data. Better yet is to make it so that other classes never need to know or care about the internal functioning of the class.

The Map class should use a constructor to fill in the data rather than a help function, since a Map without data is pretty useless. Similarly, think about minimizing data sharing (as mentioned above) and what a minimal but sufficient interface might be.

There is no obvious way to gracefully exit the program. I'd suggest addding to the switch in main() to control when the program is done.

## Use whitespace to improve readability

Lines like this:

for (int i=0;i<21;i++)


become much easier to read with a little bit of whitespace:

for (int i = 0; i < 21; i++)


## Pass by const reference where practical

The first argument to RenderScr is a Map but that causes the entire map to be duplicated. Better would be to make it const Map & because it is not modified and it doesn't need to be duplicated.

## Omit return 0

When a C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no reason to put return 0; explicitly at the end of main.

• Comments are not for extended discussion; this conversation has been moved to chat. – Simon Forsberg Apr 24 '16 at 0:09

## Don't abbreviate names unncessarily

You are abbreviating a lot of names, but there mostly is no reason for that. Instead of scr, just write screen. The more you abbreviate, the less it is clear what you actually mean. Some abbreviations also might cause misinterpretations: for example with cordx, is that a coordinate, or does it refer to an actual cord?

On the other hand, sometimes you repeat things unnecessarily in names. For example, the function RenderScr() is a member of class Screen. So the Scr part in the function name is redundant, just call it Render(). Also, in the case of cordx and cordy, the names x and y are generally understood to be coordinates, so you can cut the cord off.

I'll explain a little bit about Edward's suggestions (for anyone that stumbles upon this question like I did); in particular, I'll focus on private vs. public vs. protected variables, and why a 'reference' is often better than a 'pointer'.

Private vs. Public vs. Protected

When a function is declared in the header, there are three possible 'levels' of access (called 'access specifiers'):

'Private' means that the function or variable is only accessible by that specific class, and no other. Outside classes are never allowed to call anything under the 'private' category, so there's no risk of another class calling a function that it shouldn't.

'Public': any class can access a 'public' function or variable. For critical functions, this is far from ideal: anything that tries to call a 'public' function or variable will be able to do so, even if that causes undesirable operation.

The 'Protected' category is the middle-ground between 'public' and 'private': protected functions and variables can only be accessed by their own class and anything that derives from that class. Use this specifier to avoid creating redundant variables (you don't need to create an individual 'health' variable for each and every subclass of 'creature', for example).

Pointers vs. References

It's sometimes hard to know when you should use a pointer instead of a reference, or vice-versa. Here's the bare-bones explanation.

'Pointers' are generally non-destructive; they create a copy of the data at a specific memory address, and perform operations on that copy. This is useful when multiple functions make use of a particular variable, because each individual function will get its own copy of that variable to work with.

References are different: they're connected to one object, and one object only, and they can't be 'reseated' to refer to a different object. Unlike a pointer, they directly access stored data; any changes to the data will necessarily overwrite what's already been stored. Use a reference to modify data that is unique to a particular object, such as a player-character's name.

Anyhow, this is getting very long, so I'll leave it here. I hope this helps those of you that arrive and wonder why it's important to do things in a particular way. :)