SFML game with a sprite in a dungeon

Question

I am making a game with c++ and sfml for the first time and I would like someone to review my current code before I advance and add more rooms/levels, any input is gratefully appreciated. I was also wondering if I should make a .cpp file for each of my class header files, or is it ok to just put my methods with the header files.

main.cpp

#include <iostream>
#include <SFML/Graphics.hpp>
#include "Character.h"
#include "Sprite.h"
#include "Computer.h"
#include "Window.h"
//Use std
using namespace std;
//Boolean to determine if screen will scroll
int windowWidth = 5000;//width of window
int windowHeight = 5000;//height of window
sf::View dunge(sf::FloatRect(x, y, 5000, 5000));
sf::RenderWindow window(sf::VideoMode(windowWidth, windowHeight ), "Awesome Game" );
//player that is part of Character class and computer that is part of Sprite class
Character player("/Users/danielrailic/Desktop/Xcode /NewGame/ExternalLibs/Sprites/Player.png");
Sprite computer("/Users/danielrailic/Desktop/Xcode /NewGame/ExternalLibs/Sprites/CompSprite.png", 1200, 100);
Sprite battery("/Users/danielrailic/Desktop/Xcode /NewGame/ExternalLibs/Sprites/battery4.png", 0, 0);
Sprite wooddoor("/Users/danielrailic/Desktop/Xcode /NewGame/ExternalLibs/Sprites/wood_door.png", 1200, 1350);
//boolean for whether to show weapon or not
bool showweapon;
//main loop
int main() {
 bool showBox = false;
 //Setting up the dungeon back-round
 sf::Texture dungeon;
 dungeon.loadFromFile("/Users/danielrailic/Desktop/Xcode /NewGame/ExternalLibs/Sprites/DungeonBack.png");
 sf::Sprite backround;
 backround.setTexture(dungeon);
 while (window.isOpen()){
 // check all the window's events that were triggered since the last iteration of the loop
 sf::Event event;
 while (window.pollEvent(event)){
 // "close requested" event: we close the window
 if (event.type == sf::Event::Closed)
 window.close();
 }
 //Movement
 if (moveChar == true){
 if (sf::Keyboard::isKeyPressed(sf::Keyboard::Left)){
 player.left();
 }
 if (sf::Keyboard::isKeyPressed(sf::Keyboard::Right)){
 player.right();
 }
 if (sf::Keyboard::isKeyPressed(sf::Keyboard::Up)){
 player.forward();
 }
 if (sf:: Keyboard::isKeyPressed(sf::Keyboard::Down)){
 player.backward();
 }
 if (sf::Keyboard::isKeyPressed(sf::Keyboard::LShift))
 {
 player.Highspeed();
 }
 else{
 player.Lowspeed();
 }
 }
 if (batstat == 4){
 battery.changeimage("/Users/danielrailic/Desktop/Xcode /NewGame/ExternalLibs/Sprites/battery4.png");
 }
 if (batstat == 3){
 battery.changeimage("/Users/danielrailic/Desktop/Xcode /NewGame/ExternalLibs/Sprites/battery3.png");
 }
 if (batstat == 2){
 battery.changeimage("/Users/danielrailic/Desktop/Xcode /NewGame/ExternalLibs/Sprites/battery2.png");
 }
 if (batstat == 1){
 battery.changeimage("/Users/danielrailic/Desktop/Xcode /NewGame/ExternalLibs/Sprites/battery1.png");
 }
 if (batstat == 0){
 battery.changeimage("/Users/danielrailic/Desktop/Xcode /NewGame/ExternalLibs/Sprites/battery0.png");
 }
 if (player.getSprite().getGlobalBounds().intersects(computer.getSprite().getGlobalBounds())){
 show = false;
 player.hascomp = true;
 }
 if (player.getSprite().getGlobalBounds().intersects(wooddoor.getSprite().getGlobalBounds()) and show == false){
 window.setView(dunge);
 scroll = true;
 showBox = true;
 }
 if (sf::Keyboard::isKeyPressed(sf::Keyboard::A)){
 moveChar = true;
 }
 //draw and window stuff
 window.clear(sf::Color(0, 0, 0));
 window.draw(backround);
 if (show == true){
 window.draw(computer.getSprite());
 }
 if (show == false){
 window.draw(battery.getSprite());
 window.draw(wooddoor.getSprite());
 }
 window.draw(player.getSprite());
 window.display();
 window.setFramerateLimit(70);
 }
 }

Sprite.h

using namespace std;
bool show = true;
class Sprite{
public:
 int Spritex;
 int Spritey;
 string name;
 sf::Texture texture;
 Sprite(string image, int x, int y){
 Spritex = x;
 Spritey = y;
 texture.loadFromFile(image);
 }
 sf::Sprite getSprite() {
 sf::Sprite sprite;
 sprite.setTexture(texture);
 sprite.setPosition(Spritex, Spritey);
 return sprite;
 }
 void changeimage(string image);
};
void Sprite:: changeimage(string image){
 texture.loadFromFile(image);
}

Character.h

using namespace std;
#endif /* Character_h */
int wepx = 1200;
int wepy = 660;
int x_pos;
int y_pos;
int x = 0;
int y = 5000;
int left_limit = 110;
int right_limit = 2300;
int up_limit = 100;
int down_limit = 1400;
bool moveChar = true;
bool GoingRight = false;
bool GoingLeft = false;
bool GoingUp = false;
bool GoingDown = false;
bool scroll = false;
class Character{
public:
 string sprite;
 int health;
 int defense;
 int speed;
 int highspeed;
 int experience;
 bool move = true;
 int x_pos = 1200;
 int y_pos = 600;
 bool hascomp = false;
 sf::Texture texture;
 //Constructor - Ran everytime a new instance of the class is created
 Character(string image){
 health = 100;
 defense = 100;
 speed = 6;
 experience = 0;
 texture.loadFromFile(image);
 }
 sf::Sprite getSprite() {
 sf::Sprite sprite;
 sprite.setTexture(texture);
 sprite.setTextureRect(sf::IntRect(0, 0, 100, 100));
 sprite.setPosition(x_pos, y_pos);
 return sprite;
 }
 //Destructor - Ran when the object is destroyed
 ~Character(){
 }
 //Methods
 void forward();
 void backward();
 void left();
 void right();
 void Highspeed();
 void Lowspeed();
};
void Character::forward(){
 if (y_pos > up_limit){
 y_pos = y_pos - speed;
 wepy = wepy - speed;
 GoingUp = true;
 GoingDown = false;
 GoingLeft = false;
 GoingRight = false;
 texture.loadFromFile("/Users/danielrailic/Desktop/Xcode /NewGame/ExternalLibs/Sprites/PlayerUp.png");
 if (scroll == true){
 y = y - speed;
 }
 }
}
void Character::backward(){
 if (y_pos < down_limit){
 y_pos = y_pos + speed;
 wepy = wepy + speed;
 GoingDown = true;
 GoingUp = false;
 GoingLeft = false;
 GoingRight = false;
 texture.loadFromFile("/Users/danielrailic/Desktop/Xcode /NewGame/ExternalLibs/Sprites/Player.png");
 if (scroll == true){
 y = y + speed;
 }
 }
}
void Character::left(){
 if (x_pos > left_limit){
 x_pos = x_pos - speed;
 wepx = wepx - speed;
 GoingLeft = true;
 GoingRight = false;
 GoingUp = false;
 GoingDown = false;
 texture.loadFromFile("/Users/danielrailic/Desktop/Xcode /NewGame/ExternalLibs/Sprites/PlayerLeft.png");
 if (scroll == true){
 x = x - speed;
 }
 }
}
void Character::right(){
 if (x_pos < right_limit){
 x_pos = x_pos + speed;
 wepx = wepx + speed;
 GoingRight = true;
 GoingLeft = false;
 GoingUp = false;
 GoingDown = false;
 texture.loadFromFile("/Users/danielrailic/Desktop/Xcode /NewGame/ExternalLibs/Sprites/PlayerRight.png");
 if (scroll == true){
 x = x + speed;
 }
 }
}
void Character::Highspeed(){
 speed = 10;
}
void Character::Lowspeed(){
 speed = 6;
}

I feel like this a mess and can be greatly improved.

Answer 1

Use Meaningful Names

Rather than:

dungeon.loadFromFile("/Users/danielrailic/Desktop/Xcode /NewGame/ExternalLibs/Sprites/DungeonBack.png");

I'd rather have something like:

char const *background_file = "/Users/danielrailic/Desktop/Xcode /NewGame/ExternalLibs/Sprites/DungeonBack.png";
dungeon.loadFromFile(background_file);

Likewise, rather than:

if (player.getSprite().getGlobalBounds().intersects(computer.getSprite().getGlobalBounds())){
 show = false;
 player.hascomp = true;
}
if (player.getSprite().getGlobalBounds().intersects(wooddoor.getSprite().getGlobalBounds()) and show == false){
 window.setView(dunge);
 scroll = true;
 showBox = true;
}

I'd prefer code something like:

auto const &player_bounds = player.getSprite().getGlobalBounds();
auto const &computer_bounds = computer.getSprite().getGlobalBounds();
auto const &door_bounds = wooddoor.getSprite().getGlobalBounds();
if (player_bounds.intersects(computer_bounds)) {
 show = false;
 player.hascomp = true;
}
if (player_bounds.intersects(door_bounds) && !show) {
 window.setView(dunge);
 scroll = true;
 showBox = true;
}

Also note that for a Boolean variable, it's preferable to just say if (foo) rather than if (foo == true), or if (! foo) or if (not foo) rather than if (foo == false).

Prefer Data to Code

For example, rather than:

 if (batstat == 4){
 battery.changeimage("/Users/danielrailic/Desktop/Xcode /NewGame/ExternalLibs/Sprites/battery4.png");
 }
 if (batstat == 3){
 battery.changeimage("/Users/danielrailic/Desktop/Xcode /NewGame/ExternalLibs/Sprites/battery3.png");
 }
 if (batstat == 2){
 battery.changeimage("/Users/danielrailic/Desktop/Xcode /NewGame/ExternalLibs/Sprites/battery2.png");
 }
 if (batstat == 1){
 battery.changeimage("/Users/danielrailic/Desktop/Xcode /NewGame/ExternalLibs/Sprites/battery1.png");
 }
 if (batstat == 0){
 battery.changeimage("/Users/danielrailic/Desktop/Xcode /NewGame/ExternalLibs/Sprites/battery0.png");
 }

I'd rather use something like:

char const *bat_images[] = {
 "/Users/danielrailic/Desktop/Xcode /NewGame/ExternalLibs/Sprites/battery0.png",
 "/Users/danielrailic/Desktop/Xcode /NewGame/ExternalLibs/Sprites/battery1.png",
 "/Users/danielrailic/Desktop/Xcode /NewGame/ExternalLibs/Sprites/battery2.png",
 "/Users/danielrailic/Desktop/Xcode /NewGame/ExternalLibs/Sprites/battery3.png",
 "/Users/danielrailic/Desktop/Xcode /NewGame/ExternalLibs/Sprites/battery4.png"
};
if (batstat < elements(bat_images))
 battery.changeimage(bat_images[batstat]);

Avoid Hard-coded Paths, Magic numbers, etc.

The file names in the previous section are an obvious example of code that "works on my machine", but creates considerable difficulty as soon as you want to use it anywhere else.

Don't Repeat Yourself

First we have code to see what code was pressed, and from it decide what function to call:

if (sf::Keyboard::isKeyPressed(sf::Keyboard::Left)){
 player.left();
 }
if (sf::Keyboard::isKeyPressed(sf::Keyboard::Right)){
 player.right();
 }
if (sf::Keyboard::isKeyPressed(sf::Keyboard::Up)){
 player.forward();
 }
if (sf:: Keyboard::isKeyPressed(sf::Keyboard::Down)){
 player.backward();
 }

Then we have the four functions, which are only slightly different from each other:

void Character::forward(){
 if (y_pos > up_limit){
 y_pos = y_pos - speed;
 wepy = wepy - speed;
 GoingUp = true;
 GoingDown = false;
 GoingLeft = false;
 GoingRight = false;
 texture.loadFromFile("/Users/danielrailic/Desktop/Xcode /NewGame/ExternalLibs/Sprites/PlayerUp.png");
 if (scroll == true){
 y = y - speed;
 }
 }
}
// similar code for `right`, `left` and `back` elided

I'd rather have one function that we call with a parameter to indicate the direction, and (again) base more of it on data than code:

static const auto directions[] = {sf::Keyboard::Left, sf::Keyboard::Right, sf::Keyboard::Down, sf::Keyboard::Up};
// In main:
for (auto d : directions)
 if (sf::Keyboard::isKeyPressed(d))
 player.move(d);

...then we replace what are now discrete variables with some arrays, something like this:

// These replace existing variables:
bool going[4] = {false}; // GoingLeft, GoingRight, GoingDown and GoingRight respectively
int limits[4]; // Left, Right, Lower, Upper limits
int positions[2]; // X and Y position respectively
int wep[2] // wepx and wepy respectively
int xy[2]; // x and y respectively

...and then our code for a move looks something like this:

void Character::move(T direction) {
 static char const *texture_names[] = {
 "/Users/danielrailic/Desktop/Xcode /NewGame/ExternalLibs/Sprites/PlayerLeft.png",
 "/Users/danielrailic/Desktop/Xcode /NewGame/ExternalLibs/Sprites/PlayerRight.png"
 "/Users/danielrailic/Desktop/Xcode /NewGame/ExternalLibs/Sprites/Player.png",
 "/Users/danielrailic/Desktop/Xcode /NewGame/ExternalLibs/Sprites/PlayerUp.png",
 };
 // For the moment, I'm going to assume `Left`, `Right`, `Down` and `Up`
 // are contiguous integers in that order, so this gives 0..3:
 auto index = direction - sf::Keyboard::Left;
 // Is this a positive movement or a negative movement?
 int sign = direction == Left || direction == Down ? -1 : 1;
 // Is the movement in X or Y?
 int dir = direction == left || direction == Right ? 0 : 1;
 // The amount we're going to move by:
 auto delta = speed * sign;
 positions[dir] += delta;
 wep[dir] += delta;
 std::fill(going.begin(), going.end(), false);
 going[direction] = true;
 texture.locaFromFile(texture_names[direction]);
 if (scroll)
 xy[dir] += delta;
}

There's undoubtedly more possible improvement (there always is), but I'll leave it at that for now.

• DapperDaniel: Wow thanks a lot! This is exactly what I was looking for and thanks for taking your time to help me. Now I can clean this all up and feel good about moving forward, thanks!
• Lukas: Why use const char* if you could just go with std::string?
• Jerry Coffin: @Lukas: We're not manipulating them at all, so we don't really gain anything from using std::string. When we pass them to loadFromFile we'd have to use c_str() to get a C-string anyway. Bottom line, in this specific case, std::string causes us extra work instead of saving any, so there's no real point.
• Lukas: @JerryCoffin That's not true. loadFromFile takes a std::string so there's no need to c_str(). Plus, if you ever wanted to do something useful with these strings or have them loaded dynamically, having things set to const char* will require you to change it everywhere it isn't implicitly casted. And if you really wanted that type, you could also go with const auto.

Answer 2

1. Don't use using namespace std;

While that would work in your particular case, it's considered bad practice. Especially when you move out your code to separate header files.

See more details here please:

Why is "using namespace std;" considered bad practice?

2. Use separate translation units

I was also wondering if I should make a .cpp file for each of my class header files, or is it ok to just put my methods with the header files.

Yes you should do that, unless you have template class implementations.

When doing so, it's very important to have header guards like

#ifndef CHARACTER_H
#define CHARACTER_H
// Class declaration of Character goes here ...
#endif // CHARACTER_H 

or

#pragma once
// Class declaration of Character goes here ...

to prevent multiple declaration compiler errors.

3. Don't use global variables

From Sprite.h:

bool show = true;

Besides I don't get the purpose of that variable, you shouldn't use global variables to control the behavior of your code. If You're sure that you'll need a static variable for that make it a class member variable.

Also you'll get problems (multiple definition errors), if you include the header in multiple translation units.

• DapperDaniel: Ok thanks a lot, never knew that using namespace std is bad practice, this helps a lot.