Review by Backson

[Backson]

Hi 111nation, this is such a great side project! I love it!

I opened that up in Visual Studio 2022 and created a new solution and copied your files in there.

First thing I noticed is that the indentation is messed up. I had to set my tab width from 4 to 8, replace all indentation with tabs and set it back to 4 to have a chance of properly viewing your code. Please set up your editor such that it saves only tabs or only spaces and never mixes them. There is also whitespace at the end of lines, which you never want to have.

After fixing that I ran the program in the debugger and it works! Fantastic! Problem is, it looks like this:

That doesn't look right. Let's see if I can fix this. This doesn't look promising either:

Try to put const wchar_t vwall = L'\x2588'; so the code is ASCII but the correct Unicode glyph is used.

Okay, that still doesn't look like your screenshot, but that may just be my terminal emulator using a different font, who knows. I'll take it. Let's review some code!

You're throwing strings, never do that. If you use throw/try/catch, then the thrown object should always be a subclass of std::exception. No exceptions ;)

You do old-school memory management with new-delete. This is error prone and in fact you have a few memory errors. For example, in main() you never delete map. Another one is in the Map::Map() constructor, you create some fields with new and then later you can still throw "Cursor failure";. If that happens, your destructor doesn't run and the memory you allocated is leaked. To prevent this, use memory-safe classes or wrappers. In particular, if you need a heap-allocated array of stuff, use std::vector. If you would like to handle a raw pointer of a single object, use std::unique_ptr instead.

DON'T USE ALL CAPS IN COMMENTS, IT'S LIKE YELLING AND I DON'T LIKE MY CODE TO YELL AT ME, EXCEPT WHEN IT'S A MACRO.

Speaking of macros, avoid them. If you want to define constants do it like this: static constexpr char UP = 'A'. It is better, and even better is to use enums or enum classes, like this:

enum class MapCell : uint8_t {
  VerticalWall,
  HorizontalWall,
  Player,
  Coin,
  Space,
  Doow,
};

And then also make the map array an array of MapCell (or an std::vector).

Here is a thing I found: srand(time(0)); Don't use that, use random instead.

You seem to have noticed that your Map class is way too big, but spreading the function definitions over multiple files is not really helping. You should split the class too. A class should do "one thing", whatever that means. Here are some ideas for classes:

• The Map class, which holds the data and manages the memory (ideally by using unique_ptr and vector and not calling new or delete). It doesn't know the rules of the game, it doesn't know how it is presented to the user, it doesn't know where the data comes from. It just holds the data and has methods to manipulate it.
• A MapGenerator, which has some properties (like width, height, number of coins, whatever) and has a Generate() method which returns a Map or a unique_ptr
• A Renderer, which turns information from Map into characters in the console.
• A class which runs the simulation, grabs user input and updates the Map object, you could call it Controller, or Game or Application or Program or something.

Some more ideas: You should definitely be able to read maps from files. Maybe have options to generate maps with certain settings and save them as a file, so you can save them and play them later.

I think that's enough for now. Great work, keep working like that :) DM me if you ever need another review.

[111nation]

Thank you so much! My first review!

I will switch to using Unicode codes instead of the actual unicode character in the source code, and fixing the character output so it stays consistent on every terminal emulator. Do you have any suggestions on forcing a supported font on every terminal emulator, or using different characters?

😅 I was too thinking for a while to separate my class definition into multiple sub classes.

In terms of errors in general memory management and exception handling will look into that too, thank you for pointing that out!

I appreciate your review :)!

[Backson]

Terminals are for text, I don't think you can force any particular presentation with them. Most terminals understand VT100 escape codes, so you may be able to do something with that. If you want to force a certain visual style, you might as well move to a graphics library, like Allegro, SDL2 or SFML. But using the terminal is fine, just make sure it looks right on a bunch of them. Maybe make it an option to switch between pure ASCII, full Unicode or some special encoding, so the user can select what works for them.