2
\$\begingroup\$

I am trying to write a program that will read the highest and lowest voltage along with its associated amp and time. I am able to find the max, but I am getting all zeros for the minimum value. I need help. Please and thank you.

Sample of txt file:

Time    Volt    Ampere
0.0001  9.77667 0.147408
0.00015 9.76583 0.147525
0.0002  9.76833 0.147692
0.00025 9.75833 0.147442
0.0003  9.76833 0.147192
0.00035 9.78167 0.1473
0.0004  9.76667 0.147317
0.00045 9.765   0.14715
0.0005  9.75667 0.147
0.00055 9.765   0.14695
0.0006  9.77    0.1471
0.00065 9.7675  0.147417
0.0007  9.7725  0.147417
0.00075 9.755   0.14735
0.0008  9.765   0.147725
0.00085 9.76583 0.147783

Expected Min Output:

Time = 0.00075  Volt = 9.755 Ampere = 0.14735

Expected Max Output:

Time= 0.00035 Volt = 9.78167 Ampere = 0.1473

int main(void)
{
//Declare Variables
string a, b, c;

double time, volt, ampere;
double maxVolt = 0, maxTime, maxAmpere;
double minVolt = 10, minTime, minAmpere;

//Read from the Volts.txt file
ifstream myFile("D:\\tabit\\Documents\\Volts.txt");

//Check if the file can be opened
if (myFile.is_open())
{
    while (myFile >> a >> b >> c)
    {
        time = atof(a.c_str());
        volt = atof(b.c_str());
        ampere = atof(c.c_str());

        if (volt >= maxVolt)
        {
            maxTime = time;
            maxVolt = volt;
            maxAmpere = ampere;
        }
        if (volt < minVolt)
        {
            minTime = time;
            minVolt = volt;
            minAmpere = ampere;
        }
    }
    //Close the file
    myFile.close();
}
//Give error message if the file cannot be opened
else return(1);

//Display the Maximum results
cout << "Max Volt: " << maxVolt << endl;
cout << "Max Time: " << maxTime << endl;
cout << "Max Ampere: " << maxAmpere << endl;

//Display the Minimum results
cout << "Min Volt: " << minVolt << endl;
cout << "Min Time: " << minTime << endl;
cout << "Min Ampere: " << minAmpere << endl;

return 0;
}
\$\endgroup\$

closed as off-topic by Sᴀᴍ Onᴇᴌᴀ, 200_success, chux, Heslacher, t3chb0t Nov 2 '18 at 7:30

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Code not implemented or not working as intended: Code Review is a community where programmers peer-review your working code to address issues such as security, maintainability, performance, and scalability. We require that the code be working correctly, to the best of the author's knowledge, before proceeding with a review." – Sᴀᴍ Onᴇᴌᴀ, 200_success, chux, Heslacher, t3chb0t
If this question can be reworded to fit the rules in the help center, please edit the question.

2
\$\begingroup\$

The value of atof("Volt") is zero. The corresponding values of atof("Time") and atof("Ampere") are also zero, which gives you the minimum values of zero you observed.

You need to skip the first line of the file. Adding this before the while loop would work:

myFile.ignore(80, '\n');

Use std::numeric_limits<std::streamsize>::max() instead of 80 for a more correct (but verbose) limit of characters to skip before the new line character.


After you add the skipping of the first line, you no longer need the a, b & c variables and atof() calls; you can read the values directly:

while (myFile >> time >> volt >> ampere)
\$\endgroup\$
2
\$\begingroup\$

Your code is missing the headers you need - at least <fstream> and <iostream>. It also seems that there's a using namespace std; lurking somewhere; that's a bad practice that can subtly break your code, so I recommend that you explicitly qualify the names you use from it (std is intentionally a very short name, to save typing).

In C++, we can declare int main() - that's a prototype, unlike in C, where we need to write int main(void). The latter is certainly foreign-looking in C++.

It's probably clearer to return early if the file opening failed:

if (!myFile) {
    std::cerr << "Couldn't open input file";
    return 1;
}

I'm a little uncomfortable with the compiled-in filename to read from - this makes the program quite inflexible. Either provide the name as a command-line argument, or simply read from standard input so the program can read from any file or from a pipeline.

There's little point closing the input stream if we ignore the result - the stream's destructor will do that for us, so just let it go out of scope.

Be warned that std::endl includes a flush of the output stream - it's better to just write '\n' as line terminator (the stream will be flushed at program exit).

\$\endgroup\$
1
\$\begingroup\$
  • Usually, only working code can be reviewed. Here you have missing includes, missing using directive, ...
  • Don't declare multiples variables on the same line
  • Defines variables in the closest scope possible, here, after checked opening file.
  • Here, since your values (time, volt, ampere) seems to be packaged together, why doesn't package them in a struct?
  • In the same way, your max* and min* values come in pair, use a std::pair.
  • You declare your values as double but assign an integer. Instead, assign 0.0 and 10.0
  • Instead of a whole nested if, use a guard clause, as showed by @TobySpeight
  • Comparing floating points can sometime give wrong results, be carefull, and maybe backup on a safer way to do it. (more info)
  • If max volt equal current volt, you don't want to reassign, so a comparison "lower-than" should be sufficient. Or change the relation in the comparison between min volt and current volt to be consistent.
  • Skip your first line with myFile.ignore(std::numeric_limits<std::streamsize>::max(), '\n'); (need to #include <limits>).
  • Why place values in strings and after, convert to doubles? Put directly to doubles!
  • Using std::endl send a '\n' char and then flushes the output buffer, you don't have to pay for a flush unless you know you have to. And when you want it, explicitly use std::flush to show your intends.
  • You don't want to explicitly close the stream, the RAII magically do it for you.

Putting it all together, it gives:

#include <fstream>
#include <iostream>
#include <limits>
#include <utility>

struct Record
{
    double volt{};  
    double time{};
    double ampere{};  
};

int main()
{
    std::ifstream myFile("records.txt");

    if (!myFile) {
        std::cerr << "Couldn't open input file";
        return 1;
    }

    myFile.ignore(std::numeric_limits<std::streamsize>::max(), '\n');

    Record current{};
    auto minmax = std::make_pair<Record, Record>({10.,10.,10.}, {0.,0.,0.});

    while (myFile >> current.time >> current.volt >> current.ampere) {
        if (current.volt < minmax.first.volt) {
            minmax.first = current;
        }
        if (current.volt > minmax.second.volt) {
            minmax.second = current;
        }
    }

    //Display the Minimum results
    std::cout << "Min Volt: "   <<  minmax.first.volt      << '\n';
    std::cout << "Min Time: "   <<  minmax.first.time      << '\n';
    std::cout << "Min Ampere: " <<  minmax.first.ampere    << '\n';

    //Display the Maximum results
    std::cout << "Max Volt: "   <<  minmax.second.volt     << '\n';
    std::cout << "Max Time: "   <<  minmax.second.time     << '\n';
    std::cout << "Max Ampere: " <<  minmax.second.ampere   << '\n';

    return 0;
}

Test online

Note: You can also put the records in a vector. In that way, you can easily work with your data (filtering, sorting, ...) or find the min and max with std::minmax

Edit: at least, if you think I deserve it, tell me why

\$\endgroup\$
  • 1
    \$\begingroup\$ we probably all received downvotes for rushing in to review the code without checking that it actually works - we should have flagged or voted to close, instead. \$\endgroup\$ – Toby Speight Nov 2 '18 at 9:07
  • \$\begingroup\$ Yeah, I'm still new here, I have to be careful about off-topics. I noticed the lacking code (includes, using, ...), that should have been as a first big red alert signal. Plus he said that his code doesn't work as expected. There's the second red signal. Fine, I understand :) \$\endgroup\$ – Calak Nov 2 '18 at 9:34

Not the answer you're looking for? Browse other questions tagged or ask your own question.