7

I'm extending a class with 10 different constructors. The new subclass, SpecialImage, is used like this:

SpecialImage specialImage = new SpecialImage(..);

// Leverage the Rotate() method of superclass Image, which
// returns a new rotated image.
// Notice the type is still SpecialImage.
SpecialImage rotated = specialImage.Rotate(...);

SpecialImage implements 5 of the constructors, each of which takes the same additional parameters, Thing stuff and int range:

public class SpecialImage<TDepth> : Image<Gray, TDepth>
    where TDepth : new()
{

    Thing _stuff;
    int _range;

    public SpecialImage(Thing stuff, int range) : base()
    {
        InitParams(stuff, range)
    }

    public SpecialImage(Thing stuff, int range, Size size) : base(size)
    {
        InitParams(stuff, range)
    }

    public SpecialImage(Thing stuff, int range, int width, int height) : base(width, height)
    {
        InitParams(stuff, range)
    }

    public SpecialImage(Thing stuff, int range, string filename) : base(filename)
    {
        InitParams(stuff, range)
    }

    public SpecialImage(Thing stuff, int range, TDepth[,,] data) : base(data)
    {
        InitParams(stuff, range)
    }

    private void InitParams(Thing stuff, int range)
    {
        _stuff = stuff;
        _range = range;
    }
}

This is a tad repetitive, which is only really a problem in the face of maintenance. Every time the construction requirements of SpecialImage change, I have 5 constructors to change.

I sure feel like I should be repeating myself less, but I don't know how to remove this type of redundancy. Is it possible to make this less redundant?

  • 2
    Just to be sure, have you considered composition? Then you'd only need one SpecialImage constructor that takes an Image. – Ixrec Jul 10 '15 at 22:48
  • I have! The base class, Image, has a massive number of methods. I'll need many of them, which means I'll need to wrap each of those methods. – kdbanman Jul 10 '15 at 23:02
  • Though I suppose selectively wrapping methods is probably cleaner than a boatload of repetitive constructors. – kdbanman Jul 10 '15 at 23:03
  • 1
    This is a prime example where "Law of Demeter" should be ignored. Use composition, and then do Image rotated = specialImage.Image.Rotate(...);. I admit that if you need to wrap the output image (rotated) in a SpecialImage again then you might need to provide wrapper methods. I think you need to carefully reconsider why you need Thing and Range, and explore alternative ways of propagating those data. – rwong Jul 11 '15 at 11:07
  • @rwong, really interesting idea. I'll seriously consider it. Also, small thing, but Image rotated = specialImage.Image.Rotate(...); should be SpecialImage rotated = .... I'll edit my question to emphasize that difference's importance. – kdbanman Jul 11 '15 at 17:44
7

You could use the Builder pattern.

Don't have 10 constructors in Image, have only one which takes all possible parameters. But don't call this constructor from your actual code. Call it from a separate Builder class.

A Builder is a separate class which is initialized with the default values, offers several setters to change these values and a build() method to create a new object with its current values.

Your code to create a new image would then look like this:

ImageBuilder builder = new ImageBuilder();
builder.setStuff(stuff);
builder.setRange(range);
builder.setSize(size); // alternative overload: builder.setSize(width, height);
builder.setFilename(filename);
Image image = builder.build();

Your SpecialImageBuilder would inherit from ImageBuilder. When Image and SpecialImage have the same properties, then likely the only method you need to override is build.

By the way, an elegant trick you can do with builders is to have each setter return this. This is called "fluent interface" and allows you to write code like this:

Image image = new ImageBuilder()
                  .setStuff(stuff)
                  .setRange(range)
                  .setSize(size)
                  .setFilename(filename)
                  .build();
  • I'm a big fan of the builder pattern - when it's appropriate it really works. I suspect mutually incompatible Builder configurations are solved with different Builder constructors, in which case we're back at square one on this particular issue. I've expressed my concerns here. If you think the problem is solvable, then please weigh in! :-) – kdbanman Jul 11 '15 at 17:42
  • 2
    I'd point out that since all of the Image constructors take a Thing, the ImageBuilder should have that be part of its constructor. – user40980 Jul 11 '15 at 17:45
  • Unfortunately the builder still need to construct an Image at some point. So you still need all the constructors. Its just extra code – Ewan Oct 22 '15 at 7:14
2

No, Assuming you can't change the base class, or know that one constructor chains to the others. There's no way to choose one of multiple base constructors from another constructor. I thought you might be able to do it with generics, but apparently not.

Even if you use a factory or builder class, where you can call a create method and switch between constructors that it uses within that method, you don't solve your immediate problem. As you still require multiple constructors in your SpecialImage Class

One solution is to wrap your base class rather than inheriting it. This assumes the base class implements an interface, or you can add one on

using System;

namespace LessConstructors
{
    public class Size
    { }
    public class Thing
    { }
    public class Gray
    { }

    public interface IImage
    {
        object MethodX();
    }

    public class Image<TColour, TDepth> : IImage
    {
        public Image() { }

        public Image(Size size) { }

        public Image(int width, int height) { }

        public Image(string filename) { }

        public Image(TDepth[,,] data) { }

        public object MethodX()
        {
            throw new NotImplementedException();
        }
    }

    public interface IConstructor<T1, T2>
    {
        Image<T1, T2> Create();
    }
    public class SizeConstructor<T1, T2> : IConstructor<T1, T2>
    {
        public Size size { get; set; }

        public Image<T1, T2> Create()
        {
            return new Image<T1, T2>(this.size);
        }
    }

    public class HeightAndWidthConstructor<T1, T2> : IConstructor<T1, T2>
    {
        public int Height { get; set; }
        public int Width { get; set; }

        public Image<T1, T2> Create()
        {
            return new Image<T1, T2>(this.Height, this.Width);
        }
    }

    public class SpecialImage<TDepth> : IImage
        where TDepth : new()
    {
        private Image<Gray, TDepth> image;
        Thing _stuff;
        int _range;

        public SpecialImage(Thing stuff, int range, IConstructor<Gray, TDepth> constructor)
        {
            _stuff = stuff;
            _range = range;
            image = constructor.Create();
        }

        public object MethodX()
        {
            return this.image.MethodX();
        }
    }
}
  • 1
    This is a pretty creative solution - it really resembles the factory pattern. IConstructor is the factory interface, each Constructor is a factory. But rather than each factory creating a different implementation of the same interface, each factory returns a different configuration of the same class. Even more interesting, the factory is dependency-injected into what was originally the subclass! Neat. If it's sufficient for SpecialImage to implement Iimage (rather than inherit `Image'), I will do something like this. – kdbanman Jul 11 '15 at 18:08
1

You could use composition as lxrec suggested and then retrieve the delegate when you actually need an Image instance. That might be problematic if you'd like to just use SpecialImage wherever you'd previously be using an Image.

Other than that, can't really see a better solution. The only other suggestion I have is to use named factory methods in lieu of constructors, but that's just for readability.

1

The main consideration is convenience for the users of your class, which is part of the usability of your API. Too many or too few options could hurt usability. In particular, too many options can confuse users and force them to spend more time choosing. Therefore, you have to consider carefully whether each constructor overload will actually be used. Aggressively remove unused ones before publishing (to internal or external users of the API).

For example, if you provide a constructor that takes a Size, you certainly don't need to provide another constructor that takes int width, int height.

The constructor that takes a string filename is also a bit disingenuous. The colorspace of an image file is decided by the file content. A JPEG file could be gray or YCbCr or YCCK. (YCbCr are customarily auto-converted into RGB upon loading, but there are situations where you need the YCbCr values from the file without the quantization error caused by the conversion.) So, a constructor should not have decided the colorspace without examining the image format first.

I am deeply puzzled by the constructor that doesn't initialize its Image. The rest of your class design seems to imply that its Image must be initialized. Is it your intention to make it a Null Object? If that is not your intention, that constructor shouldn't have existed.

Finally a bit of caution. It looks like you are designing a "single-channel high-dynamic-range" image processing layer on top of an existing library. (Single-channel as in "gray", and high-dynamic-range as in the intention to support 32-bit integer, 32-bit float and 64-bit float.) You might be surprised that some image operations do not support HDR depths at all - in other words, some image operations are limited to TDepth = Byte only. Trying to perform operations on HDR depth might throw an exception at runtime.

I can find a lot of wrongs with the underlying libraries, but I must admit that designing an API for an image library is very difficult. I applaud everyone who made a good effort and shared their work.

  • Sorry, I'm going to be critical here - you haven't actually addressed my question. I think it's fairly inevitable that my library will inherit some of the design flaws from the shoulders of the giants it stands upon. I have minimized construction options, but I'll try to do it more. Second, "the constructor that doesn't initialize its Image" Actually, all of them do. The ` : base()` clause to the right of each constructor initializes the parent class - Image in this case. Third, HDR is a good guess, but that's not what I'm up to ;) – kdbanman Jul 11 '15 at 17:59

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.