Finished my current project, but....eeew

[omgdogonfire]

The program works from top to bottom, but I am afraid it would make my old CS151 teacher weep endless tears of agony. By this I mean it is highly inefficient. At least it seems that way after getting it working and compiled.

I was wondering if anyone could help me make this program a bit more concise?

Program:
Read a file that contains some values on our Unix server. Display the values, sleep for 30 seconds, reread the file, compare the new values against the old ones and display the differences.

My code:

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

int main()
{
  int readIO;        // number of readIO processed
  int readIOTwo;

  int readMerges;    // number of readIO merged
  int readMergesTwo;

  int readSectors;   // number of sectors read
  int readSectorsTwo;

  int readTicks;     // total wait time for read requests
  int readTicksTwo;

  int writeIO;       // number of writeIO processed
  int writeIOTwo;

  int writeMerges;   // number of writeIO merged
  int writeMergesTwo;

  int writeSectors;  // number of sectors written
  int writeSectorsTwo;

  int writeTicks;    // total wait time for write requests
  int writeTicksTwo;
  
  int inFlight;      // number of IO currently in flight
  int inFlightTwo;

  int ioTicks;       // total time this block device has been active
  int ioTicksTwo;

  int timeInQue;     // total wait time for all requests
  int timeInQueTwo;

  // This block of code reads the stats file and displays the
  // starting values we are going to work with.

  ifstream myFile;   // create input file stream
  myFile.open("/sys/block/sda/stat");
  myFile >> readIO;
  myFile >> readMerges;
  myFile >> readSectors;
  myFile >> readTicks;
  myFile >> writeIO;
  myFile >> writeMerges;
  myFile >> writeSectors;
  myFile >> writeTicks;
  myFile >> inFlight;
  myFile >> ioTicks;
  myFile >> timeInQue;

  cout << "\nFirst set of values:" << endl;
  cout << "Read I/Os: " << readIO << endl;
  cout << "Read Merges: " << readMerges << endl;
  cout << "Read Sectors: " << readSectors << endl;
  cout << "Read Ticks: " << readTicks << endl;
  cout << "Write I/Os: " << writeIO << endl;
  cout << "Write Merges: " << writeMerges << endl;
  cout << "Write Sectors: " << writeSectors << endl;
  cout << "Write Ticks: " << writeTicks << endl;
  cout << "in_flight: " << inFlight << endl;
  cout << "io_Ticks: " << ioTicks << endl;
  cout << "time_in_que: " << timeInQue << endl;

  myFile.close();    // closes the stats file
  sleep(30);         // sleeps the program for 30 second

  // This block of code reads the stats file again, and displays
  // the updated values we will use to calculate the differences

  ifstream myFileTwo;   // create input file stream
  myFileTwo.open("/sys/block/sda/stat");
  myFileTwo >> readIOTwo;
  myFileTwo >> readMergesTwo;
  myFileTwo >> readSectorsTwo;
  myFileTwo >> readTicksTwo;
  myFileTwo >> writeIOTwo;
  myFileTwo >> writeMergesTwo;
  myFileTwo >> writeSectorsTwo;
  myFileTwo >> writeTicksTwo;
  myFileTwo >> inFlightTwo;
  myFileTwo >> ioTicksTwo;
  myFileTwo >> timeInQueTwo;

  cout << "\nSecond set of values:" << endl;
  cout << "Read I/Os: " << readIOTwo << endl;
  cout << "Read Merges: " << readMergesTwo << endl;
  cout << "Read Sectors: " << readSectorsTwo << endl;
  cout << "Read Ticks: " << readTicksTwo << endl;
  cout << "Write I/Os: " << writeIOTwo << endl;
  cout << "Write Merges: " << writeMergesTwo << endl;
  cout << "Write Sectors: " << writeSectorsTwo << endl;
  cout << "Write Ticks: " << writeTicksTwo << endl;
  cout << "in_flight: " << inFlightTwo << endl;
  cout << "io_Ticks: " << ioTicksTwo << endl;
  cout << "time_in_que: " << timeInQueTwo << endl;

  myFile.close(); // closes the stats file

  // This block of code calculates the difference of our values
 
  cout << "\nDifferences between first and second values:" << endl;
  cout << "Read I/O Difference: " << readIOTwo - readIO << endl;
  cout << "Read Merges Difference: " << readMergesTwo - readMerges << endl;
  cout << "Read Sectors Difference: "
    << readSectorsTwo - readSectors << endl;
  cout << "Read Ticks Difference: " << readTicksTwo - readTicks << endl;
  cout << "Write I/O Difference: " << writeIOTwo - writeIO << endl;
  cout << "Write Merges Difference: "
    << writeMergesTwo - writeMerges << endl;
  cout << "Write Sectors Difference: "
    << writeSectorsTwo - writeSectors << endl;
  cout << "Write Ticks Difference: " << writeTicksTwo - writeTicks << endl;
  cout << "in_flight Difference: " << inFlightTwo - inFlight << endl;
  cout << "io_Ticks Difference: " << ioTicksTwo - ioTicks << endl;
  cout << "time_in_que Difference: " << timeInQueTwo - timeInQue << endl;
  cout << endl;

  return 0;
}

I was curious about a nested loop, but was not sure how to save the old variables when the file is opened the second time. =\

Aside from being a tad on the "bloated" side, I will format the output so the values displayed are better aligned, but that is going to be last thing I do to the program.

Any advice or insight on a way to chew the fat off this guy would be greatly appreciated!

:)

[helios]

I don't see any way to make it significantly more efficient. The only real problem is the fact that you appear to do the exact same thing (lines 45-72) twice, which would probably better inside a loop. Other than that, I don't see anything that's obviously wrong with the code.

[Nerdy314159265]

I think what you could do, just to make it look nicer, is make a struct and then declare a 2 element array of that struct. After that you could make it loop and just add a bit that can make it say second instead of first. Also you don't need 2 different ifstream variables you can just reopen the last one.

I don't see anything more than that atm.

[shacktar]

This is less efficient but more concise:

#include <iostream>
#include <fstream>
#include <string>
#include <map>
#include <utility>
using namespace std;

const int NUM_SERVER_VALUES = 11;
typedef map<string, pair<int, int> > serverInfo;

// This code reads the stats file and displays the current values
void ReadAndShowServerInfo(serverInfo& info, const string serverValueNames[], bool first)
{
    ifstream myFile;
    myFile.open("/sys/block/sda/stat");

    cout << "\n" << (first ? "First" : "Second") << " set of values:" << endl;

    for(int i = 0; i < NUM_SERVER_VALUES; i++)
    {
        myFile >> (first ? info[ serverValueNames[i] ].first : info[ serverValueNames[i] ].second);
        cout << serverValueNames[i] << ": " << (first ? info[ serverValueNames[i] ].first : info[ serverValueNames[i] ].second) << endl;
    }

    myFile.close();
}

int main()
{
    // number of readIO processed
    // number of readIO merged
    // number of sectors read
    // total wait time for read requests
    // number of writeIO processed
    // number of writeIO merged
    // number of sectors written
    // total wait time for write requests
    // number of IO currently in flight
    // total time this block device has been active
    // total wait time for all requests
    string serverValueNames[] = { "Read I/Os", "Read Merges", "Read Sectors", "Read Ticks", 
                                  "Write I/Os", "Write Merges", "Write Sectors", "Write Ticks",
                                  "in_flight", "io_Ticks", "time_in_queue" };

    serverInfo info;

    for(int i = 0; i < NUM_SERVER_VALUES; i++)
    {
        info[ serverValueNames[i] ] = pair<int, int>(0, 0);
    }

    // This code reads the stats file and displays the
    // starting values we are going to work with.

    ReadAndShowServerInfo(info, serverValueNames, true);

    sleep(30);

    // This code reads the stats file again, and displays
    // the updated values we will use to calculate the differences

    ReadAndShowServerInfo(info, serverValueNames, false);

    // This block of code calculates the difference of our values
    cout << "\nDifferences between first and second values:" << endl;

    for(int i = 0; i < NUM_SERVER_VALUES; i++)
    {
        cout << serverValueNames[i] << " Difference: " << info[ serverValueNames[i] ].second - info[ serverValueNames[i] ].first << endl;
    }

    cout << endl;

    return 0;
}

[omgdogonfire]

Thank you guys for the options and insights :)

and Shacktar - that is quite a significant, more compact version haha. I feel bad about posting my question now after seeing you rewrite everything...was not meaning for that to happen lol.

Unless you had done something similar in the past and just copied an old program..but regardless, thank you for giving me some tips on alternate ways to look at my problem.

Again, thank you all for taking time to respond!