Code review

Started by
11 comments, last by alvaro 7 years, 1 month ago

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);
        tex.loadFromFile("1.png");
        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;
}
Advertisement
Use code tags, please.
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.

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.

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

Instead of
    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.
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.
@frob when I #include "block"

@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.
Ok here are the errors please help

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

This topic is closed to new replies.

Advertisement