Came back after 7 years to learn C++ and Software Development Patterns.

[phone98]

---- PreDescription ----
Started programming back in 2015-2016 using C++ and then got lost in many other programming languages. Python, Java, C#, JS, HTML just to name a few lol.
I have decided I will only focus on C++ and python. Someone else can take care of the rest lol.

---- Description ----
I'm looking for feedback :). straight up blunt honesty lol and criticism with my code anything is welcome XD :D.

---- Current Project ----
Currency Exchanger (Will evolve to Stock Market Analyzer/Scanner/Tool)also forex, crypto...
1. Practicing with interfaces.
2. Wanted to introduce Commands Pattern(unsure if its a fit) but wanted to practice.
hence the (Exchange, Deposit, Withdraw, Exit) feedback on what to look for or anything is welcome :)

---- Reason ----
To practice on SWD Patterns, and C++ concepts.

-- CODE --

Main [CurrencyExchanger.cpp]

// CurrencyExchanger.cpp : This file contains the 'main' function. Program execution begins and ends there.
//

#include <iostream>
#include "ExchangeOne.h"

using namespace std;

int main()
{
	ExchangeOne BankOne;
	BankOne.setCompanyName("BankOne Bank");
	BankOne.start();

	return 0;
}

ExchangeOne.h

#pragma once
#include "ExchangeInterface.h"


class ExchangeOne : public ExchangeInterface {
public:
	double deposit();
	double withdraw();
	void exit();

	double applyFee();

private: 
	void HandlePromptLoop();
	void HandlePrompt();
	int USER_CHOICE;
};

ExchangeOne.cpp (method definition)

#include "ExchangeOne.h"

double ExchangeOne::deposit() {
	return 0.0;
}

double ExchangeOne::withdraw() {
	return 0.0;
}

double ExchangeOne::applyFee() {
	return 0.0;
}

void ExchangeOne::exit() {
	std::cout << "Thank you for your business from " << CompanyName << std::endl;
}

void ExchangeOne::HandlePromptLoop() {
	if (CompanyName == "") {
		std::cout << "Missing Company Name, unable to continue.\n";
		return;
	}

	do {
		prompt();
		std::cin >> USER_CHOICE;
		HandlePrompt();
	} while (USER_CHOICE != EXIT_CODE);
}

void ExchangeOne::HandlePrompt() {
	double x;
	std::string currencyToExchange;
	switch (USER_CHOICE)
	{
	case EXCHANGE:
		std::cout << "To what currency would you like to exchange: ";
		std::cin >> currencyToExchange;
		std::cout << "How much would you like to exchange: ";
		double userInput;
		std::cin >> userInput;
		x = exchangePlease(currencyToExchange, userInput);
		std::cout << "Your " << userInput << " Euros will be " << x << " " << " USD." << std::endl;
		break;
	case DEPOSIT:
		deposit();
		break;
	case WITHDRAW:
		withdraw();
		break;
	case EXIT_CODE:
		exit();
		break;
	default:
		break;
	}
}

ExchangeInterface.h

#pragma once
#include <string>
#include <iostream>
#include "Currency.h"

#define EXCHANGE 1
#define DEPOSIT 2
#define WITHDRAW 3
#define EXIT_CODE 4

class ExchangeInterface : private Currency {
public:
	virtual double exchangePlease(std::string toCurrency, double amount) {
		return exchange(toCurrency, amount);
	}
	virtual double applyFee() = 0;
	virtual double deposit() = 0;
	virtual double withdraw() = 0;
	virtual void exit() = 0;

	virtual void start() {
		HandlePromptLoop();
	}

	void setCompanyName(std::string companyName) {
		CompanyName = companyName;
	}
protected:
	virtual void HandlePromptLoop() = 0;
	void prompt() {
		std::cout << "Welcome to " << CompanyName << ", What can I do?\n";
		std::cout << "1. Exchange\n2. Deposit\n3. Withdraw\n4. Exit\n";
		std::cout << "User Choice: ";
	};
	
	int numberOfTrades;
	std::string CompanyName;
};

Currency.h

#pragma once
#include <fstream>
#include <string>
#include <unordered_map>
#include <iostream>


class Currency {
protected:
	virtual double exchange(std::string currency, double qty) {
		if (RatesBaseEuro.empty()) {
			loadLatestRateFile();
		}
		if (!RatesBaseEuro.count(currency)) {
			std::cout << "Key does not exist\n";
		}

		return std::stod(RatesBaseEuro.at(currency)) * qty;
	}

private:
	std::unordered_map<std::string, std::string> RatesBaseEuro;

	void loadLatestRateFile() {
		std::ifstream latestRateFile("latest_rates.rt");
		int index = 1;
		std::string prevCurrencyCode;

		if (latestRateFile.is_open()) {
			while (latestRateFile.good()) {
				std::string line;
				getline(latestRateFile, line);
				tokenize(line, ",");
			}
			latestRateFile.close();
		} else {
			std::cout << "Error: File could not be opened\n" << std::endl;
		}
	}
	
	void tokenize(std::string word, std::string delimeter = " ") {
		int start, end = -1 * delimeter.size();
		std::string toReturnArray[2];

		int index = 0;
		do {
			start = end + delimeter.size();
			end = word.find(delimeter, start);
			//std::cout << word.substr(start, end - start);
			toReturnArray[index++] = word.substr(start, end - start);
		} while (end != -1);

		RatesBaseEuro.insert(std::pair<std::string, std::string>(toReturnArray[0], toReturnArray[1]));
	}
};

---- Code Explained [in my head] ----
ExchangeOne -> ExchangeInterface -> Currency
anyExchange can use the interface and apply their own fees.
The interface acts as a facade or (api?) for the developer using the "tools"

Main just makes an object of ExchangeOne as if I was using the tools,
Exchange, ApplyFees and what not.

[seeplus]

A few initial observations.

main() doesn't need a return 0 at end. This is assumed. main() is the only function with a return value where a return of 0 is assumed by the compiler if not explicitly coded.

stod() et al can cause an exception if not a number to be converted. Hence try/catch clauses should be used.

There is no attempt to check for valid numeric input. attempting to obtain a numeric from a stream when a numeric isn't presented causes the stream to enter a fail state which needs to be handled.

Rather than .insert, you can use .emplace with just the elements.eg

RatesBaseEuro.emplace(toReturnArray[0], toReturnArray[1]);

The code for reading from a file could be improved. eg

for (std::string line; std::getline(latestRateFile, line); tokenize(line, ","));

Don't use -1 for string error code. std::string::npos should be used.

For string split, have a look at std::ranges and split:
https://en.cppreference.com/w/cpp/ranges/split_view

Rather than .count() == 0 to see if an item exists or not, there is now .contains()
https://en.cppreference.com/w/cpp/container/unordered_map/contains

[jonnin]

a couple more...
#define constants are frowned upon these days. Consider an enum for your specific use, but you have other choices like const, using, etc.

using namespace std is also frowned upon now. There are many posts about it here if you need details on the problems it can cause.

in serious code, you want to split the UI from the work. That way if you need to reuse these classes in a new program with a GUI, its not tied to cout everywhere, or if it needs to be silent, its not spewing prints all the time.

prefer {} to initialize, eg int index{}; //zero or int index{42};//other value
{} inits force matched types, while = inits can silently (and usually harmless but hard to find when not) convert types.

I can't put my finger on it but tokenize may need a look. It could be right, but I am not sure how index stays in the {0,1} range.

[phone98]

@seeplus : Did not think about the try/except good catch :O thanks for the documentation link btw. I'll have to look back at documentation now.

@jonnin Yup That's the goal, complete decouple from UI. Might look a little better how to better the solution if possible. Might test tokenize a little more.

Thank you for the replies and the great insights :)

[seeplus]

Did not think about the try/except

If you don't want to use try/catch then consider using strtod()
https://cplusplus.com/reference/cstdlib/strtod/

or std:from_chars()
https://en.cppreference.com/w/cpp/utility/from_chars

but this doesn't ignore leading white-space as strtod() does.

[George P]

C++ try/catch exception handling has a lot of haters out in the wild, probably because they are used to the C style.

The C++ Core Guidelines has an entire chapter on error handling:

https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#S-errors

A simplistic "is it a number" way to parse input without a lot of overhead is accept the input from the user as a std::string, then use std::stringstream from that string as an input source to a numeric variable. Any failure means "the input isn't a number".

A crude example, header only, that supports a numeric menu system that can be fleshed out for other numeric types is:

https://github.com/GeorgePimpleton/misc_files/blob/main/Menu%20Toolkit/menu_toolkit.hpp

[seeplus]

The numeric input code could usefully be refactored into a stand-alone function for general usage. Consider:

#include <string>
#include <sstream>
#include <iostream>
#include <type_traits>

template<typename T>
concept NumericType = requires(T param) {
	requires std::is_arithmetic_v<decltype(param + 1)> && !std::is_same_v<bool, T> && !std::is_pointer_v<T>;
};

template<NumericType T = int>
T getNum() {
	for (std::string input; ; ) {
		if (T response {}; std::getline(std::cin, input))
			if (std::istringstream is { input }; (is >> response) && !(is >> input))
				return response;

		std::cin.clear();
		std::cerr << "Not a number!\n";
	}
}

int main() {
	while (true)
		const auto num { getNum() };
}

This could then be extended to include a prompt, optional min/max range and default value if just \n entered etc.

[George P]

seeplus wrote:

The numeric input code could usefully be refactored into a stand-alone function for general usage.

Something I am working at, slowly, because of lack of interest lately. :Þ

One eventual goal is to use it as an input method for a console based Star Trek game in C++ I've been mucking around with for years.

[seeplus]

I like Star Trek. I've got my own C++ version which emulates the original. I assume you have seen the original HP Basic code? If you haven't it's available on the internet if you want it.

Mine's based around four classes of Klingon, Enterprise, Galaxy and Quadrant. I had a previous version where the Klingons moved but I 'lost' that version and haven't bothered updating the one I currently have. It's not really that difficult if the 'right' data structures are used. I might get around to it again one day...

[George P]

I don't know if it was the HP BASIC version, but the game code was definitely BASIC. Almost 40 years ago, on dedicated teletype terminals that used paper tape for storage. High school.

Currently I am doing a design document to get a handle on what I want the game to differ from most ST games.

1. Instead of everything stored as digits in a matrix 8 by 8 and each sector is separate from the others I'd like to have the universe be more flexible. In every ST version I have ever seen even if the Enterprise is at the edge or a corner of a sector anything in an adjacent sector is unviewable. As well as being able to "see" across a vast distance to the other sides of the sector. So all the universe objects -- stars, star bases, enemies, etc. -- are stored in separate containers so it becomes possible to view space in a consistent direction, even across sector boundaries.

2. Have an input routine that allows for the user/player enter more than one input at a time. Command, then if needed any additional input. For instance, moving. Enter the move command and then direction as well as speed. Example (crude as it is):

MOV 72 4

The angle compass uses radians instead of degrees, with zero at the "top."

Mobile super enemies will be a possible feature, as well as finding planets orbiting stars that can replenish certain ship resources. A kind of lesser star base. Rare and have to scan the area and then orbit the star to get the single resource. Energy or raw resource to augment or fix other damaged systems.

Yes, combat can damage the Enterprise.

This game is in the barest beginning planning stage. Currently I am working on the user input system, trying to craft it as generic as possible so it could be used in a text adventure game. The basic interface would be [prompt, type of input(numeric, text)]. Partition multiple inputs from a single line entry into a container so each call for input checks the container for any possible stored input and processes that for validity. If not valid stored input or nothing in the container display the prompt and wait for input.

In essence the input routine is a version of a command line argument library.

[George P]

phone98 wrote:

Came back after 7 years to learn C++ and Software Development Patterns.

The biggest hurdle to overcome is how C++ has changed. C++17, C++20 and now C++23 made some radical changes to the language. What works well with pre-C++11, C++11 and C++14 might not be the best options now.

One feature of C++20 that really changes what can be done is std::format. There are other new features you should spend time investigating. Modules really "up the game".

Get a compiler that is as up-to-date as possible. C++20 compliant at a minimum. MSYS2 is a nice GCC compiler suite that is command line, MS Visual Studio for an IDE. MSYS2 is totally free, VS has a free edition that only requires an email address to activate. MSYS2 and VS are both Windows only. If you use a different OS it should be too hard to find usable compilers. GCC is AFAIK platform independent.

Use libraries as a first choice instead of writing code. A lot of programming hours have been expended to test many of the libraries by a lot of people. Especially Boost.

[seeplus]

Re 1). The Short Range Sensor Scanner only shows in detail the current quadrant - irrespective of the position of the Enterprise within that quadrant. Long Range Sensor Scan shows the summary contents (but not the positions) of each surrounding quadrant (where part of the galaxy) of the current quadrant. Library function 0 will show a cumulative galactic record of all quadrants scanned.

Yes, this is sort of limiting but that's part of the appeal. You don't know what's there until you're been there! If the Klingons have the ability to move (not in the original - but I liked it when I had it) then Library function 0 will then not necessarily be correct as it will be out of date.

2) For direction, the original neither used degrees nor radians - but a number from 1 to 8.9 (9 was allowed but had the same effect as 1) - with 1 being right, 3 up, 5 left and 7 down. The other whole numbers being a angle of 45 degree between. If a .nn number was used then this was an acute angle between the 2 whole numbers. Library Function 2 was used to calculate direction(s) and distance(s) to all Klingons (if any) in the current quadrant and also to calculate direction/distance between any pair of current quadrant co-ordinates.

as well as finding planets orbiting stars that can replenish certain ship resources

I quite like that idea! I'll think about it.

You must destroy 15 Klingons in 30 stardates with 8 starbases
There are 329 stars reported in this galaxy
Combat area      Condition RED
   Shields dangerously low

─═──═──═──═──═──═──═──═─

       *                      Stardate         3300
    *             <*>         Condition        Red
                              Quadrant         2,1
                     +++      Sector           7,3
                              Energy           3000
 *                            Photon Torpedoes 10
                              Shields          0
─═──═──═──═──═──═──═──═─

Command: 5
Energy available =3000
Number of units to shields: 300

Command: 7

Computer active and awaiting command: 2
Direction = 7.59
Distance = 2.24
Do you want to use the calculator (Y/N): n

Command: 4
Torpedo course (1 - 9): 7.59
85 units hit on Enterprise from sector 8,5 (215 left)
Torpedo track:
(8,4)
(8,5)
Congratulations. You have destroyed a Klingon!

Command: 2
Long range sensor scan for quadrant 2,1
─────────────────────────
│  008  │ <003> │  110  │
─────────────────────────
│  003  │  005  │  006  │
─────────────────────────

so quadrant (3, 1) has 1 Klingon and 1 Starbase and no stars.

Super Star Trek (published 1976) used 3 char mnemonics instead of menu numbers:

nav = Navigation
srs = Short Range Scan
lrs = Long Range Scan
pha = Phaser Control
tor = Photon Torpedo Control
she = Shield Control
com = Access Computer

It also named the quadrants.
See https://www.atariarchives.org/bcc1/showpage.php?page=275

[George P]

The BASIC version I was first exposed to used degrees for direction, so that is kinda "canonical" for me.

I have more ideas in the design document than I relayed earlier.

Yeah, the LRS is a numeric summary of what is currently known for each sector. The SRS won't be square, it will be rounded/cutoff at the corners with the Enterprise smack dab in the center.

Another possible way to view space is to have a crewman look out a bloody window, so a slice/segment of a SRS is done.

With each 'space object' being stored in a container leaving and reentering a sector won't reset the object's position within the sector. Each SRS will be constructed when the Enterprise moves to a new location by iterating through the space object containers. Yes, multiple containers for each type of space object. Mobile and immobile.

The location of objects several sectors away now becomes an issue when traveling farther than a short distance.

I understand the reasons behind the choices made with the sectors having visible barriers, etc. but PUH-LEEZE! At least try to have some physics realism with a 21st century program.

[seeplus]

MOV 72 4

An issue with a command like this is remembering the param order. One option would be to make it like a command line with option specifies. eg

MOV -d 72 -s 4

direction 72 degrees, distance 4. The ordering then wouldn't be important. Also you could have say:

MOV -h

to obtain help for that command etc etc etc.

[George P]

Uh, no.

The reason, my design reason, for the concatenation the way I have it is the order in which a user would get prompted if they didn't type multiple inputs on one line.

Command:  MOV

Direction: 72

Distance: 4

The game isn't an exercise in program command line argument token parsing. Most people would enter input one at a time with prompts.

If someone did multiple entries and muck up one of the expected tokens the input parser would reject the input and any remaining in the queue and prompt:

Command: MOV XP 4

Spock:  "I'm sorry, Captain, I didn't understand what direction you want.

Direction: 72

Distance: 4

Of course the direction and speed could have been combined into one entry:

Command: MOV XP 4

Spock:  "I'm sorry, Captain, I didn't understand what direction you want.

Direction: 72 4

One part of the input design is use for text adventure games, like ADVENT or Zork that could allow for entering multiple commands in game areas already mapped out.

[George P]

And if the user wanted help it would be

Command: MOV HELP

If the user really mucked things up by entering an invalid command from the start

Command: PLUGH

A block of text would be shown how to obtain the list of valid commands (or just show them) and how to get explanatory help for each command. The exact wording of the help is not even in the design document phase.

Another key feature I'd like to have as briefly shown above, crew member feedback.

Command: MOV 72 4

Chekov: Aye, Captain, course 72 laid in, warp factor 4.

A helpful Enterprise library computer command function would be to show the numeric direction compass rose. Zero pointing "north" and increasing direction numbers from zero going clockwise. Alternative direction is negative numbers from zero going counter-clockwise.

This is all still in the very rudimentary design stage of basic "what I'd like to do".

[George P]

Oh, and the exact command nomenclature isn't set in stone. MOV could be mov, maybe even NAV or nav. Or mixed case NaV, etc.

The input parser and testing it is my current "first" priority.

[mbozzi]

seeplus wrote:

template<typename T>
concept NumericType = requires(T param) {
  requires std::is_arithmetic_v<decltype(param + 1)> && !std::is_same_v<bool, T> && !std::is_pointer_v<T>;
};

template<NumericType T = int>
T getNum() // [...] 

Right now NumericType works like this:

static_assert(!NumericType<bool>);
static_assert(NumericType<bool const>);
static_assert(NumericType<std::atomic_bool>);
static_assert(NumericType<std::atomic_int>);

In the concept definition, I'm not sure what was intended by
std::is_arithmetic_v<decltype(param + 1)>
Maybe you meant
std::is_arithmetic_v<T>?

"Arithmetic types" are only floating point or integral types. CV-qualification doesn't matter, so both int and int const are arithmetic types. Pointers aren't arithmetic types.

Maybe you intended a concept like this, that checks for arithmetic types beside cv-bool?

template <typename T> concept numeric_type = 
  std::is_arithmetic_v<T> && !std::same_as<bool, std::remove_cv_t<T>>;

But it doesn't make sense to constrain getNum with this concept, since getNum doesn't need a NumericType to work properly. All it requires of T is that it's default-constructible and that (is >> response) compiles and is contextually-convertible to bool.

What's the point of writing that down in a concept? Consider leaving getNum unconstrained.

[seeplus]

What's the point of writing that down in a concept? Consider leaving getNum unconstrained.

Hmm. Originally this code was a simple unstrained template:

template<typename T = int>

Then this was constrained in the next version:

template<typename T = int>
typename std::enable_if_t< std::is_arithmetic_v<T>, T>

and then when it was converted to use concepts it was tried to be further constrained to try to force compilation errors(s) for 'unsuitable' types for a number. Yes, if >> is available for a type then this could be used for that type. However this isn't what was specified/required. It was to obtain 1 number and 1 number only for a numeric type from the console input.

I'll have a look at your suggested concept. Thanks.