Timecode class design

Question

I've created a Timecode class. The "timecode" that I'm trying to represent is a timecode that is used in video editing quite often, and is seen displayed in the format "hh:mm:ss:ff", or 10:00:05:24, for example ("ff" = frames").

/// <summary>
    /// Immutable timecode.
    /// </summary>
    /// <remarks>
    /// Represents a timecode in hh:mm:ss:ff format.  Rolls over at the 24 hour mark.
    /// </remarks>
    [Serializable]
    public class Timecode
    {
        private static readonly Regex TimecodeRegex = new Regex(@"^(?<hours>\d{1,2}):(?<minutes>\d{1,2}):(?<seconds>\d{1,2}):(?<frames>\d{1,3})$", RegexOptions.Compiled | RegexOptions.ExplicitCapture);

        private int totalFrames;
        private short frameRate;

        public Timecode(int totalFrames, short frameRate = 25)
        {
            if (frameRate <= 0) throw new ArgumentOutOfRangeException("frameRate");

            this.totalFrames = totalFrames % FramesPerDay(frameRate);
            this.frameRate = frameRate;
        }

        public Timecode(int hours, int minutes, int seconds, int frames, short frameRate = 25)
        {
            int totalFrames = frames;
            totalFrames += seconds * frameRate;
            totalFrames += minutes * 60 * frameRate;
            totalFrames += hours * 60 * 60 * frameRate;

            this.totalFrames = totalFrames % FramesPerDay(frameRate);
            this.frameRate = frameRate;
        }

        /// <summary>
        /// The hours segment of the timecode.
        /// </summary>
        public int Hours
        {
            get
            {
                return (int)this.totalFrames / this.frameRate / 60 / 60;
            }
        }

        /// <summary>
        /// The minutes segment of the timecode.
        /// </summary>
        public int Minutes
        {
            get
            {
                return (int)this.totalFrames / this.frameRate / 60 % 60;
            }
        }

        /// <summary>
        /// The seconds segment of the timecode.
        /// </summary>
        public int Seconds
        {
            get
            {
                return (int)this.totalFrames / this.frameRate % 60;
            }
        }

        /// <summary>
        /// The frames segment of the timecode.
        /// </summary>
        public int Frames
        {
            get
            {
                return (int)this.totalFrames % this.frameRate;
            }
        }

        /// <summary>
        /// The total number of frames for this timecode.
        /// </summary>
        public int TotalFrames
        {
            get
            {
                return this.totalFrames;
            }
        }

        /// <summary>
        /// The framerate of this timecode.
        /// </summary>
        public short FrameRate
        {
            get
            {
                return this.frameRate;
            }
        }

        /// <summary>
        /// The total number of seconds in this timecode.
        /// </summary>
        /// <returns></returns>
        private float TotalSeconds()
        {
            return (float)this.totalFrames / (float)this.frameRate;
        }

        public override string ToString()
        {
            return string.Format("{0}:{1}:{2}:{3}",
                PadTimecodeUnit(Hours),
                PadTimecodeUnit(Minutes),
                PadTimecodeUnit(Seconds),
                PadTimecodeUnit(Frames));
        }

        /// <summary>
        /// Pads a number for display in a timecode string.
        /// </summary>
        /// <param name="unit"></param>
        /// <param name="places"></param>
        /// <returns></returns>
        private static string PadTimecodeUnit(int unit, int places = 2)
        {
            return unit.ToString().PadLeft(2, '0');
        }

        /// <summary>
        /// The total number of frames in one day, for this timecode's framerate.
        /// </summary>
        /// <returns></returns>
        private int OneDay()
        {
            return FramesPerDay(this.frameRate);
        }

        /// <summary>
        /// Parses a timecode string of the format "hh:mm:ss:ff".
        /// </summary>
        /// <param name="timecodeStr"></param>
        /// <param name="framerate"></param>
        /// <returns></returns>
        public static Timecode Parse(string timecodeStr, short framerate = 25)
        {
            GroupCollection captureGroups = TimecodeRegex.Match(timecodeStr).Groups;

            int hours = int.Parse(captureGroups["hours"].Value);
            int minutes = int.Parse(captureGroups["minutes"].Value);
            int seconds = int.Parse(captureGroups["seconds"].Value);
            int frames = int.Parse(captureGroups["frames"].Value);

            return new Timecode(hours, minutes, seconds, frames);
        }

        /// <summary>
        /// Adds two timecodes.
        /// </summary>
        /// <param name="timecodeA"></param>
        /// <param name="timecodeB"></param>
        /// <returns></returns>
        public static Timecode operator +(Timecode timecodeA, Timecode timecodeB)
        {
            if (timecodeA.frameRate != timecodeB.frameRate)
                throw new InvalidOperationException("Cannot add two timecodes with different framerates.");

            return new Timecode(timecodeA.totalFrames + timecodeB.totalFrames, timecodeA.frameRate);
        }

        /// <summary>
        /// Adds a given number of frames to the timecode.
        /// </summary>
        /// <param name="timecodeA"></param>
        /// <param name="frames"></param>
        /// <returns></returns>
        public static Timecode operator +(Timecode timecodeA, int frames)
        {
            return new Timecode(timecodeA.totalFrames + frames, timecodeA.frameRate);
        }

        /// <summary>
        /// Subtracts two timecodes.
        /// </summary>
        /// <param name="timecodeA"></param>
        /// <param name="timecodeB"></param>
        /// <returns></returns>
        /// <remarks>If the second timecode has fewer frames than the first, it is presumed to have rolled over the 24 hour mark and therefore be conceptually greater than the first.</remarks>
        public static Timecode operator -(Timecode timecodeA, Timecode timecodeB)
        {
            if (timecodeA.frameRate != timecodeB.frameRate)
                throw new InvalidOperationException("Cannot subtract two timecodes with different framerates.");

            int totalFramesA = timecodeA.totalFrames;
            int totalFramesB = timecodeB.totalFrames;

            if (totalFramesA < totalFramesB)
                totalFramesA += timecodeA.OneDay();

            return new Timecode(totalFramesA - totalFramesB, timecodeA.frameRate);
        }

        /// <summary>
        /// Subtracts frames from the timecode.
        /// </summary>
        /// <param name="timecodeA"></param>
        /// <param name="frames"></param>
        /// <returns></returns>
        public static Timecode operator -(Timecode timecodeA, int frames)
        {
            return new Timecode(timecodeA.totalFrames - frames, timecodeA.frameRate);
        }

        public static bool operator <(Timecode timecodeA, Timecode timecodeB)
        {
            return timecodeA.TotalSeconds() < timecodeB.TotalSeconds();
        }

        public static bool operator <=(Timecode timecodeA, Timecode timecodeB)
        {
            return timecodeA.TotalSeconds() <= timecodeB.TotalSeconds();
        }

        public static bool operator >(Timecode timecodeA, Timecode timecodeB)
        {
            return timecodeA.TotalSeconds() > timecodeB.TotalSeconds();
        }

        public static bool operator >=(Timecode timecodeA, Timecode timecodeB)
        {
            return timecodeA.TotalSeconds() >= timecodeB.TotalSeconds();
        }

        private static int FramesPerDay(short framesPerSecond)
        {
            return framesPerSecond * 60 * 60 * 24;
        }
    }

Although I've been doing software development for a couple of years, I still wrestle over what I feel are simple decisions when it comes to things like class design, so I'm hoping to ask a few questions about my class and hope that someone can re-assure or advise otherwise.

My fields are "totalFrames" and "frameRate", as opposed to a field for "hours", "minutes", "seconds", "frames", and "frameRate". The driving thought behind this decision was that it kept the class memory footprint much smaller, and I think calculations are much simpler this way.

My questions are:

1. I often struggle to decide whether a class member should be a property or a method. My basic understanding is that users of the class will view a property as little more than a field accessor, and therefore the implementation should be quick and clean and return as soon as possible. Do my class properties (hours, minutes, seconds, frames) still satisfy the aforementioned statement? There is a small calculation in each, but it really is a very quick calculation. Do people agree with that assessment, or would you make it a method instead?

2. I made the decision to rollover any timecode that goes beyond the 24 hour limit, because the timecodes I am used to using in media do the same. I have done this "rollover" in the constructor. Do people agree with the way I've done this, and that I've done this at all?

3. There is a presumption made in the '-' operator, that if the second timecode's value is greater than the first, then the second timecode must have "rolledover" the 24 hour mark, into the next day. Is this a valid presumption to make, as long as it is documented?

4. My Parse method uses a regex. Is this ok? Or are Parse methods generally implemented differently?

Any other advice or criticism is, of course, welcome.

Answer 1

Couple of quick notes:

I think you have a bug here, where you are not using the places argument, but a hard-coded value instead:

private static string PadTimecodeUnit(int unit, int places = 2)
{
    return unit.ToString().PadLeft(2, '0');
}

I see you have a private int field called totalFrames, yet you cast it to an int anyway in many of your methods, which seems unnecessary:

 return (int)this.totalFrames / this.frameRate / 60 % 60;

Answer 2

Logic

At a lot of places in your code is some kind of calculation taking place which could be reduced by precalculation at the only point where changes can happen, inside the constructor.

Changing the properties to autoimplemented properties with private setters like

/// <summary>
/// The hours segment of the timecode.
/// </summary>
public int Hours { get; private set; }  

/// <summary>
/// The seconds segment of the timecode.
/// </summary>
public int Seconds { get; private set; }  

they can be filled like

public Timecode(int hours, int minutes, int seconds, int frames, short frameRate = 25)
{

    frames += seconds * frameRate;
    frames += minutes * 60 * frameRate;
    frames += hours * 3600 * frameRate;

    TotalFrames = frames % FramesPerDay(frameRate);
    FrameRate = frameRate;
    Hours = hours;
    Minutes = minutes;
    Seconds= seconds;
}

and after changing the TotalSeconds() method to a property

public Timecode(int totalFrames, short frameRate = 25)
{
    if (frameRate <= 0) throw new ArgumentOutOfRangeException("frameRate");

    TotalFrames = totalFrames % FramesPerDay(frameRate);
    FrameRate = frameRate;
    TotalSeconds = (float)TotalFrames / (float)FrameRate; ;
    Frames = (int)totalFrames % frameRate;
    Seconds = (int)TotalFrames / FrameRate / 60 % 60;
    Minutes = (int)TotalFrames / FrameRate / 60 % 60;
    Hours = (int)TotalFrames / FrameRate / 60 / 60;
}

and a simplified ToString()

public override string ToString()
{
    return string.Format("{0}:{1}:{2}:{3}",
        PadTimecodeUnit(Hours),
        PadTimecodeUnit(Minutes),
        PadTimecodeUnit(Seconds),
        PadTimecodeUnit(Frames));
}

I will try to answer your questions tomorrow, if not already done.

Answer 3

I understand I'm late to the party, but

1. framerate argument of ToString is not used
2. For consistency it makes sense to have TryParse method as well
3. Depending on your taste conversion operator to TimeSpan could be added.