Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

My homework question states:

Develop a class to measure distance as feet (should be int), inches (should be float). Include member functions to set and get attributes. Include constructors. Develop functions to add two distances.

The prototype of the sum function is:

Distance Sum(Distance d);

Please check out my coding style and other aspects that can make this program better.

#include <iostream>
using namespace std;
class Distance
{
        private:
                int feet;
                float inch;
        public:
        Distance();
        Distance(int a,float b);
        void setDistance();
        int getFeet();
        float getInch();
        void distanceSum(Distance d);
};
int main()
{
        Distance D1,D2;
        D1.setDistance();
        D2.setDistance();
        D1.distanceSum(D2);
        return 0;
}
/*Function Definitions*/
Distance::Distance()
{
        inch=feet=0;
}
Distance::Distance(int a,float b)
{
        feet=a;
        inch=b; 
}
void Distance::setDistance()
{
        cout<<"Enter distance in feet";
        cin>>feet;
        cout<<endl<<"Enter inches:";
        cin>>inch;
}
int Distance::getFeet()
{
        return feet;
}
float Distance::getInch()
{
        return inch;
}
void Distance::distanceSum(Distance d)
{
        cout<<"feet="<<d.feet+feet<<endl;
        cout<<"inches="<<d.inch+inch<<endl;
}
share|improve this question
    
Whitespace is cheap nowadays: don't be afraid to use it. –  pmg Apr 15 '11 at 20:28

6 Answers 6

up vote 25 down vote accepted

When designing a class for your program, whether it's for homework or a real production application, you want to always consider how that class is going to be use and what its responsibilities should be. Each function and method should do one thing/task and it should be reflected by the method's name. Additionally, you want to keep to a consistent easy-to-read coding style when you actually start writing the code. With those two points in mind here's some things to consider in your code:

  • Distance::distanceSum is doing two tasks here.

It's not only summing feet and inches but it's printing them out as well.

  • Unnecessary type conversion.

In your default Distance() ctor, there's an implicit conversion because you're assigning 0 to a float type. You should have gotten a warning from your compiler. Consider using an initializer list like so:

Distance::Distance() : feet(0), inch(0.0)
{
}
  • Code doesn't take advantage of const correctness.

Which parameters aren't suppose to change? Which methods will be modifying your Distance class? For example, your Distance::Distance(int a,float b) isn't changing a or b. Have the compiler enforce that promise by using const:

Distance::Distance(const int a, const float b)

Similiarly:

void Distance::distanceSum(const Distance &d);
  • Inconsistent indentation and spacing.

Consider indenting the methods under public: the same way you did with private:. Add spaces to your assignments to help readability. eg. feet = a;

  • No module separation by file.

class Distance should probably be in a separate header/implementation file rather than putting everything in one main file.

  • Ninja comments.

Comments? What comments? Exactly. Consider adding a block comment at the top of your Distance class that explains the purpose for its existence. The block comment should answer questions like how is this class suppose to be used and what details is it abstracting away? Adding a comment to clarify how the feet and inch data members are going to be used. For example, it's not clear if your distance class is maintaining the same distance measurement but with different units or it's really meant to be used as one whole unit. eg. 6 feet 2 inches or 6 feet 72 inches?

With the above considerations, here's one way I would refactor your code:

In distance.h header file:

#ifndef DISTANCE_H
#define DISTANCE_H
class Distance
{
    private:
       // feet and inch is one unit. 
       // invariant: inch_ < 12.
       int   feet_;
       float inch_;
    public:
       Distance(const int feet = 0, const float inches = 0.0);
       void      setDistance(const int feet, const float inches = 0.0);
       int       getFeet() const;
       float     getInch() const;

       // returns this instance. Permits method chaining for Distance class.
       Distance& Add(const Distance &d);
};
#endif

In distance.cpp implementation:

#include "distance.h"
Distance::Distance(const int feet, const float inches) 
         : feet_(feet + inches / 12), inch_(inches % 12)
{
}

void Distance::setDistance(const int feet, const float inches)
{
    feet_ = feet + inches / 12;
    inch_ = inches % 12;
}

int Distance::getFeet() const
{
    return feet_;
}

float Distance::getInch() const
{
    return inch_;
}

Distance& Distance::Add(const Distance &d)
{
    setDistance(getFeet() + d.getFeet(), getInch() + d.getInch());

    return *this;
}

Here are the major changes above:

  • Distance no longer uses cin/cout for explicit IO. You can push that code into main.
  • class definition and implementation are now in their respectively named files.
  • Removed an extra constructor definition by taking advantage of default parameters.
  • feet and inch data members are used together to represent the measurement in distance. inch cannot be > 12 because that would mean there's enough for a foot. We enforce this by dividing feet and inches by 12 when setting distance's data.
  • const is used to clearly indicate what can and can't change the distance object.
  • Changed distanceSum to Add to better reflect what it's doing. Notice that Add is implemented only through Distance's public methods -- it does not manipulate feet_ and inch_ directly.
share|improve this answer
    
Why an underscore in feet?(feet_) –  Fahad Uddin Jan 31 '11 at 8:19
1  
@fahad that is just a coding convention I'm using, appending _ for class data members. You can certainly choose a different convention that suits you. Whatever you choose to go with just be consistent. –  greatwolf Jan 31 '11 at 8:23
2  
I don't think that unmodified function arguments need to be const- I would only mark the functions themselves as const. –  DeadMG Jan 31 '11 at 9:48
    
Yes I had the same doubt!When passed by value,you dont need to keep it const –  Fahad Uddin Jan 31 '11 at 12:50
1  
const float inches = 0.0 should be 0.0f else there is an implicit conversion going on to float from double –  John Dibling Jan 31 '11 at 20:40

A bunch of these functions should be operators. You also seem to be confused about the purpose of addition. Also, using namespace std is bad. You also didn't provide any other operators- even though any class user will logically expect that if you can add a distance, you can add a distance to the current object, and that you can subtract distances too. You also didn't provide any kind of input verification or const correctness.

class Distance
{
    int feet;
    float inch;
public:
    // Constructors
    Distance();
    Distance(int a, float b);

    // Getters
    int getFeet() const;
    float getInch() const;

    // Operator overloads - arithmetic
    Distance operator-(const Distance&) const;
    Distance& operator-=(const Distance&);
    Distance operator+(const Distance&) const;
    Distance& operator+=(const Distance&);

    // Create from console
    static Distance getDistanceFromConsole();
};
// Operator overloads - I/O
std::ostream& operator<<(std::ostream&, const Distance&);

And the implementations are...

Distance::Distance() : feet(0), inch(0.0f) {}
Distance::Distance(int argfeet, float arginch) : feet(argfeet), inch(arginch) {
    // Verify that we are actually in feet and inches.
    // Not gonna write this code- dependent on the class invariants
    // which were not explicitly specified (e.g., can have negative Distance?)
}
int Distance::getFeet() const {
    return feet;
}
float Distance::getInch() const {
    return inch;
}
Distance Distance::operator-(const Distance& dist) const {
    Distance retval(*this);
    retval -= dist;
    return retval;
}
Distance& Distance::operator-=(const Distance& dist) {
    feet -= dist.feet;
    inches -= dist.inches;
    // Verify values- e.g. that inches is less than 12
    return *this;
}
Distance operator+(const Distance& dist) const {
    Distance retval(*this);
    retval += dist;
    return retval;
}
Distance& operator+=(const Distance& dist) {
    feet += dist.feet;
    inches += dist.inches;
    // More verification here.
}

std::ostream& operator<<(std::ostream& output, const Distance& dist) {
    output << "Feet: " << dist.feet << "\n";
    output << "Inches: " << dist.inches << std::endl; // flush when done.
    return output;
}

Distance getDistanceFromConsole() {
    int feet; float inch;
    std::cout<<"Enter distance in feet";
    std::cin>>feet;
    std::cout<<endl<<"Enter inches:";
    std::cin>>inch;
    return Distance(feet, inch);
}
share|improve this answer

There's no reason to have both inches and feet since the one can be calculated from the other. Superfluous redundancy just adds complexity.

share|improve this answer
2  
Why this is not the top comment is beyond me. Though there are lots of possible improvements like mentioned in Vicors good and elaborate answer, this hits the nail on the head. Whatever you do, fix this first (and stuff will become much simpler) –  Martijn Feb 5 '11 at 0:18
    
Perhaps because the very first sentence of the assignment stated that the class should model feet and inches separately. I learned long ago to do what the assignment asks unless I know for sure that the grader will accept optimizations. –  David Harkness Feb 15 '11 at 8:56
    
@David - the text quoted as the homework assignment doesn't say that. –  anon Feb 15 '11 at 19:31
    
@Crazy Eddie - The first sentence specifically states the data types of the two attributes: "Develop a class to measure distance as feet(should be int),inch(should be float)." The second sentence implies at least two attributes by using the plural form: "Include member functions to set and get attributes." I don't know what other attributes it would be talking about. –  David Harkness Feb 15 '11 at 21:33

C++ comments:

Prefer initializer lists:

Distance::Distance()
    :feet(0), inch(0)
{
        // NOT THIS -> inch=feet=0;
}

General coding comments:

Don't combine functionality:
You should define a function to add Distance objects and one to print them. (DeadMG has that covered above).

void Distance::distanceSum(Distance d)
{
        cout<<"feet="<<d.feet+feet<<endl;
        cout<<"inches="<<d.inch+inch<<endl;
}

Also I see you don't normalize your results. You could have 1 foot 300.05 inches. When ever the state of an object changes where one parameter flows into another you should normalize the data. Have an explicit function to do so and call it each time state changes:

private: void Normalize() { int eFeet = inches / 12; feet += eFeet; inches -= (eFeet * 12);}

Implementation Comments:

But then again why do you store what is actually a single value in two different variables (feet and inches). Why not just store the total distance in inches (and then do conversion on the way in/out). Look at the unix time. It just counts the seconds since the epoch. All other values are calculated from this.

share|improve this answer

I also prefer to write public methods first, because when you review big classes you must scroll to see public members.

For example:

class MyClass
{
public:
// public members
protected:
// protected members
private:
// private members
}
share|improve this answer

You could have written implementation of methods in the class itself where the template of methods are defined. Its generally a good practice to write all those methods in the class. In your case even if you have written the methods out side the class, C++ compiler will make then inline at the time of compilation.So it is better to write those methods as inline methods. It will make the execution bit faster and will remove the use of scope resolution operator which is seemingly difficult. Below is the example how to write..It contains just one method but you can do the same for rest of the methods.

#include <iostream>
using namespace std;
class Distance
{
    private:
            int feet;
            float inch;
    public:
    Distance()
    {
        inch=feet=0;
    }
    Distance(int a,float b);
    void setDistance();
    int getFeet();
    float getInch();
    void distanceSum(Distance d);
};
int main()
{
    Distance D1,D2;
    D1.setDistance();
    D2.setDistance();
    D1.distanceSum(D2);
    return 0;
}

This is the better way. All the class methods should be defined in the class only. Suppose are to develop just a class in java, then you can not develope a good class by writing like this.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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