5
\$\begingroup\$

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


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

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

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.

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

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.

\$\endgroup\$

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.