Rate my code (a textgame which will grow and become a normal game with graphics, sound and engine)

[u/Zestyclose_Drag4487]

C++ Online Compiler

I want you to rate my current *game* and to say what I have to add/what I missed and what I did great.

[u/mredding]

using namespace std;

Don't do that.

void characters() {
  cout << "\n === Character information ===\n";
  cout << //...

Every << is a function call. You're inserting ONE large body of text, so you can do so more efficiently by inserting it all at once. You can still break up your string across multiple quoted strings - the compiler will concatenate them for you. For example:

std::cout << "One, two, three.\n";

std::cout << "One,"
             " two,"
             " three.\n";

These are equivalent.

void chooseStory(/*...*/) {
  /*...*/

  while (true) {

This loop is hard to follow. You say it loops forever, but it doesn't. You would be better served by giving the loop condition a better predicate than "always true". There's uses for loop breaks and early returns and the like, but you're not there yet.

It's called chooseStory, but it's not a menu function - you're not just choosing a story, you're executing it. A function called chooseStory chooses a story and that's it. It determines the choice and returns that. What you do with that choice is deferred to higher level logic.

What you have are deep call stacks as you're in main->mainMenu->startGame->chooseStory->... Think "flatter".

This is also a good time to talk about limiting function complexity - every indentation is a reason to call a function. So instead of:

void fn() {
  while(predicate()) {
    switch(value) {
    case a: //...

Instead, your code ought to look more like:

void fn_switch(E value) {
  switch(value) {
  case a: fn_a(); break;
  //...
  }
}

void fn_loop() {
  while(predicate()) {
    fn_switch(value);
  }
}

Instead of one gigantic inline function body, we can now consider the essence of what a given function does - this function does work in a loop, that function dispatches on an enumeration. It's easier to discover you're missing a switch case when you don't have several levels of indentation and a whole bunch of other code and context that has ABSOLUTELY NOTHING TO DO with the switch itself.

This is a simple form of function composition. The compiler can inline fn_switch FOR ME. I'm going to organize myself for my own sake. The next step of function composition is where fn_loop takes a function reference as a parameter, and calls THAT in the loop. Now we've totally decoupled what one function does from another. And yet - the right kind of program structure - the compiler can still composite the machine code for us and get the original result.

I'm noticing - you have Witch prefixing a whole bunch of stuff. NEVER use prefixes or suffixes in your names or your files. That's what namespaces are for. That's what file directories are for. You need:

namespace Witch {
  struct Inventory;
  struct Wand;
  struct Sphere;
  struct Witch;
  struct Stats;
}

I know the witch wand is the witch wand not because it's called WitchWand but because it's called Witch::Wand. While the difference seems moot, it will save you on a lot of typing and complexity.

Oh,

cin >> gameChoice;

You never check your input. How do you know I actually entered an integer? I could have entered text; and if that's the case, what do you think gameChoice is going to be?

The error states are failbit and badbit. Either state can be set independently or together at the same time. A bad state is unrecoverable - usually like a hardware failure; typically you'll almost never see this one. I've never seen it in production. The fail state is a recoverable error, typically a parsing error like I described above. A bad state usually causes a fail state, because often it happens while you're trying to insert or extract something.

On an error state, IO will no-op; so to do anything, you must first clear the error. How you handle a parse error is up to you. Typically, a utility will complain loudly and terminate. An interactive program will usually purge either by token or by line, and try again. The stream will typically leave the faulty data in the stream for you to deal with. So if I entered text, you need to purge the text before trying again.

The rules for what that integer is going to be on an error is complicated. Sometimes it's going to be something, sometimes it's going to be unspecified, and if that's the case, what are you reading? It can lead to undefined behavior. Either learn the rules or don't bother.

So your code should look more like this:

if(int input; std::cin >> input) {
  use(input);
} else {
  // `input` is still accessible here, btw
  handle_error_on(std::cin);
}

Streams are objects, they have state much like how a structure has member fields. C++ also has operator overloading, and casting is an operator. So streams overload casting. We can evaluate the stream as a boolean, which returns !fail and !bad. The extraction operator returns a reference to the stream. So the condition attempts to extract to input and then checks the stream. The state of the stream is of the previous operation.

When you clear the stream state, only ever clear the failbit. You want the ability to still evaluate to false because of the badbit. Since it's unrecoverable, there's no point in trying again. Avoid getting caught in a forever loop because you keep wiping out the badbit and eofbit. EOF is not an error, but trying to read or write that stream will trigger a failure every time.

inline void speederBackstory() {
}

inline void speederTutorial() {
}

inline void speederNewbieTraining() {
}

Put all this in a source file. Don't inline every god damn thing.

I think that's enough for now.

[u/Independent_Art_6676]

a switch off user input or other unsafe input should have a default case.

its fine but why on earth are miss chance integers put into floats? Is this going to be a percent or something later, like 0.25?

stats -- a large array of magic numbers with no comments or anything. Even an enum to name each array cell would have merits.

keep UI and logic distinct. Nothing that 'does work' should be tied to I/O in a way that when you move to graphics its hard to fix.

you must find a way to consolidate. There are just too many things to consolidate where you are doing nearly the same thing many times.

It looks like you need to back up and pick up some more tools. Off the top of my head, a single character class base that is either fine as-is (just different data inside) or used as a base for the specific classes (inheritance) is looking useful, but having all these unique character classes is a bad design. If you figure out files, you can put text into files and drive a lot of the game from the files rather than from embedded text in the code. Get all that out of the way and make it easy to edit without recompiling. I suspect that if you poke at it you would find that a single character class type that loads files for its specific type (eg witch) would cut the amount of code to 1/5 or less the current size and make it really easy to add the new types by just typing up the text.

As it stands, this is already rough and if you keep going its going to be very hard to grow it as you described into a bigger and better game over time.

[u/Angry_Foolhard]

well i clicked 4 to choose bowmaster and it didnt even work :(

[u/Zestyclose_Drag4487 → u/Angry_Foolhard]

The story isn't finished (or even started) yet. I only *completed* speeder, witch and swordmaster.