• FEATURED

View more

View more

View more

### Image of the Day Submit

IOTD | Top Screenshots

### The latest, straight to your Inbox.

Subscribe to GameDev.net Direct to receive the latest updates and exclusive content.

# Code review

12 replies to this topic

### #1Code_Black  Members

Posted 20 March 2017 - 06:04 PM

Yes I was hoping I could get some feed back on my code. I know everythings in main, but when I try to add custom headers I get all kinds of errors. Thanks.

#include <iostream>
#include <SFML/Graphics.hpp>
#include<cmath>
//#include"block.h"
//sf::RenderWindow win(sf::VideoMode(800,600),"RPG");
using namespace std;
enum class State{PAUSE,PLAYING};

class Block{
sf::Texture tex;
sf::Sprite spr;
sf::Vector2f pos;

public:
int pos_i;
Block(float x=0.f,float y=0.f,float hi=0.f,float wd=0.f){
sf::Vector2f size(hi,wd);
pos.x=x; pos.y=y;
spr.setTexture(tex);
spr.setTextureRect(sf::IntRect(0,0,size.x,size.y));
spr.setPosition(sf::Vector2f(x,y));
}

void draw(sf::RenderWindow* win){win->draw(spr);}
void set_size(sf::Vector2f size){spr.setTextureRect(sf::IntRect(0,0,size.x,size.y));}
sf::Vector2f get_pos(){return pos;}
sf::Sprite& get_tex(){return spr;}
void set_pos(sf::Vector2f pos){spr.move(pos);}
};

class World{
int wd;
int hi;
int level[6][6];

public:
std::vector<Block> block1;
int count;
//void set_pos(sf::Vector2f pos,int i,int j){block[i*j].set_pos(pos);}

World():level{
{1,0,0,0,0,0},
{1,0,0,0,0,0},
{1,0,0,0,0,0},
{1,0,0,0,0,0},
{1,0,0,0,0,0},
{1,1,1,1,1,1}}
{
wd=6;
hi=6;
//blocks=new Block[wd*hi];

for(int i=0;i<hi;i++){
for(int j=0;j<wd;j++){
if(level[i][j]==1){
Block blk(j*24,i*24,24,24);
block1.push_back(blk);
sf::Vector2f pos=blk.get_pos();
cout<<" x "<<j<<" y "<<i<<'\n';
cout<<" pos x "<<pos.x<<" pos y "<<pos.y<<'\n';
}
}
}
//block[1].set_pos(sf::Vector2f(1.f*16,4.f*16));
}

void draw(sf::RenderWindow* win){
for(int i=0;i<block1.size();i++){
block1[i].draw(win);
}
}
};

class weapon{
Block blc;
bool m_up;
bool m_dn;
bool m_lt;
bool m_rt;
sf::Vector2f m_res;

public:
weapon():blc(0,0,8,8){}
void move_rt(){m_rt=true;}
void move_lt(){m_lt=true;}
void move_up(){m_up=true;}
void move_dn(){m_dn=true;}

void stop_rt(){m_rt=false;}
void stop_lt(){m_lt=false;}
void stop_up(){m_up=false;}
void stop_dn(){m_dn=false;}
void setPos(sf::Vector2f pos){blc.set_pos(pos);}
sf::Sprite getWep(){return blc.get_tex();}
void update( sf::Vector2f pos ,sf::Vector2i mse_pos){
if(m_up)
blc.set_pos(pos - sf::Vector2f(0.f,10.f));
if(m_dn)
blc.set_pos(pos + sf::Vector2f(0.f,10.f));
if(m_rt)
blc.set_pos(pos - sf::Vector2f(10.f,0.f));
if(m_lt)
blc.set_pos(pos + sf::Vector2f(10.f,0.f));

float angle=(atan2(mse_pos.y-m_res.y/2,mse_pos.x-m_res.x/2))*180/3.14f;

blc.get_tex().setRotation(angle);
}
};

class Player{
Block blc;
weapon Weapon;
sf::Vector2f m_res;

bool m_up;
bool m_dn;
bool m_lt;
bool m_rt;

float m_speed;
sf::Time last_hit;

public:
Player():blc(0,0,16,16){}

void spawn(sf::Vector2f res,sf::Vector2f loc){
blc.set_pos(loc);
Weapon.setPos(sf::Vector2f(loc.x+5.0f,loc.y+5.0));
m_res.x=res.x;
m_res.y=res.y;
}
weapon& getWep(){return Weapon;}
bool hit(sf::Time time_hit);
sf::FloatRect get_pos(){return blc.get_tex().getGlobalBounds();}
sf::Vector2f get_cen(){return blc.get_pos();}
float get_rot(){return blc.get_tex().getRotation();}
sf::Sprite get_pla(){return blc.get_tex();}

void move_rt(){m_rt=true;}
void move_lt(){m_lt=true;}
void move_up(){m_up=true;}
void move_dn(){m_dn=true;}

void stop_rt(){m_rt=false;}
void stop_lt(){m_lt=false;}
void stop_up(){m_up=false;}
void stop_dn(){m_dn=false;}

void update( sf::Vector2f pos ,sf::Vector2i mse_pos){
if(m_up)
blc.set_pos(pos - sf::Vector2f(0.f,10.f));
if(m_dn)
blc.set_pos(pos + sf::Vector2f(0.f,10.f));
if(m_rt)
blc.set_pos(pos - sf::Vector2f(10.f,0.f));
if(m_lt)
blc.set_pos(pos + sf::Vector2f(10.f,0.f));

float angle=(atan2(mse_pos.y-m_res.y/2,mse_pos.x-m_res.x/2))*180/3.14f;

blc.get_tex().setRotation(angle);
}

};

int main()
{
//State s=State::PAUSE;
sf::Vector2f res;
res.x=sf::VideoMode::getDesktopMode().width;
res.y=sf::VideoMode::getDesktopMode().height;
sf::RenderWindow win(sf::VideoMode(res.x,res.y),"RPG");
sf::RenderWindow win1(sf::VideoMode(res.x,res.y),"RPG");
sf::View mainView;
mainView.reset(sf::FloatRect(0,0,res.x,res.y));
mainView.setViewport(sf::FloatRect(0,0,1.0f,1.0f));
sf::Clock clock;
sf::Time gameTotal;
sf::Vector2f mouseWPos;
sf::Vector2i mouseSPos;

World w;
Player pla;
weapon wep;
pla.spawn(res,sf::Vector2f(2.f,2.f));
while(win.isOpen()){
sf::Event eve;
while(win.pollEvent(eve)){
if(true){
if(sf::Keyboard::isKeyPressed(sf::Keyboard::W)){
pla.move_up();
pla.getWep().move_up();
}
else{
pla.stop_up();
pla.getWep().stop_up();
}
if(sf::Keyboard::isKeyPressed(sf::Keyboard::S)){
pla.move_dn();
pla.getWep().move_dn();
}
else{
pla.stop_dn();
pla.getWep().stop_dn();
}
if(sf::Keyboard::isKeyPressed(sf::Keyboard::A)){
pla.move_lt();
pla.getWep().move_lt();
}
else{
pla.stop_lt();
pla.getWep().stop_lt();
}
if(sf::Keyboard::isKeyPressed(sf::Keyboard:)){
pla.move_rt();
pla.getWep().move_rt();
}
else{
pla.stop_rt();
pla.getWep().stop_rt();
}
}
sf::Vector2f plaPos(pla.get_cen());
pla.update(plaPos,sf::Mouse::getPosition());
pla.getWep().update(plaPos,sf::Mouse::getPosition());
}

clock.restart();

win.clear(sf::Color::Black);
win.draw(pla.getWep().getWep());
win.draw(pla.get_pla());
w.draw(&win);
win.setView(mainView);

win.display();
}
//cout << "Hello world!" << endl;
return 0;
}


#### Attached Files

Edited by frob, 20 March 2017 - 11:20 PM.

### #2Álvaro  Members

Posted 20 March 2017 - 07:47 PM

### #3Nypyren  Members

Posted 20 March 2017 - 10:35 PM

Can't really read it in unformatted form, but:

- You need longer and more descriptive variable names.
- You shouldn't abbreviate so much stuff.
- You have inconsistent capitalization conventions (class weapon).
- All of your up/down/left/right stuff could probably be enum and array-based instead.
- move_* and stop_* should probably be changed to set(direction, bool).
- All of your numeric constants should be turned into 'const' definitions somewhere.

Edited by Nypyren, 20 March 2017 - 10:41 PM.

### #4frob  Moderators

Posted 20 March 2017 - 11:22 PM

but when I try to add custom headers I get all kinds of errors.

Please include the exact error message.

The error is probably something inside your header file.  That's plain code as well, and any parse errors in there will show up wherever the file is included.

Check out my book, Game Development with Unity, aimed at beginners who want to build fun games fast.

Also check out my personal website at bryanwagstaff.com, where I occasionally write about assorted stuff.

### #5Álvaro  Members

Posted 21 March 2017 - 05:18 AM

Nypyren mentioned it already, but let me emphasize it: The abbreviations make the code unreadable. It took me a while to figure that res' stands for "resolution".

Using 3.14f as the value of pi hurts my soul. It's unfortunate that SFML wants you to specify angles, but it's not your fault.

Do you understand how textures and sprites are supposed to be used in SFML? Each Block object should not have its own sf::Texture; it should use an sf::Texture that lives somewhere else.

    sf::Sprite& get_tex(){return spr;}

That's just weird. Why would "get_tex" return a sprite and not a texture? This is further evidence that perhaps you didn't understand how textures and sprites are supposed to be used. Textures contain an image from which sprites are cut. They are fairly heavy objects and you shouldn't create many of them. Sprites on the other hand are just a handful of numbers that describe what part of the texture should be drawn. These are light objects and you can feel free to create many of them, copy them around, etc.

get_pla returns a sprite; get_pos returns a vector if called on a block (this is fine), but it returns a rectangle if called on a weapon (???); getWep returns a sprite if called on a weapon, but it returns a weapon if called on a player? You are just messing with us.

    m_res.x=res.x;
m_res.y=res.y;
just use
    m_res = res;

What is the clock for?

  //cout << "Hello world!" << endl;


I understand you are sentimental about the origins of your program, but you should just let go of this one.

In general, comments should be used to explain some design decision, or to clarify some difficult part of the code (this is in a way an acceptance of defeat), or to document the interface to a part of your code. The only comments you have are old lines of code that don't help the reader at all, and should go.

### #6Code_Black  Members

Posted 21 March 2017 - 06:10 AM

Thank you. Like I said this is my first real try at something showing what I've learned so far. I know it's messy but I'm trying to get things to work. So any advice would be greatful.

### #7Code_Black  Members

Posted 21 March 2017 - 06:20 AM

@frob when I #include "block"

### #8Álvaro  Members

Posted 21 March 2017 - 06:42 AM

@frob when I #include "block"

That does not address what frob said. He said to "include the exact error message". Error messages might seem like mystical nonsense to you, but they are important.

### #9Code_Black  Members

Posted 22 March 2017 - 01:29 PM

||=== Build: Debug in cat (compiler: GNU GCC Compiler) ===|
obj\Debug\main.o||In function ZN5WorldC1Ev':|
C:\Users\chris\Documents\cat\main.cpp|69|undefined reference to Block::Block(float, float, float, float)'|
C:\Users\chris\Documents\cat\main.cpp|71|undefined reference to Block::get_pos()'|
obj\Debug\main.o||In function ZN5World4drawEPN2sf12RenderWindowE':|
C:\Users\chris\Documents\cat\main.cpp|87|undefined reference to Block::draw(sf::RenderWindow*)'|
obj\Debug\main.o||In function ZN6weaponC1Ev':|
C:\Users\chris\Documents\cat\main.cpp|102|undefined reference to Block::Block(float, float, float, float)'|
obj\Debug\main.o||In function ZN6weapon6setPosEN2sf7Vector2IfEE':|
C:\Users\chris\Documents\cat\main.cpp|112|undefined reference to Block::set_pos(sf::Vector2<float>)'|
obj\Debug\main.o||In function ZN6weapon6getWepEv':|
C:\Users\chris\Documents\cat\main.cpp|113|undefined reference to Block::get_tex()'|
obj\Debug\main.o||In function ZN6weapon6updateEN2sf7Vector2IfEENS1_IiEE':|
C:\Users\chris\Documents\cat\main.cpp|117|undefined reference to Block::set_pos(sf::Vector2<float>)'|
C:\Users\chris\Documents\cat\main.cpp|119|undefined reference to Block::set_pos(sf::Vector2<float>)'|
C:\Users\chris\Documents\cat\main.cpp|121|undefined reference to Block::set_pos(sf::Vector2<float>)'|
C:\Users\chris\Documents\cat\main.cpp|123|undefined reference to Block::set_pos(sf::Vector2<float>)'|
C:\Users\chris\Documents\cat\main.cpp|129|undefined reference to Block::get_tex()'|
obj\Debug\main.o||In function ZN6PlayerC1Ev':|
C:\Users\chris\Documents\cat\main.cpp|151|undefined reference to Block::Block(float, float, float, float)'|
obj\Debug\main.o||In function ZN6Player5spawnEN2sf7Vector2IfEES2_':|
C:\Users\chris\Documents\cat\main.cpp|156|undefined reference to Block::set_pos(sf::Vector2<float>)'|
obj\Debug\main.o||In function ZN6Player7get_cenEv':|
C:\Users\chris\Documents\cat\main.cpp|167|undefined reference to Block::get_pos()'|
obj\Debug\main.o||In function ZN6Player7get_plaEv':|
C:\Users\chris\Documents\cat\main.cpp|169|undefined reference to Block::get_tex()'|
obj\Debug\main.o||In function ZN6Player6updateEN2sf7Vector2IfEENS1_IiEE':|
C:\Users\chris\Documents\cat\main.cpp|185|undefined reference to Block::set_pos(sf::Vector2<float>)'|
C:\Users\chris\Documents\cat\main.cpp|187|undefined reference to Block::set_pos(sf::Vector2<float>)'|
C:\Users\chris\Documents\cat\main.cpp|189|undefined reference to Block::set_pos(sf::Vector2<float>)'|
C:\Users\chris\Documents\cat\main.cpp|192|undefined reference to Block::set_pos(sf::Vector2<float>)'|
C:\Users\chris\Documents\cat\main.cpp|198|undefined reference to Block::get_tex()'|
||=== Build failed: 20 error(s), 0 warning(s) (0 minute(s), 1 second(s)) ===|

### #10Álvaro  Members

Posted 22 March 2017 - 01:46 PM

Those look like linker errors. You have to make sure to list all the .cpp files in the compilation command.

If you still have trouble, please post the exact compilation command you are using.

Edited by Álvaro, 22 March 2017 - 01:47 PM.

### #11Bregma  Members

Posted 22 March 2017 - 01:49 PM

obj\Debug\main.o||In function ZN5WorldC1Ev':| C:\Users\chris\Documents\cat\main.cpp|69|undefined reference to Block::Block(float, float, float, float)'|

Thos look to me like you're not linking in a .OBJ file built from your Block code. I'm not sure what environment you;re in (GCC on Windows, so it's not Visual Studio) but I suspect somewhere you need to add "block.cpp" to a project or something.

Stephen M. Webb
Professional Free Software Developer

### #12Code_Black  Members

Posted 22 March 2017 - 02:23 PM

All I did was install cb and started new project and tried to add my own class and errors but when I all the code in main problem sloved. I know it wrong to code in one file so help. Thanks

obj\Debug\main.o||In function ZN5WorldC1Ev':| C:\Users\chris\Documents\cat\main.cpp|69|undefined reference to Block::Block(float, float, float, float)'|

Thos look to me like you're not linking in a .OBJ file built from your Block code. I'm not sure what environment you;re in (GCC on Windows, so it's not Visual Studio) but I suspect somewhere you need to add "block.cpp" to a project or something.

obj\Debug\main.o||In function ZN5WorldC1Ev':| C:\Users\chris\Documents\cat\main.cpp|69|undefined reference to Block::Block(float, float, float, float)'|

Thos look to me like you're not linking in a .OBJ file built from your Block code. I'm not sure what environment you;re in (GCC on Windows, so it's not Visual Studio) but I suspect somewhere you need to add "block.cpp" to a project or something.
Yes I'm using codeblocks on windows 10 32bit,2gb,1.83g atom

### #13Álvaro  Members

Posted 22 March 2017 - 02:26 PM

Learn one thing at a time. If you want to learn how to have multi-file projects, create a new project. Make two .cpp files and one header file.

main.cpp:
#include "print_it.h"

int main() {
print_it();
}

print_it.h:
#pragma once

void print_it();

print_it.cpp:
#include "print_it.h"

void print_it() {
std::cout << "Hello, world!\n";
}


Figure out how to make that work in your environment. You'll have to add both .cpp files to your project, or something like that. Use Google to see how this is done in your particular environment.

Try compiling from the command line. Something like this should work:
> g++ -std=c++11 main.cpp print_it.cpp -o main.exe -Wall -Wextra -O3