2
\$\begingroup\$

We are thinking about hiring more developers so I was asked to compose a test. I have been looking online for ideas. I found this question: How to add n days to a Date in java without importing Date/Calendar from java API?. I have attempted this myself as follows:

public class Date : IEquatable<Date>
    {
        private readonly static int[] _daysInMonth = { 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 };
        private readonly int _month;
        private readonly int _day;
        private readonly int _year;

        public Date(int day, int month, int year)
        {
            this._day = day;
            this._month = month;
            this._year = year;
        }

        //static because thinking about using in constructor for validation.
        public static int MaxDays(int month, int year)
        {
            int maxDays = _daysInMonth[month - 1];
            if (month == 2)
            {
                if ((year % 400 == 0) || ((year % 4 == 0) && (year % 100 != 0)))
                    maxDays = maxDays + 1;
            }
            return maxDays;
        }

        public Date AddDays(int daysToAdd)
        {
            int day = this._day + daysToAdd;
            int month = this._month;
            int year = this._year;
            while (day > MaxDays(month,year))
            {
                day = day - MaxDays(month,year);
                month++;
                if (month > 12)
                {
                    year++;
                    month = 1;
                }
            }
            return new Date(day, month, year);
        }

        public override bool Equals(object obj)
        {
            return Equals(obj as Date);
        }

        public override int GetHashCode()
        {
            unchecked
            {
                int hash = 17;
                hash = hash * 23 + _day.GetHashCode();
                hash = hash * 23 + _month.GetHashCode();
                hash = hash * 23 + _year.GetHashCode();
                return hash;
            }
        }

        public static bool operator ==(Date date1, Date date2)
        {
            if (ReferenceEquals(date1, null) && ReferenceEquals(date2, null))
                return true;
            if (ReferenceEquals(date1, null) || ReferenceEquals(date2, null))
                return false;
            return date1.Equals(date2);
        }

        public static bool operator !=(Date date1, Date date2)
        {
            return !(date1 == date2);
        }

        public bool Equals(Date other)
        {
            if (ReferenceEquals(other, null))
                return false;
            return _day == other._day && _month == other._month && _year == other._year;
        }
    }

I would be grateful for comments. I reviewed the terms of posting here: https://codereview.stackexchange.com/help/on-topic. This is real code (no stubs) that works and therefore I believe it is on topic.

\$\endgroup\$
  • 4
    \$\begingroup\$ If I were hiring developers, which I have been known to do every few years, I would choose someone who uses what is already tried, true, and tested in the framework, rather than pay them to take a lot of time reinventing the wheel. \$\endgroup\$ – Rick Davin Feb 27 '18 at 13:08
  • 2
    \$\begingroup\$ @RickDavin indeed, this class is more than unnecessary. Also the entire implementation could be based on the DateTime. \$\endgroup\$ – t3chb0t Feb 27 '18 at 13:14
  • \$\begingroup\$ @Rick Davin, I agree. Do you think the questioner in my linked question was asked to "reinvent the wheel" or do you think they were expected to use DateTime? \$\endgroup\$ – w0051977 Feb 27 '18 at 13:47
  • 1
    \$\begingroup\$ While the original questioner was trying to reinvent the wheel, you should forget about that. Your goal is to hire developers, and an intermediate task for you to do that is to compose a competency test. I would suggest the one posted here is not the right test and that you should keep looking. If I were hiring a .NET developer, I want to make sure they know .NET. If I were interviewing, I'd give bonus points to the candidate who asks what I did not use DateTime or possibly NodaTime. \$\endgroup\$ – Rick Davin Feb 27 '18 at 14:38
  • \$\begingroup\$ I tried to code something similar. I think an Int16 based date would be a more interesting question. Here you are 3 Int32. \$\endgroup\$ – paparazzo Feb 28 '18 at 7:46
3
\$\begingroup\$

I see the point of the commenters: why implement this, if it has already been implemented? However, I still see benefit for such a question in an interview process. Given the right analysis, you can see how the developer thinks and what question he asks, which is IMHO more important than the actual code.

Questions could be:

  • What coding guidelines shall I follow?
  • Besides that single line description, do you have acceptance criteria?
  • Which unit test framework shall I use?
  • ...

public class Date : IEquatable<Date>

Such low level classes use to be sealed. Could apply here as well.


public static int MaxDays(int month, int year)

Why make it public? The purpose of the class is to add days. There's nothing in the task that qualifies this method to be part of the API, IMHO. Make it private instead.


The class hardly useful, because one cannot access the properties of the changed date. There are no accessors for _month, _day and _year.


Comments: do you expect XML-doc comments on public (API) methods? You don't have them.


// [...] thinking about using in constructor for validation.

What would you do if it's invalid? Throw an exception? Well, Microsoft does it ...


Magic number:

if (month == 2)

Introduce a constant February to give it semantics.


I write code in some languages and operator precedence is always an issue. The line

if ((year % 400 == 0) || ((year % 4 == 0) && (year % 100 != 0)))

is hard to understand. A method IsLeapYear() might help you out of that:

private static IsLeapYear(int year)
{
    if (year % 400 == 0) return true;
    if (year % 100 == 0) return false;
    if (year % 4 == 0) return true;
    return false;
}

At the same time it gives sematics to the magic numbers.


Why do we have AddDays() but not SubtractDays()? Can I pass a negative number for subtracting, or is subtracting not allowed?

Your code does not seem to support subtraction, but the data type is still int and not uint.


return new Date(day, month, year);

Do you expect immutability? Why? For performance reasons / multi threading? Is this premature optimization? Immutability comes at the cost of garbage collection.


Magic number:

if (month > 12)

What if the interviewed person uses _daysInMonth.Length? Is that better or worse than your implementation?


Hash codes etc.: do you provide an IDE which can generate equality members (like Jetbrains R#), or do you expect the developer to come up with that prime stuff by himself?

I have never written such code myself and actually I don't want to work in a company that expects me to write such code (except I'm applying to some security company which has to deal with hashes and encryption).

\$\endgroup\$
  • 1
    \$\begingroup\$ Based on this feedback, would you hire yourself? I sometimes wouldn't hire myself neither when I see the stupid bugs I've made :-) \$\endgroup\$ – Thomas Weller Aug 18 at 1:36

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.