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="

I see a number of things that may help you improve your code.

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 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.

Rethink your class design

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.

Think about the user

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.

StackExchange Code Review Q#126513, answer score: 7