Performing calculations for different variables

Question

I'm using the below code to try and make it easy to do sensible calculations for different variables, i.e. pressure + height is nonsensical, and pressure in atm += pressure in bar should add taking units into account. I've tried playing round with dynamic and static cast as well as CRTP on the advice of other fine members of the stack exchange community, but couldn't get those methods working. My code as is works but the + operator returns a Variable type not a Pressure type.

It's all in one file at the moment as I haven't messed with declaring template types in implementation files.

#ifndef PARENT_VARIABLE_H_
#define PARENT_VARIABLE_H_

#include <string>
#include <map>
#include <stdio.h>
#include <typeinfo>
#include <boost/assign.hpp>

template <typename V,typename D> // To let different variables use different S.F.
struct Variable{
    /*
     * This structure is a abstract base class from which all dimensional variables
     * are derived. Derived variables may be operated with and units will be largely
     * automatically handled.
     */

protected:
    //Functions
    Variable(V v, std::string u,std::string * m_type, D conversions [], const std::map<std::string,short> * units);
    //~Variable();
    bool check_type(const std::string & two) const;
    V consistant_units(const Variable & two) const;

    //Variables
    V value;
    std::string unit;
    const std::map<std::string, short> * valid_units; //contains all units for which conversions have been defined.
    D * conversion; //An n by n array for calculating conversions
    std::string * type;

public:
    V get_value() const;
    std::string get_unit() const;
    void change_unit(const std::string & new_unit);

    //Overloaded operators

    //Changes self
    Variable & operator+= (const Variable &t);

    //Returns new Variable type
    Variable & operator+(const Variable & other) const;

};

//////////////////////////////////////////////////////////////////////////////////////////////////////
/*
 * Class functions
 */

FILE * E_FILE;

void warning(const std::string & message){ //Placeholder function I'm not sure what I want to do with my warning messages yet
    E_FILE = fopen ("error_file.txt","w");
    std::cout<<message; // might change to output file
    fclose (E_FILE);
}

template<typename V,typename D>
Variable<V,D>::Variable(V v, std::string u, std::string * m_type, D conversions [], const std::map<std::string,short> * units){
    value = v;
    unit = u;
    conversion = conversions;
    valid_units = units;
    type = m_type;
}

template <typename V,typename D>
V Variable<V,D>::get_value() const{
        return value;
    }

template <typename V,typename D>
std::string Variable<V,D>::get_unit() const{
    return unit;
}

template <typename V,typename D>
void Variable<V,D>::change_unit(const std::string & new_unit){
    if (valid_units->find( new_unit ) == valid_units->end()){//Check the unit is defined
        std::string message = new_unit +" is not a valid unit. /n";
        warning(message);
    }
    else{
        int target = valid_units->find(new_unit)->second;
        int original = valid_units->find(unit)->second;
        value*=conversion[ (original*valid_units->size()) + target];
        unit=new_unit;
    }
}

template <typename V,typename D>
bool Variable<V,D>::check_type(const std::string & two) const{
    if(*type!=two){
        std::string message = two + " is not the same as "
                + *type + " adding them is nonsensical. /n";
        warning(message);
        return false;
    }
    return true;
}

template <typename V,typename D>
V Variable<V,D>::consistant_units(const Variable<V,D> & two) const{
    if (unit!=two.unit){
        std::string message = *type+
                " units different, answer is in "+ unit + " /n";
        warning(message);
        int target = valid_units->find(unit)->second;
        int original = valid_units->find(two.unit)->second;
        return (two.value)*conversion[ (original*valid_units->size()) + target];
    }
    return two.value;
}

template <typename V,typename D>
Variable<V,D> & Variable<V,D>::operator +=(const Variable<V,D> & t){
    if (check_type(*t.type)){
        V other_value = consistant_units(t);
        value += other_value;
    }
    return *this;
}

template <typename V,typename D>
Variable<V,D> & Variable<V,D>::operator+(const Variable<V,D> &other) const {
    return Variable(*this) += other;
  }
/////////////////////////////////////////////////////////////////////

///////////////////////////////////////////////////////////////////////////////////////////////////////
/*
 * Pressure
 */

template <typename V>
class Pressure : public Variable<V,double> {
public:
    Pressure(V v,std::string u);
    //~Pressure();
};

///////////////////////////////////////////////////////////////////////////////////////////////////////
/*
 * Pressure
 */
static const std::map<std::string, short> PRESSURE_UNITS =
    boost::assign::map_list_of("kPa",0)("atm",1)("psi",2);

static double PRESSURE_CONVERSION[3][3]= {{1,0.009869232667,0.145037738},
                                            {101.325,1,14.6959488},
                                            {6.89475729,0.0680459639,1}};

static std::string PRESSURE_TYPE = "Pressure";

template <typename V>
Pressure<V>::Pressure(V v, std::string u) : Variable<V,double>(v,u,&PRESSURE_TYPE,PRESSURE_CONVERSION[0],&PRESSURE_UNITS){
}

#endif /* PARENT_VARIABLE_H_ */

Answer 1

A first look at the Variable base class:

I assume this must always exist!
Thus you should make it a reference (Unless there is some way to change this dynamically. Given the current interface I can's see a way to change it).

const std::map<std::string, short> * valid_units; //contains all units for which conversions have been defined.

Why would you have a pointer to a string?

std::string * type;

It should either be an object or a reference. To me it looks like the type and the method for converting to another type is really all one object that the variable should hold a reference too as a single entity. How you hold the reference can be a C++ style reference or a C++ template traits type depending on how you see it working.

Have no idea how this works or how you are setting it up.
So obviously we are missing a lot of commenting.

D * conversion; //An n by n array for calculating conversions

Constructor comments!

Variable(V v, std::string u,std::string * m_type, D conversions [], const std::map<std::string,short> * units);

• V is passed by value. Since this is a template type I would rather it was passed by const reference this will avoid any nasty surprises in the future then a heavy type is used for V.
• u Again pass by const reference.
• m_type: Why are you passing a pointer (even if you store as a pointer internally you want to pass by reference (thus it can never be NULL)).
• D here D is a single dimension array. But above you say it is a N*N array. That's inconsistent.
• units again pass by reference (even if you store a pointer internally).

Desstructor

//~Variable();

Commented out? If this is a base class you better have a good reason for not makeing this virtual. And if there is a good reason it should be well documented here.

Checking the type! Why would you pass a string to check the type?

bool check_type(const std::string & two) const;

The only operation you can perform is to check if two Variable have the same type or not. Thus this function should take a const reference to another Variable object and you can return true/false if their types are same/different.

Have no idea what this is supposed to do. More documentation needed.

V consistant_units(const Variable & two) const;

Ok. That's obvious.

V value;

Not as obvious what this does:

std::string unit;

OK. Personal opinio: I hate Getters/Setters. Best way to break your objects encapsulation ever invented. Also it tightly couples your class to always providing this interface in the future as somebody will use it and then you will have to maintain it.

V get_value() const;

OK. I can see why you may want to get the value. But return by reference. Also you need to provide a const version that returns a const reference to the value. Unless you wanted to use the value as part of a copy constructor in which case you should implement the copy constructor.

std::string get_unit() const;

No reason to be able to get the unit!. You need it because of the way you implemented check_type(). But you are leaking your abstraction here. Prefer to change check_type(). The other reason to leak this is for streaming purposes, but a better solution would be to implement a print method.

OK. Self modification.

Variable & operator+= (const Variable &t);

But what reference are you returning here?

Variable & operator+(const Variable & other) const;

EDIT: (Based on extended question)

I'm not sure what you mean by "I assume this must always exist! Thus you should make it a reference."

Will it ever be NULL? If it is never NULL then pass by reference.

Why a pointer to a string; I want a single string, "Pressure", for all Pressure instances, then a different one, "Length" for all Length types.

The create your set of types somewhere. And pass references to these types to your variable.

My first instance was to use static inside the class but as I understand it this would cause both Pressure and Length to have the same string, type.

You could have a static in the class:

struct Variable
{
    /* STUFF */
    static std::string const  PresureType;
    static std::string const  LengthType;
};

Now in the constructor of the object you would pass one of these values (by reference).

Variable pressure(15, "Bar", PresureType, myConversions, myUnits);

If I was to use a reference wouldn't I have to assign it on deceleration. Is there something I'm missing here?

Not sure what you mean. You are already passing it to the cosntructor. There is no change.

I'm passing a string that I've already got from another Variable it's a function that's meant to be called solely from within the class but I can see why this is confusing/bad design.

Yes get rid of it. Methods are actions that can be performed on your object (any method that does not look like a verb is probably not really part of the class).

I thought the reference was to a copied instance of the lhs which has been += to the rhs. I was also under the impression using the default copy constructor was fine as only value and unit need to be unique. Is this wrong?

You are returning a reference to a local variable. That variable goes out of scope and will be destroyed at the end of the method. If you turn on your compiler warnings it will tell you about this.