views:

986

answers:

10

The question I want to ask is thus:

Is casting down the inheritance tree (ie. towards a more specialiased class) from inside an abstract class excusable, or even a good thing, or is it always a poor choice with better options available?

Now, the example of why I think it can be used for good.

I recently implemented Bencoding from the BitTorrent protocol in C#. A simple enough problem, how to represent the data. I chose to do it this way,

We have an abstract BItem class, which provides some basic functionality, including the static BItem Decode(string) that is used to decode a Bencoded string into the necessary structure.

There are also four derived classes, BString, BInteger, BList and BDictionary, representing the four different data types that be encoded. Now, here is the tricky part. BList and BDictionary have this[int] and this[string] accessors respectively to allow access to the array-like qualities of these data types.

The potentially horrific part is coming now:

BDictionary torrent = (BDictionary) BItem.DecodeFile("my.torrent");
int filelength = (BInteger)((BDictionary)((BList)((BDictionary)
             torrent["info"])["files"])[0])["length"];

Well, you get the picture... Ouch, that's hard on the eyes, not to mention the brain. So, I introduced something extra into the abstract class:

public BItem this[int index]
{
    get { return ((BList)this)[index]; }
}
public BItem this[string index]
{
    get { return ((BDictionary)this)[index]; }
}

Now we could rewrite that old code as:

BDictionary torrent = (BDictionary)BItem.DecodeFile("my.torrent");
int filelength = (BInteger)torrent["info"]["files"][0]["length"];

Wow, hey presto, MUCH more readable code. But did I just sell part of my soul for implying knowledge of subclasses into the abstract class?

EDIT: In response to some of the answers coming in, you're completely off track for this particular question since the structure is variable, for instance my example of torrent["info"]["files"][0]["length"] is valid, but so is torrent["announce-list"][0][0], and both would be in 90% of torrent files out there. Generics isn't the way to go, with this problem atleast :(. Have a click through to the spec I linked, it's only 4 small dot-points large.

+3  A: 

You really should not access any derived classes from the base class as it pretty much breaks the idea of OOP. Readibility certainly goes a long way, but I wouldn't trade it for reusability. Consider the case when you'll need to add another subclass - you'll also need to update the base class accordingly.

petr k.
+1  A: 

If file length is something you retrieve often, why not implement a property in the BDictionary (?) class... so that you code becomes:

BDictionary torrent = BItem.DecodeFile("my.torrent");
int filelength = torrent.FileLength;

That way the implementation details are hidden from the user.

RickL
If you read the spec (which was linked to, and a whoping 6 lines long), BDictionary's can be anywhere in the data structure, so this answer really doesn't make sense at all, sorry.
Matthew Scharley
+5  A: 

I think I would make the this[int] and this[string] accessors virtual and override them in BList/BDictionary. Classes where the accessors does not make sense should cast a NotSupportedException() (perhaps by having a default implementation in BItem).

That makes your code work in the same way and gives you a more readable error in case you should write

 (BInteger)torrent["info"][0]["files"]["length"];

by mistake.

Rasmus Faber
As a sidenote, this is what I was effectively doing, except I was using new to override instead of virtual/override. Stupid, stupid. Much cleaner, nicer solution, thankyou!
Matthew Scharley
'new' doesn't actually override. You just get 2 methods with the same name - if someone casts to the base class, then calls the method, they will get the original base class method, NOT the overridden one
Orion Edwards
A: 

Did you concider parsing a simple "path" so you could write it this way:

BDictionary torrent = BItem.DecodeFile("my.torrent");
int filelength = (int)torrent.Fetch("info.files.0.length");

Perhaps not the best way, but the readability increases(a little)

Stormenet
A: 
  • If you have complete control of your codebase and your thought-process, by all means do.
  • If not, you'll regret this the day some new person injects a BItem derivation that you didn't see coming into your BList or BDictionary.

If you have to do this, atleast wrap it (control access to the list) in a class which has strongly typed method signatures.

BString GetString(BInteger);
SetString(BInteger, BString);

Accept and return BStrings even though you internally store it in a BList of BItems. (let me split before I make my 2 B or not 2 B)

Gishu
A: 

Hmm. I would actually argue that the first line of coded is more readable than the second - it takes a little longer to figure out what's going on it, but its more apparant that you're treating objects as BList or BDictionary. Applying the methods to the abstract class hides that detail, which can make it harder to figure out what your method is actually doing.

John Christensen
+1  A: 

The way I see it, not all BItems are collections, thus not all BItems have indexers, so the indexer shouldn't be in BItem. I would derive another abstract class from BItem, let's name it BCollection, and put the indexers there, something like:

abstract class BCollection : BItem {

      public BItem this[int index] {get;}
      public BItem this[string index] {get;}
}

and make BList and BDictionary inherit from BCollection. Or you could go the extra mile and make BCollection a generic class.

neaorin
The accessor's return BItem's though, so this still wouldn't help any in the long run for the application given.
Matthew Scharley
A: 

If you introduce generics, you can avoid casting.

class DecodedTorrent : BDictionary<BDictionary<BList<BDictionary<BInteger>>>>
{
}


DecodedTorrent torrent = BItem.DecodeFile("mytorrent");
int x = torrent["info"]["files"][0]["length"];

Hmm, but that probably won't work, as the types may depend on the path you take through the structure.

EEK! Ok, this works for this (very) specific example... but in reality, it's a variable tree structure...`torrent["info"]["files"]` is valid. But so is `torrent["announce-list"][0][0]`. Your generics just broke :(
Matthew Scharley
A: 

Is it just me

BDictionary torrent = BItem.DecodeFile("my.torrent");int filelength = (BInteger)((BDictionary)((BList)((BDictionary)             torrent["info"])["files"])[0])["length"];

You don't need the BDictionary cast 'torrent' is declared as a BDictionary

public BItem this[int index]{&nbsp; &nbsp; get { return ((BList)this)[index]; }}public BItem this[string index]{&nbsp; &nbsp; get { return ((BDictionary)this)[index]; }}

These don't acheive the desired result as the return type is still the abstrat version, so you still have to cast.

The rewritten code would have to be

BDictionary torrent = BItem.DecodeFile("my.torrent");int filelength = (BInteger)((BList)((BDictionary)torrent["info"]["files"])[0])["length"];

Which is the just as bad as the first lot

The middle part is added to the abstract class (BItem), hence they have the accessors and don't need to be cast anymore to use them.
Matthew Scharley
+1  A: 

My recommendation would be to introduce more abstractions. I find it confusing that a BItem has a DecodeFile() which returns a BDictionary. This may be a reasonable thing to do in the torrent domain, I don't know.

However, I would find an api like the following more reasonable:

BFile torrent = BFile.DecodeFile("my.torrent");
int filelength = torrent.Length;
Thomas Eyde
Decode and DecodeFile both return BItems, it's just the case in a (valid) torrent file that the root item is in fact a dictionary and hence can be safely cast straight away (if it's an invalid file, the cast exception gets caught by the general exception handling)
Matthew Scharley
Ok, I am fine with that. If I were coding this, I still think I'd add my own abstractions/classes to encapsulate the details. That way I may introduce classes which removes the need for the downcasts.
Thomas Eyde
For the record, I am. But I still need a way of parsing and representing the data as well, since Bencoding is used all over the protocol, not simply for encoding torrent files.
Matthew Scharley