Problem with runaway number of properties

Question

Rewritten Question

I appreciate the feedback and in response to that I'm re-writing my question. I can't give my specific situation (classes, etc), nor do I think that it would be helpful, as I work in a very niche area that wouldn't make much sense to those outside of it, but I will try and use a similar but invented analogous situation to give something more concrete.

I have an application and two libraries of interest here. These are quite mature (about ten years old) and relate to a product that has been selling that long. The application reads files of various types, including images, and makes them searchable and viewable. It also produces a detailed report.

One library (ImageIO) is responsible for reading images. It doesn't just read JPEGs and PNGs, but hundreds of different formats that have been encountered over the years. Formats are continuously being added to it. It can also spit out standard formats like PNGs and JPEGs.

Another library is responsible for the reporting. It doesn't just handle images, but all sorts of file types. It gives a detailed report including a list of all of the metadata used.

When I got handed the code, the main application has a class called Document which contains, among other things, a list of Images. An Image has some set properties and methods including Height, Width, and GetBitmap. Each of the image types has its own subclass; JpegImage, PngImage, TiffImage, DicomImage and so on. Most of these have custom properties; camera used, white point, colorspace, title, GPS location and so on. Most have between one and six extra properties. Some properties are common to many types (like exif data), while many image types, particularly the more niche types (like BobsImage) have properties that are unique to that image type.

Image

// Some methods
int[][] GetBitmap()

// Some properties
int Height
int Width

The main application only uses a few of these properties when they exist. The reporting library reports on them all. There are dozens of properties. There are no special methods, though behind the scenes, some types use some properties for the standard methods. For example, using the aspect ratio for producing the BitMap.

The application uses a magic string to tell the reporting library what sub-class the images really are. The reporting library then uses that to cast the Image back to it's subclass, and then heaps of ifs and switches to report accordingly.

I was not happy with this architecture. My first attempt was to turn Image into and an IImage interface, and then bundle properties into groups and have relevant interfaces for the extra. The IImage seems to work fine, but the properties are an issue; there were about as many interfaces as properties, and then they were tested with an "is a" style test, which felt like I was pretty much back with the switch statements.

IImage

// Some methods
int[][] GetBitmap()

// Some properties
int Height
int Width

IGps

Double[] GetGps()

My second attempt was to just add bool HasProperty(PropertyId id) and T GetProperty<T>(PropertyId) to the IImage. Then none of the other interfaces were required.

enum PropertyId
GpsData,
ExifData, ...


IImage

// Some methods
int[][] GetBitmap()

// Some properties
int Height
int Width

// New methods
bool HasProperty(PropertyId id)
T GetProperty<T>(PropertyId)
List<PropertyId> GetSupportedProperties()

This really cleaned up the Reporting library; it could enumerate over the GetSupportedProperties and no ifs or switches. It also means it didn't have to care about the hundreds of sub-classes, and in fact, sub-classes weren't even required. A generic Image class that implemented IImage could be made that just contained a list of properties, types for run-time type checking and values.

It still feels bad. It removes compile-time type checking. For example, var gps = GetProperty<string>(PropertyId.Gps) would compile, but a Gps is a double array, not a string. So it would throw an exception at runtime.

Also, Flater points out I'm corrupting the point of interfaces, and he is completely right. The reason I'm asking this question is because I think my answer is dirty; it's just the least dirty answer I have. The first and second approaches seemed worse (the original seemed much worse).

The solution would preferably be able to handle adding properties easily. I have no control over what data image formats decide to use. We have not written a single image format; we either get them from specs (like PNG), or as with about 95% of out formats, we reverse engineer them. That is the benefit our software brings; it understands, views and reports on rare file types (including image formats). About 70% of our time goes into reverse engineering new formats, which arrive on our doorstep faster than we can reverse engineer them.

The reverse engineering really hampers forward planning. You might find it hard to believe some of the data that is stored. I'm constantly surprised, and I've been doing this for over a decade. This means that we have to be reactive, as we can't be proactive.

When I used the tree of interfaces (I don't care if they inherit from IImage or from others as needed) I find that I do have fewer interfaces than there are image types, or properties, but still dozens. And checking to see if an object implements an interface doesn't feel much better than calling HasProperty, but perhaps that is my own subjective issue.

Flater's suggestion seems to line up with my first attempt (the second model) a bit, and Simon B seems to be suggesting my current, second attempt (the third model) is best. I could be reading this wrong. If either is true, I'll live with the dirty feelings; it just felt like there must be some better approach out there, though I haven't found it.

I hope the context, though fake (but only a little fake) helps. I'm sorry I wasn't more clear the first time around. I hope this is better. I appreciate the time people took to help, and I will eventually accept an answer.

---

Old question kept for reference only

I am refactoring a smelly class and I'm sure I'm making a pig's ear of it. It feels like a common problem, but I can't see a common solution. As the domain is fairly niche, I've changed names etc.

I have an interface, let's say IThing, which has a few methods and started with a few properties. As time went on, many different IThings cropped up with different properties. (IThing is a sort of interface to multiple different reverse-engineered Things that we have no control over, so the properties are thrust on us.)

We ended up with a pattern of the sort bool HasSpecialNumber, int SpecialNumber {get; set;}. This got smelly as we added more and more properties, with every implementation having to implement 20+ methods just to say they don't support a property.

I thought of using a mixin approach, but maybe I'm thinking of this wrongly, because it would involve as many interfaces as properties or combinations of properties, and a lot of casting. It also seems heavy-handed when I'm only providing properties here and the methods are not changing.

An IThing looks sort of like this (C#ish pseudo-ish code)

IThing

// Some methods every Thing supports
DoSomething
DoSomethingElse

// A bunch of properties some Things support
bool HasSpecialNumber { get; }
int SpecialNumber { get; }

bool HasName { get; }
string Name { get; }

... and so on

Apart from the smell, every time a property was added, a whole bunch of classes broke. These all needed to be serialized too, using protobuf-net. Many of these classes were only distinct in that they had special objects.

The next thing we tried was reducing the properties to two methods, with a private method for adding properties.

IThing

// Some methods every Thing supports
DoSomething
DoSomethingElse

// A bunch of properties some Things support
bool HasProperty( PropertyIdEnum propertyId )
T GetProperty<T>( PropertyIdEnum propertyId )

// Private method for adding properties
void AddProperty<T>( PropertyIdEnum propertyId, T value )

This sort of worked. Dozens of properties became two accessor methods, and updating the PropertyIdEnum didn't break anything. The AddProperty was used to add properties to a dictionary that mapped the IDs to objects, with a Type stored alongside to ensure no weird casting errors. But I exchanged compile-time type checking for run-time type checking. Also, protobuf-net doesn't support serializing Objects or Types, though that is an implementation detail.

We ditched the AddProperty abstraction and went back to dozens of classes. That resolved the protobuff-net issue at the cost of having a lot more classes to worry about. We still lack the compile-time type safety.

I see this issue all over the place in areas I work. For example, ffmpeg and the CODECs they deal with, each with special behaviour. The solutions they use are constrained by backwards compatibility though, an they are working in heavily optimized C while I'm in C#. Is there some pattern or advise for dealing with a run-away set of properties that need to be handled trough a single common interface? If I had control over the properties I would just not be in this situation in the first place, but I don't, so here I am.

Answer 1

The usual solution is the one you have abandoned, which is generally summarized as "prefer composition over inheritance".

Create a simple class, which has a container to hold the special values. That container could be a dictionary/map, which is effectively a collection of name+value pairs. If you use the right container, querying it can be reasonably fast.

Answer 2

You're trying to metaprogram and it's going to cause you more issues than you're actually solving. You're trying to create an interface which takes any (not yet defined) set of properties, which defeats the purpose of the interface: enforcing that your classes implement a given set of properties (or methods).

A bunch of properties some Things support

Interfaces should list what a class must have. Not what it could possible have. That's just not the purpose of an interface. This is an unusual variation on a Liskov (LSP) violation, you are signing yourself up for having to write constant checks whether your thing actually contains a given property or not, along the lines of:

if(myThing.HasSpecialNumber)
{
    // do something with myThing.SpecialNumber
}

Your second attempt would rewrite the above syntax, but it doesn't actually prevent you from needing to constantly doublecheck your things.

Statically typed languages focus heavily on knowing the structure of your objects, and not having to guess/check for it. Split your interfaces and only implement them in classes that fulfill the interface's contract, and don't trying to fake-implement them in other classes.

Something along the lines of:

public interface IThing
{
    // Some methods every Thing supports
    void DoSomething();
    void DoSomethingElse();
}

public interface IThingWithName : IThing
{
    string Name { get; set; }
}

public class ThingWithoutName : IThing
{
    public void DoSomething() 
    {

    }

    public void DoSomethingElse()
    {

    }
}

public class ThingWithName : IThingWithName 
{
    public void DoSomething() 
    {

    }

    public void DoSomethingElse()
    {

    }

    public string Name { get; set; }
}

This enables you to know for a fact that a given class (ThingWithoutName/ThingWithName), and thus any of its instances, has a Name property or not, and thus you don't need to keep checking if it does.

This ensures that you have strong and static typing instead of runtime object evaluations, which C# isn't built to do (it can be done on a technical level, but it's a bad approach with many drawbacks).

Some footnotes:

• IThingWithName is obviously not a great name, but your question doesn't have enough context for me to pick an appropriate name here.
• It's unclear whether you need to inherit your interfaces, or define separate interfaces and implement both of them on a class. This is a contextual consideration that your question does not specify. I suspect I picked the correct option based on how you phrased your question, but I can't be 100% sure.
• If you're going to end up with a unique interface for every property, you're probably doing something wrong. But again, the question is too light on context for me to definitively state anything on this.

Answer 3

Your problem: You have about hundred different image classes. Some, lets say six, have an "EXIF" property. The user of your library doesn't care what image class handles his image, they want to know whether the image has the "EXIF" property or not, and read or change the EXIF property. And since the user of your library doesn't know at build time what the image is, this has to be done at runtime.

One approach is that the user only sees an "Image" class, and the "Image" class has three methods "hasExif", "getExif", "setExif". "hasExif" returns true or false, "getExif" gets the data or asserts / throws an exception, "setExif" sets the data or asserts / throws an exception. The Image class would have pointers to an implementation of an EXIF setter and getter; Image classes supporting the property set them, others don't.

It will be lots of code, but all very straightforward, all clean code. Having two dozen similar properties isn't complexity.

Answer 4

If I understand your question properly, your problem is not with the image processing at all, it is simply in presenting the full set of properties to the reporting app when you have so many variations of property sets.

There's a very simple solution to this: use a dictionary (a.k.a map). Add a single method called GetProperties() (or something) like that to a base interface that all the image classes implement. Each type of image can then present its properties to the reporting layer. The reporting layer then can simply iterate over all the properties and present them.

It might be good enough to just have a dictionary of strings keys and string values. This leaves the job of how to present them in the image class which can be problematic if some of these properties are complex (e.g. made up of multiple values) and you need to be able to present them in various ways. In that case, it might be useful to define a second interface which accepts a presentation object and calls methods on it to describe the property. I'm short on time at the moment but if you want more detail on that, I can get back to you, upon request.

Answer 5

tl;dr– Sounds like you're basically decompiling the old source code; since decompilation tends to yield messy results, that you're getting messy results seems like a foreseeable consequence. So instead of a bottom-up refactoring, you'll probably need a top-down redesign where the old code can help fill out implementation details, but probably shouldn't rely on it to provide the overall logical structure.

---

My suspicion is that you're doing a bottom-up approach rather than a top-down approach.

For example, consider a well-designed program that's then compiled to native code with aggressive optimizations enabled. Then someone takes that compiled-program and decompiles it. Regardless of the input program's clear, elegant structure, the decompilation of it can be a real mess.

So my suspicion's that, in refactoring this older program, you're doing it kinda like a decompiler – instead of taking a top-down approach, you're trying to decompile the old source code piece-by-piece, resulting in the same sort of mess that decompilers typically produce.

Fundamentally, you can't do the refactoring piece-by-piece; instead, you need to figure out some general logical structure that can describe everything, then descend from that, as in a top-down approach. This is, write your new program as a new program rather than as a decompilation of an old program.

Specifically, forget about the issues with interfaces and properties and whatnot for now. Instead, focus on how to structure the overall logic that describes, say, images, if that's what you're working with. Figure out an abstraction that encompasses them all, then build subtypes that narrow in on specifics. Once you have a good logical structure, then you can draw from the old code to help fill out the implementation details.

Otherwise, it'd seem like you'd be stuck doing the work of a decompiler, with the messy final product being a foreseeable consequence.

---

Thought experiment: Why do decompilers produce such messy results?

You may want to take some time to look into decompilation and consider why, exactly, decompilers don't exactly reproduce good source code, especially when the original program was written in a very high-level language and then compiled to native code with aggressive optimizations.

Once you get a handle on that, look at the source code you're refactoring as compiled native code. I mean, yeah, you may be refactoring stuff that's already in C#, but C# isn't the real source code; the real source code existed in the original software designer's mind, which they then compiled into C#. This compilation from-mind-to-C# is a lossful process, much like compiling C# to native-code is; and, like a decompiler not getting the structure that was lost in the compilation, you're not getting the structure that was lost when whoever first wrote the code compiled it into C#.

Which you can't really fix. Unfortunately, if the original programmer is gone, the original source code is, too. The C# you've got is a poor substitute for the actual full specification that once existed in someone's mind.

In short, my suggestion'd be to focus on how your refactoring approach is like what a decompiler does. Once you get that, I suspect that the way to fix it should be more obvious.

Answer 6

The problem here is the lack of encapsulation. Interfaces should opt to expose methods over properties. By having the calling code access the properties in each of these implementations, knowledge leaks out from the implementation to the caller.

Would it be possible to delegate the responsibility of reporting to each implementation? You would then have a GenerateReport method on your interface and the caller would not need to know what properties the class has.

Repeat for the other ways in which the class is used and you might find that the responsibilities are more clear.

---

The dictionary solution sucks because you lose static type checking, but those properties shouldn't be publically accessible in the first place. Once they are private, static typing will work out naturally and you will be forced to figure out ways to keep the format specific knowledge in the format class itself.

Answer 7

In Swift, and possibly other languages, another approach may be possible: in Swift you define a protocol (almost the same as an interface) say ExifHandler with two methods getExif and setExif. Any class or struct independent of can declare that it supports ExifHandler. Your code accessing the property would look like

If let handler = image as? ExifHandler {
    let exif = handler.exif
    ...
    handler.exif = newExif
}

Or:

If let exif = image as? ExifHandler? .exif {
   ...
}