The errors in your code were this:
c1.ShowDetails;
should be c1.ShowDetails();
Car c1
and RentalSystem r1
are different objects and you ask the user for a car name in c1
and try to use this name in r1
.
When you fix them both, your program should run in a way you might expect.
However, there are still a few design flaws (some of which are already mentioned in the comments under your question) in your code. The two most common ones are this "using namespace std" and "magic numbers".
using namespace std;
First of all, don't do using namespace ...
. This will just put everything from the namespace std
into the global namespace and might cause some naming collisions. In some cases (I use it for e.g. std::chrono_literals
) using this makes your code easier to read and is not that bad, but only in the innermost scope possible and not just globally. You can check out Why is "using namespace std;" considered bad practice? for reasons why not to use it.
Magic numbers
In your main()
function, you write if(choice == 1)
. I read your code and have no idea what 1
stands for, not even a guess. Therefore it will be hard for someone else (and probably you in a few weeks/months) to read/edit/maintain your code. I would recommend doing something like this:
constexpr int option1{ 1 }; // but give it a meaningful name, not like I just did
if (choice == 1) {
// ...
}
But you should of course use a meaningful name to it, since option1
doesn't tell me what it is either. You could check What is a magic number, and why is it bad? out aswell.
By the way I would recommend to use a better name for choice
as well, since this would help me to understand what option1
is even better. So in general don't use magic numbers and give your variables (and functions, classes, whatever) meaningful names.
DRY principle (Don't repeat yourself)
If I look at your for
loop in Car::CheckInput()
and other loops, there is much repetition going on. This makes your code way harder to change, since you have to change it in all those places. You use
seats = 5;
totalrent = seats * 10000;
cout << "Total Rent:" << totalrent;
break;
4 times, which just small differences (only the amount of seats changes). And pay attention to that 4
in your loops. You have it in each loop. If you get an additional car or get rid of one, you need to change this 4 in each single place.
Objects
As the comments under your question already say, you have string AvailableCars[4]
, int CarStock[4]
and a "standalone" amount of seats floating around.
Now imagine your dealership gets a new car. You have to read your whole code and try to find each place where you need to add information about this car. It would be much easier to have all information bundled in one place. You should try to restructure your classes to avoid this.
Further RentalSystem
is derived from Car
. Why is that? Is your RentalSystem
some sort of car? Maybe a cabrio? RentalSystem
should have a member which stores Car
s. Your "free floating" arrays from Car
should be in there. This would also make the Car
class redundant, but you might add other Vehicles
and additional functionality to your Car
later on so I think this is fine.
Finding the chosen car
Your for
loop in Car::CheckInput()
contains something like if (name == AvailableCars[0])
in it. With the index going from 0
up to 2
(which is the index before the last plus for the last a else
). This defies the whole reason why you would use a array and a loop and you could not just add a new Car
to your "free floating" arrays, you would also need to adjust this, which is just unnecessary work.
There are still some minor things which I didn't mention yet.
- I notices you
#include <fstream>
but don't use it and you didn't #include <string>
(which could already be in #include <iostream>
, but I would recommend still including it). (and please put a blank between the "#include
and <iostream>
even it is the same for the compiler :D)
- When you ask the user for input, tell them what exactly you expect from them (what does
"Enter choice pls"
mean exactly?) and put there some additional space too, like "Enter choice pls: "
, so it doesn't look like "Enter choice pls5"
after they wrote something.
- Always initialize your variables
My solution
#include <iostream> // please add space between "#include" and the file :D just a personal preference though
#include <string> // you use the class "std::string", so include it even if it might already get included with "iostream"
#include <array> // std::array has many advantages over c-style arrays, so I prefer it, use std::vector if you need to resize your array
#include <algorithm> // for std::find_if()
class Vehicle {
public:
Vehicle(const std::string& name, int seats)
: name(name), seats(seats) {
}
std::string name{};
int seats{};
//int model, totalrent; // you never use model, so why does it exists? If you want to add something later, its fine
// declaring multiple variables on the same line can lead to errors when using raw pointers (which you shouldnt anyways)
// so you might just do it on multiple lines
//int totalrent{}; // this should not be here, since the dealer calculates a rent, not the vehicle itself
};
// if you only ever have "Car" and no other "Vehicle" and you don't think of extending either one you don't need to have a sperate class
class Car : public Vehicle {
public:
Car(const std::string& name, int seats)
: Vehicle(name, seats){
}
void ShowDetails() const {
std::cout << "Name: " << name << ", ";
//std::cout << "Model: " << model << ", "; // if you decide to use "model", uncomment this
std::cout << "Seats: " << seats << std::endl;
}
};
// this should not inherit Car, since it is not a car, but might include multiple cars
class RentalSystem {
public:
void SimulateInteraction() {
ShowDetalis();
std::string carName{ GetInput() };
auto inputCarIter{ std::find_if(stock.begin(), stock.end(), [&carName](const StockItem& item) {
return item.car.name == carName;
}) };
if (inputCarIter == stock.end()) {
std::cout << "Sorry this car isn't available!" << std::endl;
return;
}
constexpr int basePrice{ 10'000 }; // I gues this is what your 10'000 is supposed to be?
int rent{ inputCarIter->car.seats * basePrice };
std::cout << "Total Rent: " << rent << std::endl;
inputCarIter->numberOfAvailableCars--; // if you rent one, decrement the amount of cars you have
}
// I have seperated this classes members and methods into public and private, since a user might not
// need or want to know everything about the details of your rental
private:
// don't have to seperate arrays which contain data which is related to each other
// you could use a std::pair<> for this too
struct StockItem {
Car car;
int numberOfAvailableCars;
};
std::array<StockItem, 4> stock{
StockItem(Car("Alto", 5), 4),
StockItem(Car("Mercedes", 2), 4),
StockItem(Car("Corolla", 3), 4),
StockItem(Car("Audi", 4), 4)
};
void ShowDetalis() const {
std::cout << "Available Cars are:" << std::endl; // use an additional std::endl
for (const auto& item : stock) {
// only print them if they are still available
if (item.numberOfAvailableCars > 0) {
item.car.ShowDetails();
}
}
}
std::string GetInput() {
std::cout << "Enter the car name you would like to rent: ";
std::string carName{};
std::cin >> carName;
return carName;
}
};
int main() {
int choice{};
std::cout << "Enter choice pls: ";
std::cin >> choice;
if (choice == 1) {
RentalSystem rental;
rental.SimulateInteraction();
}
// else if (choice != 1 && choice != 2) // (choice != 1) will always be true
// there is no additional branch for when the user inputs 2 since I dont know what it means
else {
std::cout << "Sorry choose between available options." << std::endl;
}
return 0;
}
If you have questions on something like lambda expressions (with the weird [](){}
syntax), std::find_if
, std::array
or something else feel free to leave a comment and I will try to explain them a bit.
I can recommend Learn C++ and The Definitive C++ Book Guide and List for learning all the basics and cppreference if you ever need to look something up.