Problem
How it should work:
Creates three example Films which are classified as Comedy, Horror and Historic. Those have special attributes each, and can change the players stats. These stat changes are caused by random chances. The last step is not implemented yet, because I had several problems with doing so. Maybe reviewing my code will help me improving it, and have a better understanding how I can implement the doAction(); to apply these stat-changes for the player.
Review suggestions:
-
General improvisation of my code, where are problems, have I choosen the right containers, are there variables which I should protect/make private, how can I still use them later on, where are error handlings missing, how can I implement them correctly?
-
Can I improve it with using pointer? For sure, but how, and why, and how can I take hold on them in the main.cpp?
-
Help with writing of the doAction is greatly appreciated, but not necessary
Here is the Code, any help will be greatly appreciated:
main.cpp
#include <iostream>
#include <algorithm>
#include "Struktur.h"
#include "Methods.h"
#include "Player.h"
int main()
{
Player pl;
do{
std::string Chosen;
char Action;
bool afraid;
bool courage;
bool cheaper;
bool boring;
bool knowledge;
auto Film1=Comedy("Laugh");
Film1.Data();
auto Film2=Horror("Scream");
Film2.Data();
auto Film3=Historic("Educate");
Film3.Data();
int DurationArray[]={Film1.FilmD, Film2.FilmD, Film3.FilmD};
int PrizeArray[]={Film1.FilmP, Film2.FilmP, Film3.FilmP};
const int minduration=*std::min_element(DurationArray,DurationArray+3);
const int minprize=*std::min_element(PrizeArray,PrizeArray+3);
Chosen=pl.RequestAct();
Action=pl.getAction(Chosen);
pl.checkAct(Action, minduration, minprize);
//also possible: switch(Action) case
if(Action=='L'){
cheaper=Film1.cheap;
}
if(Action=='S'){
afraid=Film2.afraid;
courage=Film2.courage;
}
if(Action=='E'){
boring=Film3.boring;
knowledge=Film3.knowledge;
}
}while(pl.play!=false);
std::cout<<"Game Ended";
std::cin.ignore(-1, 'n'); //let console stay open even if there's something in the buffer left
std::cin.get();
#if false
int NewRandomNumber;
NewRandomNumber=Methods(0,1);
std::cout<<NewRandomNumber<<std::endl;
#endif // false
return 0;
}
methods.h:
#ifndef METHODS_H_INCLUDED
#define METHODS_H_INCLUDED
int Methods(int low, int high);
#endif // METHODS_H_INCLUDED
methods.cpp:
#include <iostream>
#include <cstdlib>
#include <vector>
#include "Methods.h"
int Methods(int low, int high){
return((int)std::rand()/RAND_MAX)*(high-low)+low;
}
Struktur.h:
#ifndef STRUKTUR_H_INCLUDED
#define STRUKTUR_H_INCLUDED
class Struktur{
public:
Struktur(std::string Title): FilmT(Title){}
Struktur(int Duration): FilmD(Duration){}
virtual ~Struktur(){} //destruct later
virtual void Data()=0;
//private:
std::string FilmT;
int FilmD;
int FilmP;
};
class Comedy:public Struktur{
public:
Comedy(std::string Title): Struktur(Title){}
int maybecheaper();
bool cheap=maybecheaper();
void Data()override{ //Replace the data with those of the specific animal
std::cout<<"Title: "<<FilmT<<std::endl;
FilmD=126;
if(cheap==true){
FilmP=10;
}else{
FilmP=20;
}
}
};
class Horror:public Struktur{
public:
Horror(std::string Title): Struktur(Title){}
void Data()override{
std::cout<<"Title: "<<FilmT<<std::endl;
FilmD=78;
FilmP=18;
}
int maybeafraid();
bool afraid=maybeafraid();
int maybecourage();
bool courage=maybecourage();
};
class Historic:public Struktur{
public:
Historic(std::string Title): Struktur(Title){}
void Data()override{
std::cout<<"Title: "<<FilmT<<std::endl;
FilmD=160;
FilmP=28;
}
int maybeboring();
bool boring=maybeboring();
int maybeknowledge();
bool knowledge=maybeknowledge();
};
#endif // STRUKTUR_H_INCLUDED
struktur.cpp:
#include <iostream>
#include "Struktur.h"
#include "Methods.h"
void Daten(Struktur& da){
da.Data();
auto a=dynamic_cast<Comedy*>(&da);
if(a!=nullptr){ //then: Comedy
a->maybecheaper(); //call special method
}
auto b=dynamic_cast<Horror*>(&da);
if(b!=nullptr){
b->maybeafraid();
b->maybecourage();
}
auto c=dynamic_cast<Historic*>(&da);
if(c!=nullptr){
c->maybeboring();
c->maybeknowledge();
}
}
int Comedy::maybecheaper(){
bool check=Methods(0,1); //50/50
return check;
}
int Horror::maybeafraid(){
bool check;
int random=Methods(0,3); //1/4
if (random==0){
check=true;
}else{
check=false;
}
return check;
}
int Horror::maybecourage(){
bool check;
int random=Methods(0,2);//2/3
if(random!=2){
check=true;
}else{
check=false;
}
return check;
}
int Historic::maybeboring(){
bool check;
int random=Methods(0,5);//5/6
if(random!=0){
check=true;
}else{
check=false;
}
return check;
}
int Historic::maybeknowledge(){
bool check;
int random=Methods(0,1);
if(random==0){
check=true;
}else{
check=false;
}
return check;
}
Player.h:
#ifndef PLAYER_H_INCLUDED
#define PLAYER_H_INCLUDED
class Player
{
public:
Player();
//friend class Struktur;
int RemoveEnjoyment(int amount);
double RemoveMoney(int amount);
int getMoney();
int getTime();
int GetEnjoyment();
int GainEnjoyment(int amount);
int GetCourage();
int GainCourage(int amount);
int GetKnowledge();
int GainKnowledge(int amount);
void Stats();
std::string RequestAct();
char getAction(std::string ChosenFilm);
void checkAct(char Action, int MinDuration, int MinPrize);
void EndGame();
bool play;
int CheckTurns();
friend int maybecheaper();
private:
int Turns;
int Courage;
int Enjoyment;
int Knowledge;
int Time;
double Money;
};
#endif // PLAYER_H_INCLUDED
player.cpp:
#include <iostream>
#include <vector>
#include <string>
#include "math.h"
#include "Player.h"
std::string Player::RequestAct(){ //Ask which film the Player wants to watch
std::string Film;
std::cout<<"Enter the name of the film you want to watch"<<std::endl;
std::cin>>Film;
return Film;
}
char Player::getAction(std::string ChosenFilm){
char Action;
if (ChosenFilm=="Laugh"){
Action='C';
}else if(ChosenFilm=="Scream"){
Action='H';
}else if(ChosenFilm=="Educate"){
Action='E';
}else{
std::cout<<"You haven't entered a valid titlen";
getAction(RequestAct());
}
return Action;
}
//Enjoyment
int Player::GetEnjoyment(){ //gives the actual value of Enjoyment
return Enjoyment;
}
int Player::GainEnjoyment(int amount){ //adds Enjoyment, return new val
int beforeEnjoyment=Enjoyment;
Enjoyment+=amount;
Enjoyment=std::min(Enjoyment, 100); //value can't be above 100(%)
return Enjoyment-beforeEnjoyment;
}
int Player::RemoveEnjoyment(int amount){
int beforeEnjoyment=Enjoyment;
Enjoyment-=amount;
Enjoyment=std::max(Enjoyment, 0); //can't decrease more
return Enjoyment-beforeEnjoyment;
}
//Courage
int Player::GetCourage(){ //gives the actual value of Courage
return Courage;
}
int Player::GainCourage(int amount){
int beforeCourage=Courage;
Courage+=amount;
return Courage-beforeCourage;
}
//Knowledge
int Player::GetKnowledge(){
return Knowledge;
}
int Player::GainKnowledge(int amount){
int beforeKnowledge=Knowledge;
Knowledge+=amount;
return Knowledge-beforeKnowledge;
}
//Money
int Player::getMoney(){
return Money;
}
//Time
int Player::getTime(){
return Time;
}
//Actual Stats
void Player::Stats(){ //Print Stats
std::cout<<"Your Stats: n";
std::cout<<"Courage: "<<Courage<<"n";
std::cout<<"Enjoyment: "<<Enjoyment<<"n";
std::cout<<"Knowledge "<<Knowledge<<"n";
std::cout<<"Time: "<<Time<<"n";
std::cout<<"Money: "<<Money<<"n";
}
Player::Player(){ //Stats to begin with
Turns=0;
Courage=10;
Enjoyment=100;
Knowledge=10;
Time=500;
Money=100;
}
int Player::CheckTurns(){
return Turns;
}
//Action
void Player::checkAct(char Action, int MinDuration, int MinPrize){
//Tracking...
int CourageChange=0;
int EnjoymentChange=0;
int KnowledgeChange=0;
int TimeChange=0;
int MoneyChange=0;
if(Money<=MinPrize){
Player::EndGame();
}
if(Time<=MinDuration){ // least possible time...(Horror)
Player::EndGame();
}
if(Turns>=10){
Player::EndGame();
}
Turns++;
return;
}
void Player::EndGame(){
std::cout<<"EndGame";
Player::play=false;
}
Solution
I see a number of things that may help you improve your code.
Fix the formatting
Some of the files appear to be formatted consistently, but the Player.cpp file, in particular, seems to have almost all of the lines unindented, which makes it difficult to read and understand the code.
Use whitespace to improve readability
Lines like this:
std::cout<<NewRandomNumber<<std::endl;
are quite difficult to read with no whitespace. I’d prefer to see it like this:
std::cout << NewRandomNumber << std::endl;
Use better names
There is a function named Methods() which is actually a random number generator, and a member function named Data() which looks like it should actually be part of a constructor. There is also a class named Struktur which should probably actually be called Film or Movie. Those are not very good names because they don’t give much hint as to what they do.
Eliminate “magic values”
Generally, constants that are used in multiple places should be named constants. This makes the code both easier to read and understand, and also prevents errors of the type that are now in the code. For instance, the Player::getAction() routine returns C, H or E, but in main() what is checked for is L, S and E.
Rethink your data structures
As mentioned above, Struktur is not an informative name for that class. Let’s first rename it to Film. Next, let’s think about the goal. It seems that your intent is for the act of “watching” a film in the game alters some attributes of the Player. If that’s correct, then we might want to express that using syntax like this:
Player pl;
Film f;
pl.watch(f);
This method could be implemented like this:
void Player::watch(const Film &f) {
Courage += f.deltaCourage();
Enjoyment += f.deltaEnjoyment();
Knowledge += f.deltaKnowledge();
Time += f.deltaTime();
Money += f.deltaMoney();
++Turns;
}
The Film class would then need to implement those methods. Alternatively, one could use a std::valarray or std::array of attributes instead of having them individually named.
Here’s how the interface to a File class might look with the attributes defined as a std::array:
#include <string>
#include <array>
using Attributes = std::array<int, 5>;
class Film{
public:
enum Genre { comedy, horror, historic };
Film(Film::Genre genre, std::string title);
Attributes attributes() const { return attr; }
private:
std::string title;
Attributes attr;
};
Don’t use floating point for money
Rather than using double for representing money, keep it as an int and track the number of cents. This avoids rouding errors when adding and subtracting, as sometimes occurs with floating point.
Have constructors completely construct an object
The constructors for Comedy, Horror and Historic each only partly initialize those objects, requiring the user to subsequenty call the poorly named Data() function to finish the job. Instead, it’s usually better to completely construct an object in the constructor so that it is immediately useful without any further action required. As an example of how to do that, here is a complete Film.cpp file corresponding to the class definition above:
#include "Film.h"
#include <iostream>
#include <random>
static std::random_device rd;
static std::mt19937 gen(rd());
static std::bernoulli_distribution cheap(0.50);
Film::Film(Film::Genre genre, const std::string &title) :
title{title},
attr{}
{
if (genre == comedy) {
std::cout << "Comedyn";
attr = {0, 0, 0, 126, (cheap(gen) ? 1000 : 2000)};
} else if (genre == horror) {
std::cout << "Horrorn";
attr = {0, 0, 0, 78, 1800};
} else if (genre == historic) {
std::cout << "Historicn";
attr = {0, 0, 0, 160, 2800};
}
}
Keep data members private
The FilmD and FilmP data members are currently public and are collected into two separate arrays. Instead, I’d suggest keeping a the data members private and providing a const accessor function if required.
Prefer class attributes to derivation
To differentiate among horror films and comedies, the current code derives new classes from the base Struktur (or in my renamed version, Film) class. However, the only difference appears to be in how each film affects the Player rather than any deeper difference. For that reason, I’d suggest instead using an enum to name the different genres and then have that be an argument passed to the constructor.
Use a better random number generator
The price of admission to a comedy film is 10 half the time, and 20 half the time according to the comments. We can use a more modern random number generator with C++11.
std::random_device rd;
std::mt19937 gen(rd());
std::bernoulli_distribution cheap(0.50);
int price = (cheap(gen) ? 10 : 20);
Omit return 0
When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.
Note: when I make this suggestion, it’s almost invariably followed by one of two kinds of comments: “I didn’t know that.” or “That’s bad advice!” My rationale is that it’s safe and useful to rely on compiler behavior explicitly supported by the standard. For C, since C99; see ISO/IEC 9899:1999 section 5.1.2.2.3:
[…] a return from the initial call to the
mainfunction is equivalent to calling theexitfunction with the value returned by themainfunction as its argument; reaching the}that terminates themainfunction returns a value of 0.
For C++, since the first standard in 1998; see ISO/IEC 14882:1998 section 3.6.1:
If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;
All versions of both standards since then (C99 and C++98) have maintained the same idea. We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function. Reasons against omitting seem to boil down to “it looks weird”. If, like me, you’re curious about the rationale for the change to the C standard read this question. Also note that in the early 1990s this was considered “sloppy practice” because it was undefined behavior (although widely supported) at the time.
So I advocate omitting it; others disagree (often vehemently!) In any case, if you encounter code that omits it, you’ll know that it’s explicitly supported by the standard and you’ll know what it means.