3
\$\begingroup\$

I'm just looking for more feedback on style/approach regarding this small problem. Also, if you have tips to make it more readable or C++-like, please let me know.

Also, why is the problem asking for the sum and mean of each name and all names, while the sum of a single element is itself? Am I missing anything in terms of answering the question?

#include <stdio.h>
#include <stdlib.h>
#include <map>
#include <string>
#include <iostream>
#include <sstream>
#include <algorithm>
#include <iterator>

/********************************************************************
 * Read a sequence of possibly whitespace-separated (name,value)
 * pairs, where the name is a single whitespace-separated word and
 * the value is an integer or floating-point value. Compute and print
 * the sum and mean for each name and the sum and mean for all names
 * ******************************************************************/

using namespace std;

std::multimap<string,float> _map;

istream& operator>>(istream& stream, pair<string,float> &in ) {
    return stream >> in.first >> in.second;
}

ostream& operator<<(ostream& stream, pair<string,float> &out ) {
    return stream << "(" << out.first 
                  << ", " << out.second << ")" << endl;
}

int main( int argc, char *argv[ ] ) { 
    istream *is = &cin;
    do {
        pair<string,float> input;
        (*is) >> input;
        _map.insert(input);
    } while( is->peek( ) != EOF );

    ostream *os = &cout;
    multimap<string,float>::iterator mit = _map.begin( );
    float sum = 0.0;
    while( mit != _map.end( ) ) {
        pair<string,float> p_pair = (*mit);
        (*os) << p_pair;
        sum+=p_pair.second;
        mit++;
    }
    float mean = static_cast<float>( sum/_map.size( ) );
    (*os) << "Sum: " << sum << " Mean: " << mean << endl;
}
\$\endgroup\$
3
\$\begingroup\$

Although it might be a bit longer (I haven't bothered to count lines), I think I'd prefer something on this order:

#include <map>
#include <string>
#include <numeric>
#include <iostream>
#include <utility>
#include <iterator>
#include <algorithm>

typedef std::multimap<std::string, float> vtype;
typedef std::pair<std::string, float> ptype;

namespace std { 
std::istream &operator>>(std::istream &is, ptype &v) { 
    return is >> v.first >> v.second;
}
}

float add_second(float a, vtype::value_type b) { 
    return a + b.second;
}

template <class FwdIt>
float average(FwdIt start, FwdIt end) { 
    float total = std::accumulate(start, end, 0.0f, add_second);
    return total / std::distance(start, end);
}

int main() {
    vtype values((std::istream_iterator<ptype>(std::cin)), 
                  std::istream_iterator<ptype>());

    vtype::iterator start, end;

    for (start = values.begin(); start != values.end(); start=end) {
        end = values.upper_bound(start->first);
        std::cout << start->first << ": " << average(start, end) << "\n"; 
    }
    std::cout << "Overall: " << average(values.begin(), values.end()) << "\n";
    return 0;
}

If you preferred, you could use an std::vector instead of an std::multimap. That would require some minor changes to the code (e.g., using std::upper_bound instead of std::multimap::upper_bound, and adding a call to std::sort after reading the data, but would otherwise be pretty similar. It would, however, probably be a bit more efficient (less CPU time and memory).

While I suppose others might disagree, I prefer this for the simple reason that I find it easier to understand. We start by reading in all the data. We then find each run of identical keys, and compute the average for that run and print it out. When we reach the end of the data, we do the same for the overall average.

In fairness, if you were working with huge amounts of data, this probably wouldn't be the best way do do things. It does spend more time walking through the same data than is strictly necessary. At the same time, unless you're going to rewrite the code to overlap reading the data with computing the result, it's unlikely to make much real difference -- almost regardless of how you do the computation, that I/O will almost certainly occupy the vast majority of the overall time.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I went looking for this function on the sgi website, which is what I have been using for my on-line resource. But I can't seem to find this function values((std::istream_iterator<ptype>(std::cin)), std::istream_iterator<ptype>()); What does it do? \$\endgroup\$ – Matthew Hoggan Aug 1 '11 at 21:17
  • \$\begingroup\$ @Matthew: it's not really a function -- it's defining a vector named values, and initializing it from a pair of iterators (in this case, istream_iterators, which read data from an istream using operator>>). In case you care, the double parens around the first argument are to prevent the "most vexing parse". \$\endgroup\$ – Jerry Coffin Aug 1 '11 at 21:23
  • \$\begingroup\$ I guess I was just having a hard time reading the syntax. Typedefs really throw me for a loop sometimes. Being taught Java in school has done me much harm. Thanks for the clarification. \$\endgroup\$ – Matthew Hoggan Aug 1 '11 at 21:30
  • 1
    \$\begingroup\$ @Matthew: To paraphrase George Bernard Shaw, "C++ and Java are two languages separated by a common syntax." \$\endgroup\$ – Jerry Coffin Aug 2 '11 at 1:43
2
\$\begingroup\$

Comments:

At the file scope all words beginning with underscore are reserved.
See: https://stackoverflow.com/questions/228783/what-are-the-rules-about-using-an-underscore-in-a-c-identifier/228797#228797

std::multimap<string,float> _map;

The aexct rules are a pain to remember (I had to go and check to make sure it was reserved). So I never prefix an identifier with '_'.

I would not define stream operators for standard types.

istream& operator>>(istream& stream, pair<string,float> &in ) 
ostream& operator<<(ostream& stream, pair<string,float> &out )

The trouble with doing this is that you may (in larger projects) clash with other people doing the same or with standard operators. (I though this would already work for std::pair without any work (obviously I was wrong)) I would only ever write stream operators fro my own types.

References are your friend:

istream *is = &cin;

Why take the address of a stream. Create a reference:

istream& is = &cin;

This is almost certainly wrong:

    (*is) >> input;
    _map.insert(input);

What happens if the input operator fails?

    while(is >> input)
    {
        // Do until the input fails.
    }

So rewriting your loop:

pair<string,float> input;
while(is >> input)
{
    // Is not the whole point that a name may appear multiple times.
    // Otherwise why compute the sum and average for each name
    // Thus you need to increment the count.
    myMap[input.first] += input.second;
}

Not a big deal with a small object but I would get in the habit of using references.

pair<string,float> p_pair = (*mit);         // This makes a copy.

pair<string,float> const& p_pair = (*mit);  // This is an alias to the object.

Here is what I would do:

#include <map>
#include <memory>
#include <iterator>
#include <algorithm>
#include <iostream>

// Simple formatter used to 
// Print the data in a particular way. This formatter is tied to the particular storage format.
struct OutputFormater
{
    std::map<std::string, std::pair<int, int> >::value_type const& data;

    OutputFormater(std::map<std::string, std::pair<int, int> >::value_type const& d)
        : data(d)
    {}

    friend std::ostream& operator<<(std::ostream& stream, OutputFormater const& value)
    {
        std::string const&   name   = value.data.first;
        int const&           count  = value.data.second.first;
        int const&           sum    = value.data.second.second;

        return stream << name << ":  " << sum << ": " << (sum/count) << " ";
    }
};

This I hope makes main very east to read.

int main()
{
    std::map<std::string, std::pair<int, int> >     data;
    std::string     name;
    int             count;

    int             totalCount = 0;
    int             totalSum   = 0;

    // Read in the data
    while(std::cin >> name >> count)
    {
        totalCount++;
        totalSum  += count;
        data[name].first++;             // increment the count;
        data[name].second   += count;   // increment the sum of counts;
    }

    // Copy the container to a stream using a formatter.
    std::copy(data.begin(), data.end(), std::ostream_iterator<OutputFormater>(std::cout, "\n"));
    if (totalCount > 0)
    {
        std::cout << totalSum << ": " << totalSum/totalCount << "\n";
    }
}
| improve this answer | |
\$\endgroup\$
0
\$\begingroup\$

This is an alternative solution that also uses many things from the STL. It doesn't separate input from calculation, though.

#include <algorithm>
#include <iostream>
#include <map>
#include <numeric>
#include <string>
#include <utility>

using std::cin;
using std::cout;
using std::map;
using std::pair;
using std::string;

/********************************************************************
 * Read a sequence of possibly whitespace-separated (name,value)
 * pairs, where the name is a single whitespaace-separated word and
 * the value is an integer or floating-point value. Compute and print
 * the sum and mean for each name and the sum and mean for all names
 * ******************************************************************/

void data_print_plain(const string &name, double sum, int count) {
  double mean = sum / count;
  cout << name << " sum " << sum << " mean " << mean << "\n";
}

void data_print(const pair<string, pair<double, int> > &p) {
  data_print_plain(p.first, p.second.first, p.second.second);
}

double add(double x, const pair<string, pair<double, int> > &p) {
  return x + p.second.first;
}

int main() {
  map<string, pair<double, int> > data;

  string name;
  double value;
  while (cin >> name >> value) {
    pair<double, int> &p = data[name];
    p.first += value;
    p.second++;
  }

  for_each(data.begin(), data.end(), data_print);

  cout << "\n";
  double total = accumulate(data.begin(), data.end(), 0.0, add);
  data_print_plain("total", total, data.size());

  return 0;
}
| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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