5
\$\begingroup\$

I'm developing an MP4 parser. Each class represents each box in MP4 file, and every box class derives from base box class. The problem is that below code is duplicated on every box class:

    private ulong MyDataSize;
    public override ulong BoxContentEndOffset
    {
        get { return base.BoxContentEndOffset + this.MyDataSize; }
    }

    protected new ParsingState MyParsingState { get; set; }
    public override bool IsParsingDone
    {
        get { return this.MyParsingState.IsParsingDone(); }
    }
    public override bool CanContinueParsing
    {
        get { return ((base.MyParsingState.IsParsingDone() ||
         base.MyParsingState.CanContinueParsing()) && this.MyParsingState.CanContinueParsing()); }
    }

Also, each box-container class has duplicated code like below. (AsReadOnly is an extension method here)

public sealed class SampleTableBox : SimpleContainerBoxBase, IActualElement<IBox>
{
    private static readonly IElementType<IBox>[] _ImplementedElementTypes
     = { BoxElementType.Get("stbl") };
    public ICollection<IElementType<IBox>> ImplementedElementTypes
    {
        get { return _ImplementedElementTypes; }
    }

    private static readonly ReadOnlyCollection<IElementType> _PossibleParents
        = new ReadOnlyCollection<IElementType>(
            new IElementType[]
            {
                BoxElementType.Get("minf")
            });
    public override ICollection<IElementType> PossibleParents
    {
        get { return _PossibleParents; }
    }

    private static ReadOnlyDictionary<ICollection<IElementType>, Tuple<int, int>> _RequiredElements
        = new ReadOnlyDictionary<ICollection<IElementType>, Tuple<int, int>>(
            new Dictionary<ICollection<IElementType>, Tuple<int, int>>()
            {
                { new [] { BoxElementType.Get("stsd") }.AsReadOnly<IElementType>(), new Tuple<int, int>(1, 1) },
                { new [] { BoxElementType.Get("stts") }.AsReadOnly<IElementType>(), new Tuple<int, int>(1, 1) },
                { new [] { BoxElementType.Get("stsz"), BoxElementType.Get("stz2") }.AsReadOnly<IElementType>(), new Tuple<int, int>(1, 1) },
                { new [] { BoxElementType.Get("stsc") }.AsReadOnly<IElementType>(), new Tuple<int, int>(1, 1) },
                { new [] { BoxElementType.Get("stco"), BoxElementType.Get("co64") }.AsReadOnly<IElementType>(), new Tuple<int, int>(1, 1) }
            });
    public override IDictionary<ICollection<IElementType>, Tuple<int, int>> RequiredElements
    {
        get { return _RequiredElements; }
    }
}

What is the best way to reduce duplicate code?

EDIT: SampleContainerBoxBase code

public abstract class SimpleContainerBoxBase : BoxBase, IParent<IBox>
{
    private ElementArray<IBox> _BoxArray;

    private ulong MyDataSize;
    public override ulong BoxContentEndOffset
    {
        get { return base.BoxContentEndOffset + this.MyDataSize; }
    }

    protected new ParsingState MyParsingState { get; private set; }
    public override bool IsParsingDone
    {
        get { return this.MyParsingState.IsParsingDone(); }
    }
    public override bool CanContinueParsing
    {
        get { return ((base.MyParsingState.IsParsingDone() || base.MyParsingState.CanContinueParsing()) && this.MyParsingState.CanContinueParsing()); }
    }

    public IElementType ParentType
    {
        get { return this.BoxType; }
    }

    public virtual IDictionary<ICollection<IElementType>, Tuple<int, int>> RequiredElements
    {
        get { return null; }
    }
    public virtual bool CheckChildAdditionalRequirements()
    {
        return this.CallCheckChildAdditionalRequirements();
    }
    public ICollection<IBox> ChildElements
    {
        get { return (this._BoxArray == null ? null : _BoxArray.Elements); }
    }

    public override MP4ParsingError TryParse(Stream stream, IParent parent, ulong boxPosition, ulong boxLength)
    {
        MP4ParsingError error;

        if (!base.MyParsingState.IsParsingDone())
        {
            error = base.TryParse(stream, parent, boxPosition, boxLength);
            if (error != MP4ParsingError.Success)
                return error;
        }

        if (this.MyParsingState == ParsingState.Initial)
        {
            this._BoxArray = new ElementArray<IBox>();

            ulong availableDataLength = this.Size - base.BoxContentEndOffset;
            this.MyDataSize = availableDataLength;

            this.MyParsingState = ParsingState.ParsedPartially;
        }

        error = _BoxArray.TryParse(stream, this, boxPosition + base.BoxContentEndOffset, this.MyDataSize);
        if (error == MP4ParsingError.Success)
        {
            /* success */
        }
        else if (error == MP4ParsingError.DelayedParsingRequired)
        {
            this.MyParsingState = ParsingState.DelayedParsing;
            return MP4ParsingError.DelayedParsingRequired;
        }
        else
        {
            this.MyParsingState = ParsingState.InvalidStructure;
            return error;
        }

        this.MyParsingState = ParsingState.Parsed;
        return MP4ParsingError.Success;
    }
}

BoxBase:

public abstract class BoxBase : IBox
{
    public ulong PositionInFile { get; set; }
    private ulong MyDataSize;
    public virtual ulong BoxContentEndOffset
    {
        get { return 0 + this.MyDataSize; }
    }

    public IParent Parent { get; private set; }
    public virtual ICollection<IElementType> PossibleParents
    {
        get { return null; }
    }

    protected ParsingState MyParsingState { get; private set; }
    public virtual bool IsParsingDone
    {
        get { return this.MyParsingState.IsParsingDone(); }
    }
    public virtual bool CanContinueParsing
    {
        get { return this.MyParsingState.CanContinueParsing(); }
    }

    public ulong Size { get; set; }
    public IElementType<IBox> BoxType { get; set; }

public virtual MP4ParsingError TryParse(Stream stream, IParent parent, ulong boxPosition, ulong boxLength)
{
    /* (MPEG-4 'Box' parsing; size and FourCC) */
}
}

IParent:

public interface IParent
{
    IElementType ParentType { get; }
    IDictionary<ICollection<IElementType>, Tuple<int, int>> RequiredElements { get; }
    bool CheckChildAdditionalRequirements();
}

public interface IParent<TElement> : IParent
    where TElement : IBox
{ 
    ICollection<TElement> ChildElements { get; }
}

IBox:

public interface IBox
{
    ulong PositionInFile { get; set; }
    ulong BoxContentEndOffset { get; }

    IParent Parent { get; }
    ICollection<IElementType> PossibleParents { get; }

    bool IsParsingDone { get; }
    bool CanContinueParsing { get; }

    ulong Size { get; set; }
    IElementType<IBox> BoxType { get; set; }

    MP4ParsingError TryParse(Stream stream, IParent parent, ulong boxPosition, ulong boxLength);
}
\$\endgroup\$
5
  • \$\begingroup\$ please post base class code, so we can make changes accordingly \$\endgroup\$
    – Paritosh
    Commented Aug 13, 2014 at 3:09
  • \$\begingroup\$ @paritosh added base class code \$\endgroup\$ Commented Aug 13, 2014 at 3:30
  • \$\begingroup\$ I don't understand, why can't you have the duplicated code only in the base class? \$\endgroup\$
    – svick
    Commented Aug 13, 2014 at 14:40
  • \$\begingroup\$ @svick because each box class must have its own parsing state (ex: if the parent box class successfully parsed its contents but the child finds its own contents invalid, IsParsingDone and CanContinueParsing must be false and hence should be overridden) \$\endgroup\$ Commented Aug 13, 2014 at 15:55
  • \$\begingroup\$ I have an implementation at net7mma.codeplex.com that your welcome to reference. \$\endgroup\$
    – Jay
    Commented Oct 16, 2015 at 22:19

1 Answer 1

3
\$\begingroup\$

If I understand this correctly, you shouldn't use inheritance here, use composition instead.

A child box is not a kind of the parent box, it's contained within the parent box. You treat boxes that can contain multiple child boxes this way, I don't see why boxes that contain only a single child should be any different.

This way, all the duplicate logic will be in the base class (which will now be a real base class, not a parent box), so it will exist only once.


new ReadOnlyDictionary<ICollection<IElementType>, Tuple<int, int>>(
    new Dictionary<ICollection<IElementType>, Tuple<int, int>>()
    {
        { new [] { BoxElementType.Get("stsd") }.AsReadOnly<IElementType>(), new Tuple<int, int>(1, 1) },
        { new [] { BoxElementType.Get("stts") }.AsReadOnly<IElementType>(), new Tuple<int, int>(1, 1) },
        { new [] { BoxElementType.Get("stsz"), BoxElementType.Get("stz2") }.AsReadOnly<IElementType>(), new Tuple<int, int>(1, 1) },
        { new [] { BoxElementType.Get("stsc") }.AsReadOnly<IElementType>(), new Tuple<int, int>(1, 1) },
        { new [] { BoxElementType.Get("stco"), BoxElementType.Get("co64") }.AsReadOnly<IElementType>(), new Tuple<int, int>(1, 1) }
    }

When you have code that has as much repetition as this, consider writing a helper function that will take care of it:

public static ReadOnlyDictionary<TKey, TValue> AsReadOnly<TKey, TValue>(
    this IDictionary<TKey, TValue> input)
{
    return new ReadOnlyDictionary<TKey, TValue>(input);
}

private static ReadOnlyDictionary<ICollection<IElementType>, Tuple<int, int>>
    CreateRequiredElementsDictionary(Dictionary<string[], Tuple<int, int>> input)
{
    return input.ToDictionary(
        kvp =>
            (ICollection<IElementType>)kvp.Key
                .Select(BoxElementType.Get)
                .ToArray<IElementType>()
                .AsReadOnly(),
        kvp => kvp.Value)
        .AsReadOnly();
}

…

CreateRequiredElementsDictionary(
    new Dictionary<string[], Tuple<int, int>>
    {
        { new[] { "stsd" }, Tuple.Create(1, 1) },
        { new[] { "stts" }, Tuple.Create(1, 1) },
        { new[] { "stsz", "stz2" }, Tuple.Create(1, 1) },
        { new[] { "stsc" }, Tuple.Create(1, 1) },
        { new[] { "stco", "co64" }, Tuple.Create(1, 1) }
    })

Or even better, write a custom collection that allows you to use better collection initializer:

class RequiredElementsDictionary : Dictionary<ICollection<IElementType>, Tuple<int, int>>
{
    // TODO: better parameter names
    public void Add(IEnumerable<string> strings, int firstInt, int secondInt)
    {
        Add(
            strings.Select(BoxElementType.Get).ToArray<IElementType>().AsReadOnly(),
            Tuple.Create(firstInt, secondInt));
    }
}

…

new RequiredElementsDictionary
{
    { new[] { "stsd" }, 1, 1 },
    { new[] { "stts" }, 1, 1 },
    { new[] { "stsz", "stz2" }, 1, 1 },
    { new[] { "stsc" }, 1, 1 },
    { new[] { "stco", "co64" }, 1, 1 }
}.AsReadOnly()

Or maybe also switch the order of parameters, which lets you use params:

class RequiredElementsDictionary : Dictionary<ICollection<IElementType>, Tuple<int, int>>
{
    // TODO: better parameter names
    public void Add(int firstInt, int secondInt, params string[] strings)
    {
        Add(
            strings.Select(BoxElementType.Get).ToArray<IElementType>().AsReadOnly(),
            Tuple.Create(firstInt, secondInt));
    }
}

…

new RequiredElementsDictionary
{
    { 1, 1, "stsd" },
    { 1, 1, "stts" },
    { 1, 1, "stsz", "stz2" },
    { 1, 1, "stsc" },
    { 1, 1, "stco", "co64" }
}.AsReadOnly()
\$\endgroup\$
1
  • \$\begingroup\$ Well, inheritance goes like this: BoxBase --> FullBoxBase --> MovieHeaderBox, so each of them needs to maintain its own parsing state. SimpleContainerBoxBase is a base class for all container boxes (MovieBox, TrackBox, etc). \$\endgroup\$ Commented Aug 14, 2014 at 0:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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