Charles Burns
Charles Burns

Reputation: 10602

Class design for system which is hierarchical, but not neatly so

I've run into this several times, so would like to use a real example and get ideas on how more experienced C# developers handle this.

I'm writing a .NET wrapper around the unmanaged MediaInfo library, which gathers various data about media files (movies, images...).

MediaInfo has many functions, each applying to different types of files. For example, "PixelAspectRatio" applies to images and video but not to audio, subtitles, or others.

A subset of the functionality I'd like to wrap is below:

General Video   Audio    Text   Image  Chapters  Menu    (Name of function)    
x       x       x        x      x      x         x       Format
x       x       x        x      x      x         x       Title
x       x       x        x      x      x         x       UniqueID
x       x       x        x      x                x       CodecID
x       x       x        x      x                x       CodecID/Hint
        x       x        x      x                x       Language
x       x       x        x      x                        Encoded_Date
x       x       x        x      x                        Encoded_Library
x       x       x        x      x                        InternetMediaType
x       x       x        x      x                        StreamSize
        x       x        x      x                        BitDepth
        x       x        x      x                        Compression_Mode
        x       x        x      x                        Compression_Ratio
x       x       x        x                       x       Delay
x       x       x        x                       x       Duration
        x       x        x                               BitRate
        x       x        x                               BitRate_Mode
        x       x        x                               ChannelLayout
        x       x        x                               FrameCount
        x       x        x                               FrameRate
        x       x        x                               MuxingMode
        x       x        x                               MuxingMode
        x       x        x                               Source_Duration
        x                x      x                        Height
        x                x      x                        Width
        x                       x                        PixelAspectRatio
                x                                        SamplingRate
x                                                        Album
x                                                        AudioCount
x                                                        ChaptersCount
x                                                        EncodedBy
x                                                        Grouping
x                                                        ImageCount
x                                                        OverallBitRate
x                                                        OverallBitRate_Maximum
x                                                        OverallBitRate_Minimum
x                                                        OverallBitRate_Nominal
x                                                        TextCount
x                                                        VideoCount

As you can see, the start of a not-too-bad class mapping would be one class for the functionality specific to each stream type and a base class with functionality common to all types.

Then this path gets a little less obvious. There are many functions common to {general, video, audio, text, and image} stream types. Okay, so I guess I can make a class with a smelly name like "GeneralVideoAudioTextImage", and then another named GeneralVideoAudioText (which inherits from GeneralVideoAudioTextImage) for the functionality common to those things, but not "Image" streams. This would awkwardly follow the "is a" rule of class hierarchies, I guess.

This is already not looking elegant, but then there are those occasional cases like "Width" which do not fit into any group which is cleanly a subset of another group. Those cases can simply duplicate functionality where necessary -- implement in Video, Text, and Image individually, but that would obviously violate DRY.

A common first approach would be MI, which C# does not support. The usual answer to that seems to be "use MI with interfaces," but I can't find entirely see how that can follow DRY. Perhaps my failing.

Class hierarchies have been discussed on SO before, as have alternatives to MI (extension methods, etc.) but none of those solutions seemed a good fit. For example, extension methods seem better used for classes whose source you cannot edit, like the String class, and are harder to locate because they are not really tied with the class, though they may work. I haven't found a question asked about a situation like this, though that's probably a failure of my use of the search tool.

An example of a MediaInfo feature, wrapped, might be:

int _width = int.MinValue;
/// <summary>Width in pixels.</summary>
public int width {
    get {
        if(_width == int.MinValue)
            _width = miGetInt("Width");
        return _width;
    }
}

// ... (Elsewhere, in another file) ...
/// <summary>Returns a MediaInfo value as an int, 0 if error.</summary>
/// <param name="parameter">The MediaInfo parameter.</param>
public int miGetInt(string parameter) {
    int parsedValue;
    string miResult = mediaInfo.Get(streamKind, id, parameter);
    int.TryParse(miResult, out parsedValue);
    return parsedValue;
}

My question is: How have you handled situations like this, systems which are kind-of-hierarchical-but-not-quite? Have you found a reasonably elegant strategy, or just accepted that not every simple problem has one?

Upvotes: 5

Views: 409

Answers (2)

Charles Burns
Charles Burns

Reputation: 10602

I marked Andrew Kennan's answer correct because I think it is probably the best general approach to a somewhat complex hierarchy. However, I didn't up using his suggestion in my final code.

I am posting this new answer in order to help any future SO users who look up this question up because of a similar problem.

While interfaces are probably the best general solution for something more cleanly hierarchal, they didn't work out for me in this case. I wouldn't have been able to release this as an OSS project without using an anonymous name had I done hierarchical grouping. The interface names implied were starting to smell really bad, like "IGeneralVideoAudioTextImageMenuCommon" and "IVideoAudioTextImageMenuCommon"; the inheritance declarations with 4 or 5 of these were inelegant the likes of which mankind had not previously seen:

    ///<summary>Represents a single video stream.</summary>
    public sealed class VideoStream : Media, IGeneralVideoAudioTextImageMenuCommon,
    IGeneralVideoAudioTextMenuCommon, IVideoAudioTextImageMenuCommon,
    IVideoTextImageCommon, /* ...ad nauseum. */
    {
        // What would you even name these variables?
        GeneralVideoAudioTextImageMenuCommon gvatimCommon;
        GeneralVideoAudioTextMenuCommon gvatmCommon;
        VideoAudioTextImageMenuCommon vatimCommon;
        VideoTextImageCommon vticCommon;

        public VideoStream(MediaInfo mi, int id) {
        gvatimCommon = new GeneralVideoAudioTextImageMenuCommon(mi, id);
        gvatmCommon = new GeneralVideoAudioTextMenuCommon(mi, id);
        vatimCommon = new VideoAudioTextImageMenuCommon(mi, id);
        vticCommon = new VideoTextImageCommon(mi, id);
        // --- and more. There are so far at least 10 groupings. 10!
        /* more code */
    }

The naming, one of the two hard problems in computer science according to Phil Karlton, is really approaching a more philosophical definition of "is a". A car "is a" vehicle, and yes technically a car also "is a" MotorcycleCarTruckOrAirplane, but this is all about allowing humans to understand the code, right?

I considered a hybrid approach, where groupings with a small set of functionality would be merged. But then I asked myself, "does this make the code easier to understand?" My answer was, "No.", because seeing an inheritance hierarchy implies a certain code organization to the reader. A second method would likely add more complexity than was saved: Not a win.

My solution, though I still believe a better one must exist, is to place all functionality common to more than one stream into a single class, "MultiStreamCommon", grouped internally using #regions:

public class MultiStreamCommon : Media
{
    public MultiStreamCommon(MediaInfo mediaInfo, StreamKind kind, int id)
        : base(mediaInfo, id) {
            this.kind = kind;
    }

    #region AllStreamsCommon
    string _format;
    ///<summary>The format or container of this file or stream.</summary>
    ///<example>Windows Media, JPEG, MPEG-4.</example>
    public string format { /* implementation */ };

    string _title;
    ///<summary>The title of the movie, track, song, etc..</summary>
    public string title { /* implementation */ };

    /* more accessors */
    #endregion

    #region VideoAudioTextCommon
    /* Methods appropriate to this region. */
    #endregion
    // More regions, one for each grouping.
}

Each stream creates an instance of MultiStreamCommon and exposes relevant functionality through accessors:

public sealed class VideoStream : Media
{
    readonly MultiStreamCommon streamCommon;

    ///<summary>VideoStream constructor</summary>
    ///<param name="mediaInfo">A MediaInfo object.</param>
    ///<param name="id">The ID for this audio stream.</param>
    public VideoStream(MediaInfo mediaInfo, int id) : base(mediaInfo, id) {
        this.kind = StreamKind.Video;
        streamCommon = new MultiStreamCommon(mediaInfo, kind, id);
    }

    public string format { get { return streamCommon.format; } }
    public string title { get { return streamCommon.title; } }
    public string uniqueId { get { return streamCommon.uniqueId; } }
    /* ...One line for every media function relevant to this stream type */
}

The upside is that the ugliness, while still there, is limited to the MultiStreamCommon class. I believe #region groupings are much more readable than each stream class having six or so inherited interfaces. How does a maintainer know where to add a new MediaInfo function? Just figure out which streams it applies to and put it in the appropriate region. If they misgroup, it still works and can be seen through an accessor.

The downside is that media streams do not inherit the appropriate functionality -- accessors must be written. Documentation is also not inherited, so each stream will need a tag for every accessor, leading to duplicated documentation. That said: Duplicated documentation is better than duplicated code.

Enforcing a proper inheritance hierarchy in this case is far more complex than the underlying problem, which we have to remember, is just wrapping some library.

For those interested in the code associated with these thread, or in its purpose (easily getting media file information using .NET), the Google Code page for this project is here. Please let me know of any thoughts, especially from those who have posted the large quantities of beautiful code I've come across (Jon Skeet, Marc Gravell, and others).

Upvotes: 0

Andrew Kennan
Andrew Kennan

Reputation: 14157

I think you're best off using a combination of interfaces and, if the implementation is more complex than a bunch of properties, composition to provide shared implementations of the interfaces:

abstract class Media  {
 // General properties/functions
}

class VideoAndImageCommon { // Crappy name but you get the idea
 // Functions used by both video and images
}

interface IVideoAndImageCommon {
 // Common Video & Image interface
}

class Video : Media, IVideoAndImageCommon {
  private readonly VideoAndImageCommon _commonImpl = new VideoAndImageCommon();

  // Implementation of IVideoAndImageCommon delegates to _commonImpl.
}

class Image : Media, IVideoAndImageCommon {
  private readonly VideoAndImageCommon _commonImpl = new VideoAndImageCommon();

  // Implementation of IVideoAndImageCommon delegates to _commonImpl.
}

Upvotes: 5

Related Questions