Properly declaring a function using class members?

Question

I'm in the process of trying to convert a C program using structs into C++ with classes. I am trying to call a member function of a class B from within class A. Here is a snippet to explain my error:

    class Time{
        public:
        Time();
        Time(int hour, int minute);
        int time_Equal(Time &a, Time &b) const;
        -----
        private:
        int hour;
        int minute;
    };

    Boolean Time::time_Equal(Time &a, Time &b) const
    {
        /* If hour of Time "a" equals hour of Time "b" */
        if(a.hour == b.hour){
            if(a.minute == b.minute)
                    return TRUE;
        }
    }

    class DaTime{
        public:
        DaTime();
        DaTime(Day day, Time start, Time end);
        Boolean dt_Equal(DaTime &a, DaTime &b) const;

        private:
        int duration;
        Time start;
        Time end;
    };

    Boolean DaTime::dt_Equal(DaTime &a, DaTime &b) const
    {
            if(time_Equal(a.start, b.start)){
                   if(time_Equal(b.end, a.end))
                          return TRUE;
            }

            else
                    return FALSE;        
    }

The error I am getting is that time_Equal is not declared in this scope. The reason I do not understand this is because I have included a header file where Time is declared before DaTime. I don't see the issue with how I have declared the functions.

`time_Equal` is a member of `Time`, nor `DaTime`. Also, what is `Boolean`? Is it the same as `int`? Your definitions have to match your declarations. — juanchopanza, Feb 23 '14 at 22:25
Boolean is previously defined. It's defined as `TRUE == 1` and `FALSE == 0`. — Allen S, Feb 23 '14 at 22:29
So `Boolean` is an enum ? Get rid of it, it offers literally zero benefit. Either use `int` or use `bool`. Pick one and stick with it. (preferably the latter). — WhozCraig, Feb 23 '14 at 22:33
If I change the `if(time_Equal(a.start, b.start))` portion to `if(start.time_Equal(a.start, b.start))` the compiler stops complaining. Why is that? That can't surely be a correct way of declaring the member to perform the function on. — Allen S, Feb 23 '14 at 22:33
@AllenS Because you made it a member of `Time`, so it has to be called on a `Time` object. In `start.time_equal(a.start, b.start)`, `start` is the object that `this` points at inside the function. In your implementation, however, you are not making use of the `*this` object, so the `start.` prefix seems unnecessary. That's just because you've implemented it wrong. — Joseph Mansfield, Feb 23 '14 at 22:36

score 3 · Accepted Answer · edited May 23 '17 at 12:20

First, note that the boolean type is just bool.

Second, the return type of the time_Equal declaration is int, whereas the definition has Boolean. Pretty sure you want both to be bool. They should at least match.

Third, your time_Equal function does not modify its arguments, so you should make them const references (const Time &a and const Time &b).

Fourth, your time_Equal function is a non-static member of Time, but it doesn't use any of its members. That is, its output depends only on its arguments, and not on the object that this points at. This is a code smell. It means your function should probably be a static function or you should change the function to use this. So you have two options:

Keep it a member and change it to:
```
bool time_Equal(const Time &other) const;
```
And then in the implementation you should compare *this with the other argument. In this case, you would call it like a.start.time_Equal(b.start).
Make it either a static member or take it out of the class and make it a friend function and keep it with two arguments:
```
bool time_Equal(const Time &a, const Time &b) const;
```
In this case, you would call it like time_Equal(a.start, b.start).

Fifth, C++ has a built in operator to perform test for equality (==) which you can overload for a class. So you can in fact implement the above two options respectively like so:

bool operator==(const Time &other) const; // As a non-static member
// or
bool operator==(const Time &a, const Time &b) const; // As a static member or friend function

Then you can compare times like so: a.start == b.start. Nice, huh?

For more information, see the operator overloading FAQ.

Sixth, your time_Equal implementation does not return false when it should. You can easily rewrite the body as:

return a.hour == b.hour && a.minute == b.minute;

Wow, thanks for such an elaborate explanation including alternative options. — Allen S, Feb 23 '14 at 22:45
@AllenS I should probably mention that the friend function `operator==` overload is usually preferred. — Joseph Mansfield, Feb 23 '14 at 22:46
We just now started learning about operator overloads in class, so I was a bit iffy on trying it. I will try to implement it that way once I get this all resolved. Which, thanks to you and everyone else's help I think I will be able. — Allen S, Feb 23 '14 at 23:04

Vlad from Moscow · Answer 2 · 2014-02-23T22:38:30.730

Function time_Equal is declared as a non-static member function of a class. So it can be used only with an instance of the class. Moreover it is declared as having parameters of type Time. However in function dt_Equal you call it passing arguments of type int

        if(time_Equal(a.start, b.start)){
               if(time_Equal(b.end, a.end))
                      return TRUE;
        }

You should declare function time_Equal either as a non-member function with two parameters or as member function with one parameter.

As a member function it could look as

bool Time::time_Equal( const Time &rhs ) const
{
    return (  hour == rhs.hour  && minute == rhs.minute );
}

As a non-member function it must be declared as a friend function of the class because data members hour and minute are defined as private Or if you have getters to hour and minute then there is no necessary to make it friend. In any case it could look as

bool Time::time_Equal( const Time &lhs, const Time &rhs )
{
    return (  lhs.hour == rhs.hour  && lhs.minute == rhs.minute );
    // or return (  lhs.get_hour() == rhs.get_hour()  && lhs.get_minute() == rhs.get_minute() );

}

score 0 · Answer 3 · answered Feb 23 '14 at 22:26

0

time_Equal is a method in the Time class, yet you call it like it is a global function. An instance level method shall be called on an object.

answered Feb 23 '14 at 22:26

mcserep

3,231
21
36

Properly declaring a function using class members?

3 Answers3