0
\$\begingroup\$

It is my job to store the workout routine for a given individual in a multimap<string, Exercise*> using an overloaded extraction operator in main. I wanted to know if my code for said operator can be simplified and made more intuitive. There are three classes, but I have only shown Routines; the others seem trivial to include. Implementations will be done in the header file. I will be extracting information from the following file (routine.txt):

Donald Duck,2
Chin-ups,30
Lat Pulldown,20,90
David Ash, 2
Push-ups,5
Wrist curl,10, 2

The format is name,numberOfExercises followed by numberOfExcercises exercises. Each exercise involves weights or does not. A weighted exercise is formatted as exerName,reps,weight. A non-weighted exercise is formatted as exerName,reps. The inclusion of a space after a comma is not a mistake. main contains the following:

int main() {
   Routines routines;
   string name;
   ifstream inf("routines.csv");

   inf >> routines;
}

The routines class has the following:

#include <fstream>
#include <map>
#include "exercise.h"
#include "weightedexercise.h"

using namespace std;

class Routines
{
  multimap<string, Exercise*> routineMap;

public:
  friend istream& operator>>(istream& is, Routines& rout)
  {
    string name, line;
    char comma;
    int numEx, rep, weight = 0;
    // numEx is number of exercises.
    // name is name of person. line is the exercise information.

    getline(is, name, ',');

    do
    {
      is >> numEx;
      getline(is, line);

      for(int i = 0; i < numEx; i++)
      {
        getline(is, line, ',');
        is >> rep;
        is.get(comma);
        if(comma == ',')
          is >> weight;

        if(weight)
        {
          rout.routineMap.insert(multimap<string,
          Exercise *>::value_type(name, new WeightedExercise(line, rep, weight)));
          getline(is, line);
        }
        else
        {
          rout.routineMap.insert(multimap<string,
          Exercise *>::value_type(name, new Exercise(line, rep)));
        }

        weight = 0;
      }
    } while(getline(is, name, ','));
  }
};
\$\endgroup\$
  • \$\begingroup\$ Don't see any delete commands!. I think your problem will simply be solved be changing Exercise* into Exercise as the value type for your map. \$\endgroup\$ – Martin York Jun 15 '17 at 3:30
1
\$\begingroup\$

Avoid pointers

In this case, there seems to be no particularly good reason for your map to store pointers to the data instead of just storing the data directly. It'll simplify your code a bit if you just have it store something like a vector<exercise>. Alternatively, you could consider using a std::multimap<std::string, Exercise> (which has a similar effect).

Note that many people are likely to give advice more like: "avoid raw pointers". While that's good advice in itself, I prefer to avoid (explicit use of) pointers entirely if possible. If you find that you can't avoid using a pointer, then use a smart pointer. But if you can avoid using any sort of pointer at all, so much the better.

Distribution of responsibilities

You've defined an Exercise class (or struct), but all the responsibility for reading an Exercise from a file has been assigned to Routines. At least IMO, it would be better if an Exercise knew how to extract itself from a file, and a Routine just made use of that to read the exercises in a routine.

map data type

Right now, your Routines is basically just a map with an overload of operator>> to extract data and populate that map from the file. We don't need to define a new class just for that purpose though. map already uses a template to define the type of data it stores--we can just use that (and give it a name, if we like):

using routine = std::pair<std::string, std::vector<Exercise> >;

Putting those together we get extraction operators for routine and exercise that look something like this:

std::istream &operator>>(std::istream &in, Exercise &e) {
    std::string temp;
    std::getline(in, temp);
    std::istringstream is(temp);
    std::getline(is, e.name, ',');
    is >> e.reps;
    if (is.peek() == ',') {
        is.ignore(1);
        is >> e.weight;
    }
    else
        e.weight = 0;
    return in;
}

std::istream &operator>>(std::istream &is, routine &r) {
    std::getline(is, r.first, ',');
    int count;
    is >> count;
    is.ignore(1024, '\n');
    for (int i=0; i<count; i++) {
        exercise e;
        is >> e;
        r.second.push_back(e);
    }
    return is;
}

So now we've restricted the messiness of reading an exercise correctly to a somewhat smaller area. A Routine just reads a name and a some number of Exercises.

With those in place, initializing a map of routines from a file can look something like this:

std::ifstream in("routines.csv");

std::map<std::string, std::vector<exercise>> routines {
    std::istream_iterator<routine>(in), {} };

Of course, if you really want to define your own complete class for Routine, you can do that--but I'd still keep the responsibility for reading an Exercise in an extractor specifically for an Exercise.

\$\endgroup\$
0
\$\begingroup\$
  1. Avoid owning raw pointers. Even without exceptions, they are prone to leak.
    Use a sensible std::unique_ptr, std::any or std::variant. The latter even avoid extra dynamic allocations, though at the cost of potentially wasting a small amount of memory.

  2. Are you sure there's always at least one person?

\$\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.