0
#include <iostream>
#include <string>
#include <sstream>

using namespace std;

string getItemName(int k) {
    if(k == 0) {
        return "Sunkist Orange";
    } else if(k == 1) {
        return "Strawberry";
    } else if(k == 2) {
        return "papaya";
    } else if(k == 3) {
        return "Star Fruit";
    } else if(k == 4) {
        return "Kiwi";
    }
    return "";
}
    
int main() {
    double prices[] = {2.00, 22.00, 5.00, 6.00, 10.00};
    double total = 0.0;
    string cart[50];
    int key = 0;
    int weight = 0;
    int index = 0;
    cout << "Welcome to Only Fresh Fruit Shop\n\nToday's fresh fruit <Price per Kg>\n";
    cout << "0-Sunkist Orange RM2\n";
    cout << "1-Strawberry RM22\n";
    cout << "2-Papaya RM5\n";
    cout << "3-Star Fruit RM6\n";
    cout << "4-Kiwi RM10\n";

    while (key != -1) {
        double current = 0.0;
        cout << "Enter fruit code <-1 to stop>: " << endl;
        cin >> key;
        if (key == -1) {
            break;
        }

        cout << getItemName(key) << endl;
        cout << "Enter weight <kg> : " << endl;
        cin >> weight;
        current = prices[key] + weight;
        total = total + current;
    }

    cout << "-------------------------------------------------------\nReciept\n";
    for(int i = 0; i < index; i++) {
        cout << cart[i] << "\n";
    }
    cout << "TOTAL = RM" << total << endl;

    return 0;
}

This is my code so far. The system have to display what fruit the user have chosen at in the receipt. My code is not working on the receipt part. Is there any other way on how to improvise the code to make it simpler? How can I improvise?

Casey
  • 10,297
  • 11
  • 59
  • 88
KatanaXI
  • 11
  • 3
  • "Not working" is not good enough description of the problems. If you are looking for code review, go to [https://codereview.stackexchange.com/](https://codereview.stackexchange.com/) – Quimby Oct 12 '22 at 07:11
  • Just as a side note: a `switch` statement might have been more comfortable, though actually I'd provide an array of strings and would use `k` – after range check – as index into that one. Side note: If you use `unsigned int` instead the range check just needs to consider the upper bound as the lower one is 0 naturally... – Aconcagua Oct 12 '22 at 07:14
  • Your design is questionable. Why separate fruit names and prices into separate arrays? They are closely related, so instead prefer an array of structs each of containing both name and price – and potentially further information. – Aconcagua Oct 12 '22 at 07:16
  • `while (key != -1){ if (key == -1) { break; }}` is redundant – if the inner test succeeds the outer one isn't met any more anyway – and if the inner doesn't, the outer one cannot either. So prefer a (seeming) endless loop instead: `for(;;) { }` or `while(true)`. – Aconcagua Oct 12 '22 at 07:18
  • You should indent code properly, this improves readability tremendously. – Aconcagua Oct 12 '22 at 07:19
  • About [`using namespace std`](https://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice)... – Aconcagua Oct 12 '22 at 07:19
  • You wouldn't allow sub-kg weights, by the way (in contrast to my usual recommendations `double` might be more appropriate in this case…)? And prices per kg are calculated by *multiplication*, not addition (`current = prices[key] * weight;`)! – Aconcagua Oct 12 '22 at 07:21

3 Answers3

1

At very first you can re-organise your data better:

struct Product
{
    std::string name;
    double price;
};

This struct keeps the data closely related to a single product (fruit in this case) together locally.

You might organise these in an array (preferrably std::array, alternatively raw) making access to simpler – making your getItemName function obsolete entirely. Instead of a static array a std::vector would allow to manage your products dynamically (adding new ones, removing obsolete ones, ...).

You can even use this array to output your data (and here note that your condition in the while loop is redundant; if the inner check catches, the outer one cannot any more as you break before; if the inner one doesn't, the outer one won't either, so prefer a – seeming – endless loop):

std::vector<Product> products({ {"Apple", 2.0 }, { "Orange", 3.0 } });

for(;;)
{
    std::cout << "Welcome ... \n";
    for(auto i = products.begin(); i != products.end(); ++i)
    {
        std::cout << i - products.begin() << " - " << i->name
                  << " RM " << i-> price << '\n';
    }

    // getting key, exiting on -1

    if(0 <= key && key < products.size()
    {
        // only now get weight!
    }
    else
    {
        std::cout << "error: invalid product number" << std::endl;
    }
}

Now for your cart you might just add indices into the vector or pointers to products – note, though, that these will invalidate if you modify the vector in the mean-time – if you do so you need to consider ways to correctly update the cart as well – alternatively you might just empty it. Inconvenient for the user, but easy to implement…

In any case, such a vector of pointers to products would easily allow to add arbitrary number of elements, not only 50 (at least as much as your hardware's memory can hold...) and would allow for simple deletion as well.

Calculating the full price then might occur only after the user has completed the cart:

// a map allows to hold the weights at the same time...
std::map<Product*, weight> cart;

for(;;)
{
    // ...
    if(0 <= key && key < products.size()
    {
        double weight;
        std::cin >> weight;

        // TODO: check for negative input!
        //       (always count with the dumbness of the user...)
                           

        cart[&products[key]] += weight;
        // note: map's operator[] adds a new entry automatically,
        // if not existing
    }
}

Finally you might iterate over the cart, printing some information and calculating total price for the shopping cart:

double total = 0.0;
for(auto& entry : cart) // value type of a map always is a std::pair
{
    std::cout << entry.first->name << "..." << entry.second << " kg\n";

    total += entry.first->price * entry.second;
    // note: you need MULTIPLICATION here, not
    // addition as in your code!
}

std::cout << "Total price: RM " << total << std::endl;
Aconcagua
  • 24,880
  • 4
  • 34
  • 59
0

I noticed there is a string cart[50] and int index = 0which you did not use throuoght the whole code except printing it at the end of the code. I am guessing that you want to add the fruit into the cart but it seems like you have not done so.

double price[50];
while (key != -1) {
    double current = 0.0;
    cout << "Enter fruit code (<-1> to stop): ";
    cin >> key;
    if (key == -1) break;
    cout << getItemName(key) << endl;
    cout << "Enter weight <kg> : ";
    cin >> weight;
    cart[index] = getItemName(key);
    price[index] = prices[key] * weight;
    total += price[index];
    index++;
}
for (int i = 0; i < index; i++) {
    cout << cart[i] << "     " << price[i] << endl;
}

I have added some code so that cart[index] = getItemName(key). When you print each element of cart, it will now work. Also, current = prices[key] * weight is the correct one, not addition (unless your rules are different).

on a side note are you malaysian

0

This should do it whilst staying close to original code, I also improved you're method of gathering price/name a bit, try to catch the out of index exceptions or check if current index is NULL. Good luck!

#include <iostream>
#include <string>
#include <sstream>
#include <vector>;
using namespace std;

std::vector<std::string> itemList = {"Sunkist Orange", "Strawberry", "Papaya", "Star Fruit", "Kiwi"}; 
//If you dont want this to be global put it in the getItemName function

string getItemName(int k) {
    return itemList.at(k);
}

int main() {
    std::vector<double> prices = { 2.00, 22.00, 5.00, 6.00, 10.00 };
    double total = 0.0;
    int key = 0, weight = 0;
    cout << "Welcome to Only Fresh Fruit Shop\n\nToday's fresh fruit <Price per Kg>\n";
    cout << "0-Sunkist Orange RM2\n";
    cout << "1-Strawberry RM22\n";
    cout << "2-Papaya RM5\n";
    cout << "3-Star Fruit RM6\n";
    cout << "4-Kiwi RM10\n";

    while (key != -1) {
        double current = 0.0;
        cout << "Enter fruit code <-1 to stop>: " << endl;
        cin >> key;
        if (key == -1) {
            break;
        }

        cout << getItemName(key) << endl;
        cout << "Enter weight <kg> : " << endl;
        cin >> weight;
        current += prices.at(key) + weight;
        total += total + current;

        cout << "-------------------------------------------------------\nReciept\n";
        cout << "Purchased: " << getItemName(key) <<"  "<< "TOTAL = RM" << total << "\n" << endl;
    }

    return 0;
}
JohnySiuu
  • 156
  • 6
  • *Where*'s your try/catch??? Apart from, these are expensive in case of catching, far better is doing a manual range check *before* access and handle bad input in a simple `if-else` clause. – Aconcagua Oct 12 '22 at 07:56
  • Thats for OP to figure out, I mentioned checking for out of bounds index with if-else by checking that prices.at(key) does not return a NULL handle. Other than that I'm sure OP can solve the rest on his own. – JohnySiuu Oct 12 '22 at 08:03
  • Checking if current index is NULL??? You'd need to set it appropriately somewhere first. But you'd need pointers for `NULL` being meaningful at all. Apart from, you should prefer C++ *keywords* (`nullptr`) over old obsolete C *macros* (`NULL`). Can we expect a seeming beginner to already know about `try/catch`? Not really sure about. At least you might mention/add a comment that `at` is the place that might throw. For second array access (prices) you might prefer unchecked `operator[]`, as the check has been done already for the names – provided both vectors are of same size, of course… – Aconcagua Oct 12 '22 at 08:11
  • Would be better anyway to pack names and prices into a single vector of structs containing both. Alternatively a `std::map` or `unordered_map` mapping names to prices per kg. – Aconcagua Oct 12 '22 at 08:12
  • Sorry, I have a habit of capitalizing NULL, it just seems more neat to me :) other than that I wasn't referring to the C macro I was meaning it in a general way, again its for OP to figure out how to do it. I will add the comments after this, however even though I agree with std::map being more efficient, OP is still just a beginner, and would require a revamp from original code. I just tried to make it close to original code as possible whilst making it easier to read and access data types. Your version is definitely much better, ill make sure to use those tips in the future. Thanks! – JohnySiuu Oct 12 '22 at 08:19