0

I want to write a little program which should be used in supermarkets. everything is fictitious and it's only for learning purposes. However, The tool generate a new data for every new article. in the data there are 2 lines, the name and the prise. The data is named as the article number of the product. So the user enter a articlenumber and the tool looks for a data with this number, if it found it, it reads the 2 lines and initiates the variables. But for some reasons it does not convert and copy the strings correctly. here is the part which loads the data.

int ware::load()
{    
    string inhalt;

    cout << "please insert article number" << endl;
    cin  >> articlenumber;

    productname.open(articlenumber, ios::in);

    if (!productname.is_open())
    {
        cout << "can't find the product." << endl;
        return 1;
    }

    if (productname.is_open())
    {
        while (!productname.eof())
        {
            getline(productname, inhalt);
            strcpy(name,inhalt.c_str());

            getline(productname, inhalt); 
            price = atoi (inhalt.c_str());

            cout << inhalt << endl;
        }

        warenname.close();    
    }

    cout << endl << endl <<
        "number: " << inhalt <<
        "  preis: " << price <<
        "  name: " << name <<
        endl << endl; //this is a test and will be deleted in the final

}

hope you can help me!

Here is the class:

    class ware{

private:
    char     articlenumber[9];
    char     name[20];
    int      price;
    fstream  warennamefstream;
    ifstream warenname;

public:
    void newarticle(); //this to make a new product.
    void scan();       //this to 'scan' a product (entering the article number ;D)
    void output();     //later to output a bill
    int  load();       //load the datas.


};

hope everything is fine now.

Brian Tompsett - 汤莱恩
  • 5,753
  • 72
  • 57
  • 129
globus243
  • 710
  • 1
  • 15
  • 31
  • 4
    This is not enough information, half of the variables and types is not known. You should strip things down to a minimal compiling example and explain what you think it is doing different than what you want (we dont know what you wanted it to do, we can only say that it does what it does) – PlasmaHH Sep 23 '11 at 21:00
  • You're using `getline` and `eof` in the wrong. This has come up at least three times just today. I think we have a good reference question here on SO, but I can't find it at the moment. Maybe someone else can chime in. – Kerrek SB Sep 23 '11 at 21:01
  • How is `name` declared and why are you mixing c-style strings and std::string? I ask because of your use of `strcpy`. – Blastfurnace Sep 23 '11 at 21:02
  • 1
    http://stackoverflow.com/search?q=getline ;) – AJG85 Sep 23 '11 at 21:03
  • Why do I see `productname.open()` and `warenname.close()`? Is this your actual code? – Blastfurnace Sep 23 '11 at 21:06
  • @AJG85: I believe I once saw a very definite post on "how to and how not to use `>>` and `getline`". Maybe it wasn't on SO after all... – Kerrek SB Sep 23 '11 at 21:08
  • 2
    Rob's Rule #33: Never say `endl` when you mean `'\n'`. See [this question](http://stackoverflow.com/questions/5492380/what-is-the-c-iostream-endl-fiasco/5492605#5492605). – Robᵩ Sep 23 '11 at 21:19
  • i answer from above. 1. now everything is there sorry for this 2. well this code is similiar to another which already used succesfully. 5. no there only works with the fstream library i think. 7. i will remember this. ;D – globus243 Sep 23 '11 at 21:39

1 Answers1

1

First, you have a using namespace std; somewhere in your code. This occasionally leads to subtle bugs. Delete it. ( Using std Namespace )

int ware::load()
{    
    string inhalt;

    cout << "please insert article number" << endl;
    cin  >> articlenumber;

The type of articlenumber is incorrect. Declare it std::string, not char[]. ( What is a buffer overflow and how do I cause one? )

productname.open(articlenumber, ios::in);

There is no reason to have an ifstream lying around waiting to be used. Also, there is no point in providing ios::in -- it is the default. Just use the one-argument form of the ifstream constructor.

if (!productname.is_open())
{
    cout << "can't find the product." << endl;
    return 1;
}

Don't bother checking to see if the file opened. Your users don't care if the file was present or not, they care whether the file was present AND you retrieved the essential data.

if (productname.is_open())
{
    while (!productname.eof())
    {
        getline(productname, inhalt);
        strcpy(name,inhalt.c_str());

        getline(productname, inhalt); 
        price = atoi (inhalt.c_str());

        cout << inhalt << endl;
    }

    warenname.close();    
}

This loop is just wrong.

  • Never invoke eof(). It doesn't do what you think it does, and will cause bugs.
  • Why is this a loop? Aren't there only two lines in the file?
  • There is no point in calling close. Just let the file close when the istream goes out of scope.
  • Why is warename different than productname?
  • Don't store your data in char[]. This is the 21st century. Use std::string.

.

    cout << endl << endl <<
        "number: " << inhalt <<
        "  preis: " << price <<
        "  name: " << name <<
        endl << endl; //this is a test and will be deleted in the final
  • Never use endl when you mean to say '\n'. Each of those endl manipulators invokes flush, which can be very expensive. ( What is the C++ iostream endl fiasco? )
  • You forgot to return a value.

Try this instead:

int ware::load()
{    
    // This declaration should be local
    std::string articlenumber;
    cout << "please insert article number" << endl;
    cin  >> articlenumber;

    // This declaration should be local
    std::ifstream productname(articlenumber.c_str());

    // These declarations can be class members:
    std::string name;
    int price;
    std::string number;
    if(getline(productname, name) &&
        productname>>price &&
        productname>>number)
    {
        cout << "\n\n" <<
            "number: " number <<
            "  preis: " << price <<
            "  name: " << name <<
            "\n\n"; //this is a test and will be deleted in the final
        return 0;
    } else {
        cout << "can't find the product." << endl;
        return 1;
    }
}
Community
  • 1
  • 1
Robᵩ
  • 163,533
  • 20
  • 239
  • 308
  • great, this is how declarations should be. I'm a native german, so i write my codes in german. when i translated it, i made that mistake with warenname.close() sorry for this ;) – globus243 Sep 23 '11 at 22:14
  • The lesson from that is: Never type code into SO questions -- always copy and paste from a recently-compiled source file. – Robᵩ Sep 23 '11 at 22:16