6
\$\begingroup\$

I'm a C++ beginner and have made a simple class. But I'm not sure if this is well-written. It's basically just a Date class.

#include <iostream>

using namespace std;

class Date{
    int d, m, y;    //Day, Month, Year
public:
    Date(int dd = 1, int mm = 1, int yy = 1);

    int addDay(const int &dd) { d += dd; }
    int addMonth(const int &mm) { m += mm; }
    int addYear(const int &yy) { y += yy; }

    int day() const { return d; }
    int month() const { return m; }
    int year() const { return y; }

    void display() const;   //Print to screen

};

void Date::display() const { 
    cout << d << "." << m << "." << y << endl;
}

Date::Date(int dd, int mm, int yy){
    d = dd;
    m = mm;
    y = yy;
}

For example, the arguments of addDay(const int &dd);. Is that good, or should it just be an integer without const and without reference?

\$\endgroup\$
10
\$\begingroup\$

For built-in types, there is no point in passing it as a const& - prefer a plain int (or a const int) instead.

Other comments:

  1. Your add- functions are said to return int even though you don't have a return statement. I don't think that would or should even compile. Change them to void.
  2. Take the time to write out day, month and year. The extra seconds in typing is nothing compared to the readability gain.
  3. Are you sure you want a default date?
  4. Consider overloading operator<< to allow printing your class.
  5. Consider changing your display() function into a to_string function instead, and have it return a std::string.
  6. Make your class easy to use correctly, and hard to use incorrectly. What if month is 12 and someone calls addMonth()? What is someone passes in June as month, but 31 as day? Is day day the day of the month, or day of the year? Adhere to the principle of least astonishment. Your class should behave the way users of the class expect it to.
  7. Don't pollute the global namespace with using namespace std.
  8. Split the code into a header and an implementation file. Even though you won't need it now, it's good practice. That makes point 7 even more crucial.

You need to make some judgments to design a Date class properly. I won't give you any further suggestions, because it is a good exercise to think about it. Revise your code, and think about the pros and cons of various approaches. Apart from that and the points above, your code looks OK.

\$\endgroup\$
  • \$\begingroup\$ That was fast. :-) I'll try not to repeat anything you've mentioned (if I find anything else). \$\endgroup\$ – Jamal Jul 10 '13 at 18:00
  • \$\begingroup\$ @Jamal I was just lucky, I guess :-) \$\endgroup\$ – Lstor Jul 10 '13 at 18:02
  • \$\begingroup\$ Indeed. I managed to answer one question alone yesterday, so that's good enough for me. \$\endgroup\$ – Jamal Jul 10 '13 at 18:04
  • 1
    \$\begingroup\$ @NormalPeopleScareMe: I'm updating my answer regarding defaults. As for the other question, std::string is preferred in C++. \$\endgroup\$ – Jamal Jul 10 '13 at 18:19
  • 1
    \$\begingroup\$ @NormalPeopleScareMe Always use const when possible. Use references when it's the right thing to do. Sometimes you want a copy, for example -- then you shouldn't use references. For types that are similar or smaller in size to a pointer, use pass-by-value. \$\endgroup\$ – Lstor Jul 11 '13 at 15:57
4
\$\begingroup\$

@Lstor made many great points. I'll add a few things:

  • About using namespace std.

  • Alongside @Lstor's point regarding writing out your data members, I would recommend initializing them on separate lines. Also, when you do rename your members as such, your accessors should be renamed as well. Otherwise, they will end up having the same name, which will cause errors.

  • Pet-peeve here: using int when dealing with only positives. I would prefer unsigned int instead. You could even typedef them, but that's up to you.

  • Your constructor should have an initializer list instead. So, this:

    Date::Date(int dd, int mm, int yy){
        d = dd;
        m = mm;
        y = yy;
    }
    

    would look like this:

    Date::Date(int dd, int mm, int yy) : d(dd), m(mm), y(yy) {}
    
  • If you still want a default date, you would need a default constructor as well. The one above allows the user to set his/her own date. This one also has an initializer list, but without arguments:

    // the year can default to anything, but I chose 2000 for my example
    Date::Date() : d(1), m(1), y(2000) {}
    
\$\endgroup\$
  • \$\begingroup\$ So the default constructor would be the constructor from your example which I'll add to the existing one? \$\endgroup\$ – Normal People Scare Me Jul 10 '13 at 18:33
  • \$\begingroup\$ @NormalPeopleScareMe: yes, an additional one. You can have multiple constructors as long as their arguments (or lack of arguments) vary. \$\endgroup\$ – Jamal Jul 10 '13 at 18:35
  • \$\begingroup\$ You can have a constructor with default arguments and an initializer list, too. Date::Date(int d = 1) : day(d). \$\endgroup\$ – Lstor Jul 10 '13 at 18:48
  • \$\begingroup\$ @Lstor: Right. Is that preferred over what I mentioned, or does it not matter? \$\endgroup\$ – Jamal Jul 10 '13 at 18:51
  • \$\begingroup\$ It depends. When you have a default value that makes sense, use defaults. If you don't, you should probably just do plain overloading, or maybe only keep the version that takes arguments. In any case, don't use overloading to simulate defaults, and vice versa. Java opted to drop default arguments altogether, whereas for example C# and Python also have them. Item 24 of Effective C++ (2nd Ed.) discusses the issue further. \$\endgroup\$ – Lstor Jul 10 '13 at 18:59
3
\$\begingroup\$

If you're interested in bugs/missing functionality in addition to design issues, your addDay(), etc methods need to handle overflows. ex

Date foo = new Date(10, 7, 2013);
foo.addDay(30); //currently sets foo to July 40th, 2013.

Date bar = new Date(10, 7, 2013);
bar.addDay(-30); //currently sets bar to July -20th, 2013.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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