Capturing temperature information for a patient

Question

I just finished writing a program that captures temperature information for a patient. I'm looking for suggestions on how to improve the code that I have. Although what I have compiles and does what I want, I wonder if there is any methodology I'm using inappropriately despite it still working.

Some particular areas I worry about are redundant code and my use of the conversion and fever methods (you can ignore the clearly bad math. I just wanted any calculation until I determined functionality).

Temperature.hpp:

#ifndef Temperature_h
#define Temperature_h

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

class Temperature {
private:
    string newPatient;
    float newTemp;
    char newDegree;

public:
    enum TempMethod {oral, arm, leg, bum};

    Temperature();
    Temperature(string, float, char);
    ~Temperature();

    string getPatient() const;
    float getTemp() const;
    char getDegree() const;

    float setFaren();
    float setCels();
    void hasFever();



};


#endif /* Temperature_h */

Temperature.cpp:

Temperature::Temperature(string patient, float temp, char degree){
    newPatient = patient;
    newTemp = temp;
    newDegree = degree;
}


Temperature::~Temperature(){

}


string Temperature::getPatient() const{
    return newPatient;
}

float Temperature::getTemp() const{
    return newTemp;
}

char Temperature::getDegree() const{
    return newDegree;
}

float Temperature::setFaren(){
    float faren = newTemp+32;
    return faren;
}

float Temperature::setCels(){
    float celsi = newTemp-32;
    return celsi;
}

void Temperature::hasFever(){
    if(newTemp >= 100.4){
        cout << "Patient " << newPatient << " has a fever!!\n";
    }
}

main.cpp:

#include <iostream>
#include <string>
#include "Temperature.h"
using namespace std;


int main() {
    string patient;
    float temp;
    float celsius;
    float farenheit;
    char degree;

    cout << "Enter Patient Name: ";
    getline(cin, patient);
    cout << "Enter the tempature: ";
    cin >> temp;
    cout << "Enter degree method (F or C): ";
    cin >> degree;



    Temperature Temp_1(patient, temp, degree);
    cout << "\nPatient Name: " << Temp_1.getPatient() << endl <<
    "Temperature: " << Temp_1.getTemp() << " degrees " << Temp_1.getDegree() << endl;

    if(Temp_1.getDegree() == 'F'){
        celsius = Temp_1.setCels();
        cout << "Temperate in Celsius: " << celsius << " degrees Celsius" << endl;
    }
    else if(Temp_1.getDegree() == 'C'){
        farenheit = Temp_1.setFaren();
        cout << "Temperature in Farenheit: " << farenheit << " degrees Farenheit" << endl;
    }

    Temp_1.hasFever();

    return 0;
}

Answer 1

Technicalities

Your destructor is the same as the default destructor -- I wouldn't bother defining one. It makes me think that the class is going to actually do something in there, so it's strange when it doesn't.

---

Your header doesn't use iostream, only your implementation does. This means that the #include should be in the cpp file. Try to keep headers as tightly included as possible.

---

You're not mutating the patient variable so there's no need to make a copy. Instead, you should take it by const reference.

Temperature(const std::string& patient, float temp, char degree);

---

Instead of assigning your parameters, you should use an initializer list. Assinging means that they might be default constructed first then assigned, and this is unnecessary.

Temperature::Temperature(const std::string& patient, float temp, char degree)
  : patient(patient), temp(temp), degree(degree)
{ }

---

You did a good job of making your getters const, but getFahrenheit, getCelsius and hasFever should all also be const (they should also be renamed as this implies -- see the Style section).

---

You shouldn't use using namespace std in header files.

---

For little toy programs like this, it doesn't matter much, but it can be a good habit to always remember that any kind of IO can fail. In particular, if you want to indulge in some paranoia, you should check that your cin extraction was successful. After all, what if someone puts in a non-sense value when you're expecting a float. You don't particularly want your program to continue after that.

Style

float Temperature::setFaren(){
    float faren = newTemp+32;
    return faren;
}

This isn't setting anything. This is a getter. It should be named getFaren, or even better, getFahrenheit. To get a bit cliche-y for a second, code is read much more than it's written. Strive for clarity even if that means typing a few extra characters.

Also, the variable is a bit superfluous. I'd go with a simple return newTemp + 32;.

---

#ifndef Temperature_h

Include guards are traditionally SCREAMING_SNAKE_CASE:

#ifndef TEMPERATURE_H

---

Temperature(string, float, char);

This is confusing as a consumer of the class. You should give these parameters names:

Temperature(string patient, float temperature, char degree);

---

You have some non-optimal namings. For example, newPatient, newTemp, and newDegree don't really make sense. Why are they new? Just call them patient, temp and degree. If you're worried about them clashing with the local variables in the constructor, you can just use this->patient. I would also consider calling patient patientName since patient makes me expect some kind of model rather than a simple string.

I also am not a fan of the parameter name temp. That makes me assume it's some kind of temporary variable. temperature is clearer.

degree also unfortunately has issues. Fahrenheit and Celsius aren't degrees. They're scales of temperatures. This parameter could instead be named scale.

---

void Temperature::hasFever(){
    if(newTemp >= 100.4){
        cout << "Patient " << newPatient << " has a fever!!\n";
    }
}

This shouldn't output, but should instead return a bool. The consumer of the class should decide what to do with this information. What if you had a list of Temperatures and you wanted to count how many instances were considered to have a fever. You would likely not want this to output each time a fever was found. Instead, you want the method to return a bool that you can then decide what you want to do with it.

Also, you need to be aware of what scale you're using here. The threshold for a fever in Celsius is much different than the threshold in Fahrenheit.

---

enum TempMethod {oral, arm, leg, bum};

As this seems to be unused, I would remove it.

---

If this code were going to live longer than 1 homework assignment or practice, I would expect the class and its methods to be documented.

Design

I would decide on a canonical scale and use that internally. Then, your other scale(s) would simply convert in their getters. For example, you could always store Celsius and just convert to Fahrenheit when needed (this would of course mean that your constructor would need to convert to Celcius when Fahrenheit is provided).

---

I would consider using an enum for your scale instead of a char. An enum would mean that a user could only provide correct values. It also more explicitly signals what is supported. For example, someone might try to pass in K as your code currently is, then your conversion methods might not support that. Even worse, someone could currently pass in some non-sense like a.

If you do keep it a char, make sure to validate that it's one of the expected values.

---

It's strange for Temperature to have a patient name. It means that you're not really modeling a temperature; you're modeling a patient/temperature combination. In other words, you've coupled your Temperature object to your patient. What if you wanted to model the temperature of a room? Or of a bucket of water? There's nothing about a temperature that inherently has a patient name.

You should likely have some form of a Patient model that has a temperature (or something even more complex than this in the real world). More pragmatically though, this is a completely reasonable way to do it as long as you're aware that you've made this decision and the program isn't going to grow much. Do keep in mind though that this is a very real tradeoff, and if this program were to ever grow in complexity, you'd likely want to decouple these.

Answer 2

Your interface is BAD

float Temperature::setFaren(){
    float faren = newTemp+32;
    return faren;
}

float Temperature::setCels(){
    float celsi = newTemp-32;
    return celsi;
}

So I can call two different functions and get different results. I see from this interface I am supposed to interrogate the getDegree() and then choose which function to call? Also those are not set functions they get a result without changing the state of the object.

You should store your temperature in a single unit (Lets pick Kelvin or any one you like). Then you can compare objects of type temperature quite easily (as they will be on the same scale). When creating a temprature object you can convert the input numbers to Kelvin for starage and convert them to the appropriate form when retrieved.

enum TempType {Celsius, Fahrenheit, Kelvin};

class Temperature
{
    float  kelvin;
    public:
        Temperature(TempType type, float value)
        {
            switch(type) {
                case Celsius:    kelvin = convertC2K(value);break;
                case Fahrenheit: kelvin = convertF2K(value);break;
                case Kelvin:     kelvin = value;            break;
                default:
                    throw UnknownType;
        }
        // conversion operators are const members
        float getC() const {return convertK2C(kelvin);}
        float getF() const {return convertK2F(kelvin);}
        float getK() const {return kelvin;}

        // No need for setters.

        // Comparators may be useful
        bool operator==(Temperature const& rhs) const {return kelvin == rhs.kelvin;}
        bool operator< (Temperature const& rhs) const {return kelvin <  rhs.kelvin;}
};

I see no need to make the temperature object mutable. If I wanted to replace it I create another object and assign it over the old value.

Patient Data.

A temperature does not have a patient. A patient may have a temperature or lots of data that includes a temperature. Nor does a temperature have a fever (this is something a patient has).

 class PatientData
 {
     std::string name;
     Temperature temp;
     Weight      weight;
     Height      height;

     public:
         PatientData(std::string const& name, Temperature const& temp, Weight const& weight, Height const& height)
             : name(name)
             , temp(temp)
             , weight(weight)
             , heigth(temp)
         {}
         bool hasFever() const {return temp.getF() > 100.4;}

         // Need to be able to order Patients to put them in a set.
         bool operator<(PatientData const& rhs) const {return name < lhs.name;} // assumes all patients have a unique name.
};

std::set<PatientData> hospitalPatients;
    

Answer 3

I see some things that may help you improve your code.

Be consistent with file names

The question as posted calls the header file Temperature.hpp but main() includes Temperature.h. I'd recommend the latter but either way is technically fine as long as it's consistently applied.

Let the compiler generate code where it can

The destructor for Temperature doesn't do anything. The compiler can just as easily supply that for you, so I'd recommend allowing it to do so. Likewise the constructor with no arguments doesn't even exist, so I'd recommend deleting both.

Prefer initializer lists to constructor body

Right now your constructor looks like this:

Temperature::Temperature(string patient, float temp, char degree){
    newPatient = patient;
    newTemp = temp;
    newDegree = degree;
}

However, I'd recommend using an initializer list instead like this:

Temperature::Temperature(string patient, float temp, char degree) :
    newPatient{patient},
    newTemp{temp},
    newDegree{degree}
{
}

This particular style also uses {} style direct initialization, so requires C++11, but one could use the older () style if needed with an old compiler.

Rethink your object design

Rather than having two possible ways to store the temperature, what would make more sense would be to have only one way to store it (say F or C or even K), and then multiple ways to extract and convert values if needed. For example, if the internal representation were C, then you could have an interface like this:

float Temperature::getF() const { return newTemp*9/5 + 32 };
float Temperature::getC() const { return newTemp; }

Eliminate "magic numbers"

Rather than having the "magic value" of 100.4 in hasFever(), it would instead be better to make it a named constant:

constexpr static float Temperature::FeverValue{38.0};

Isolate I/O

Rather than having hasFever() be a void function that happens to print, it would be better to make it a bool function and leave the printing external. One might code it like this:

bool Temperature::hasFever() const {
    return newTemp >= FeverValue;
}

Include your header in the implementation

Right now the Temperature.cpp file does not include the Temperature.h file, but it should.

Eliminate unused things

The TempMethod enum is never used and should be omitted until actually needed and used.

Fix the typo

The word "temperature" is not spelled correctly in main(). This doesn't affect the program much, but could affect the impression that the user gets of the program. Typos in the user interface are particularly bad for that reason.

Perform better error checking for user input

As it is currently written, one can type in "Fred" as the temperature and the code attempts to process that data. Better would be to include more checking, including perhaps range checking, since it's unlikely that any (living) patient will have a temperature of 0 or 100C, for example.

Throw an error with invalid data

Right now the constructor gets a char to indicate whether the units are F or C but then doesn't actually check to make sure that it's one of those.

Use better names

Names like newTemp and newPatient and even the class name Temperature are not really very descriptive. I'd omit new from those variable names and maybe change the class name to PatientTemperature.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. It's not necessarily wrong to use it, but you should be aware of when not to (as when writing code that will be in a header).

Fix the conversion error

You should double check to see that your conversion formulas are correct. At the moment they are not.

Don't use std::endl when '\n' will do

Using std::endl emits a \n and flushes the stream. Unless you really need the stream flushed, you can improve the performance of the code by simply emitting '\n' instead of using the potentially more computationally costly std::endl.

Eliminate return 0

When a C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no reason to put return 0; explicitly at the end of main.

Putting it all together

Here's a revised program showing how most of these suggestions look in context:

PatientTemperature.h

#ifndef PatientTemperature_h
#define PatientTemperature_h

#include <string>

class PatientTemperature {
private:
    std::string name;
    float temperature;
    constexpr static float FeverValue{38.0};

public:
    PatientTemperature(std::string, float, char);
    float setValue(float, char);

    std::string getPatient() const { return name; }
    float getF() const { return temperature*9/5 + 32; }
    float getC() const { return temperature; }
    bool hasFever() const { return temperature >= FeverValue; }
};

#endif /* PatientTemperature_h */

PatientTemperature.cpp

#include "PatientTemperature.h"
#include <stdexcept>

PatientTemperature::PatientTemperature(std::string patient, float temp, char degree) :
    name{patient}
{
    setValue(temp, degree);
}

float PatientTemperature::setValue(float temp, char degree) {
    switch (degree) {
        case 'F':
            temp = (temp - 32) * 5.0/9.0;
        case 'C':
            temperature = temp;
            break;
        default:
            throw std::range_error("Temp must be either 'F' or 'C'");
    }
    return temperature;
}

main.cpp

#include "PatientTemperature.h"
#include <iostream>
#include <string>

int main() {
    std::string patient;
    float temp;
    char degree;

    std::cout << "Enter Patient Name: ";
    std::getline(std::cin, patient);
    std::cout << "Enter the temperature: ";
    std::cin >> temp;
    std::cout << "Enter degree method (F or C): ";
    std::cin >> degree;

    PatientTemperature Temp_1(patient, temp, degree);
    std::cout << "\nPatient Name: " << Temp_1.getPatient() 
        << "\nTemperature: " << Temp_1.getF() << " degrees F"
        << "\nTemperature: " << Temp_1.getC() << " degrees C" 
        << "\nThis patient " << (Temp_1.hasFever() ? "does" : "does not") 
        << " have a fever.\n";
}