What is wrong with this constructor definition?

Question

I want to write a class of "Clock", which basically holds hours, minutes and seconds (as integers).

I want that by default the constructor would initalize hours,minutes and seconds to 0 unless other input was given. So I declared the following:

    Clock(int hour=0, int minute=0, int second=0);

Which basically should give me 4 different constructors - the first one is the default constructor, and then a constructor which recieves hours as an input, a constructor which recievrs hours and minutes as an input, and a constructor which recieves all 3 as an input.

Next, I want the constructor to initalize the fields only if the input is valid, meaning, hours should be between 0 to 24, minutes and seconds should be between 0 to 60. So I wrote 3 functions that checks if the input is legal, and then I implemented the constructor as the following:

Clock::Clock(int hour , int minute, int second ){
    if (isHourLegal(hour) && isMinLegal(minute) && isSecLegal(second)){
        time[0] = hour; time[1] = minute; time[2] =second;
    }
    else
        Clock();
}

Where time[] is an array of length tree which holds the hours,minutes and seconds fields of the object.

Basially, if one of the inputs is wrong, Im calling Clock(); which should create an object with the default intefers I wrote when I declared.

Is there something wrong with the way I implemented the constructor? Im having unexpected outputs when I try to print an object fields.

Also,

Assume I fixed my constructor to:

Clock::Clock(int hour , int minute, int second ){
    if (isHourLegal(hour) && isMinLegal(minute) && isSecLegal(second)){
        time[0] = hour; time[1] = minute; time[2] =second;
    }
    else
        time[0] = 0; time[1] = 0; time[2] =0;
    }
}

Im trying to call the default constructor in my main function by :

Clock clock1();
clock1.printTime();

(printTime is a method which prints the fields).

But I get an error. Is there a porblem using the constructor this way?

Does this answer your question? [Can I call a constructor from another constructor (do constructor chaining) in C++?](https://stackoverflow.com/questions/308276/can-i-call-a-constructor-from-another-constructor-do-constructor-chaining-in-c) — John Kugelman, Dec 13 '21 at 17:09
`Clock();` creates and discards a new temporary object. It does not affect the object already being constructed, so your member variables are then uninitialized. — Fred Larson, Dec 13 '21 at 17:09
Per the linked question, you can only delegate to another constructor in the initializer list. This means it is an unconditional call. You can't gate it behind any logic. Your best bet is to just put the `0`s inline; there's no good way I can think of to avoid repeating the default values. (You can use a helper method, but I prefer not to go that route. If you do, it [must not be virtual](https://stackoverflow.com/questions/26464122/should-i-call-a-member-function-in-a-constructor).) — John Kugelman, Dec 13 '21 at 17:15
@JohnKugelman I updated my constructor. Can you tell why I cannot use the default one in the way Im trying to in the main function? — DirichletIsaPartyPooper, Dec 13 '21 at 17:17
Just write `Clock clock;`. The parentheses are unnecessary, and confuse the compiler. `Clock clock1();` is parsed as a function prototype rather than a variable definition. See: [Most vexing parse](https://en.wikipedia.org/wiki/Most_vexing_parse) — John Kugelman, Dec 13 '21 at 17:18
`Clock clock1();` should be `Clock clock1;`. With parentheses it's a declaration of a function named `clock1` that takes no arguments and returns an object of type `Clock`. Just like `int f();`. — Pete Becker, Dec 13 '21 at 17:19
@JohnKugelman -- the compiler isn't confused; it's doing exactly what it's supposed to do. And this **isn't** the most vexing parse. In the page you linked to, this is not even mentioned. It's simply the way you declare a function that takes no arguments. Sure, beginners are frustrated when they write this and it doesn't do what they expect, but that's easily overcome; it's not the least bit vexing. The examples on that page, however, are, indeed, vexing. — Pete Becker, Dec 13 '21 at 17:21
Further reading where it is mentioned: [The Most Vexing Parse: How to Spot It and Fix It Quickly](https://www.fluentcpp.com/2018/01/30/most-vexing-parse/) — John Kugelman, Dec 13 '21 at 17:23
@PeteBecker: How is this not the vexing parse? That's actually _the_ example of the vexing parse. Am I missing something? — andreee, Dec 13 '21 at 17:38
@andreee -- The underlying rule is that if it could be a function declaration, it is a function declaration. And, yes, `int f();` satisfies that rule. But the term "most vexing parse" was invented to describe more subtle things; the first example on the Wikipedia page is `void f(double my_dbl) { int i(int(my_dbl)); }`. If "most vexing parse" is read so broadly, so that it applies to every application of the underlying rule, it loses its meaning. As I said before, `int f();` is not in any meaningful sense "vexing". It's **the** way to declare a function that takes no arguments. — Pete Becker, Dec 13 '21 at 17:45
@Matt Why? when I write "Clock myClock;" in the main I do get an intialized object with the default values — DirichletIsaPartyPooper, Dec 13 '21 at 17:45
@Matt -- that's the **implementation** of the constructor, not the **declaration**. The implementation isn't allowed to repeat the default arguments from the declaration. — Pete Becker, Dec 13 '21 at 17:46
@PeteBecker good point. I've never seen it that way. Thanks for your clarification. — andreee, Dec 13 '21 at 17:47
@Matt it's not. If you look precisely, the "fixed" constructor is a separate definition `Clock::Clock(...)`, so the default arguments _must not_ go here (and only in the declaration). The OPs code code should be fine in that respect. — andreee, Dec 13 '21 at 17:50
True, I did a lazy copy/paste and that messed things up. I should have paid better attention. — Matt, Dec 13 '21 at 17:52

andreee · Accepted Answer · 2021-12-13T17:42:43.190

You can solve this easily.

If you have default arguments, chances are these will pass the test (you define them, after all :-)). So why don't take advantage of this and just write a single constructor that handles both cases?

In both cases (i.e. with/without passing arguments to it), the checks will be performed and on success a valid Clock will be constructed. For your default arguments, this will be trivially true. In the failure case, your constructor will throw and you won't be having a half-initialized/broken object roaming around.

Maybe the point which you are missing is: If the time format is not legal, you should usually throw.

Look at the following definition, which should be sufficient for your case.

// Declaration
Clock(int hour=0, int minute=0, int second=0);

// ...

// Definition
Clock::Clock(int hour, int minute, int second)
  : time { hour, minute, second } // assuming int time[3]; here
{
    if (!isHourLegal(hour) || !isMinLegal(minute) || !isSecLegal(second)){
        throw std::invalid_argument("Illegal time format!");
    }
}

What is wrong with this constructor definition?

1 Answers1