1

I'm writing a BitTorrent client as a personal project, and the client includes a parser for the Bencode encoding format. The parser stores different types of encoded values (ints, strings, lists, dictionaries) as subclasses of a Bencode abstract class so that I can nest values cleanly (e.g. list is a vector<Bencode *>).

According to the BitTorrent specification, all the data in the ".torrent" file is in a Bencoded dictionary, meaning if the file I got is parseable, I want my Bencode * object to have a subclass dictionary, if not I would like to reject it.

I can do this easily enough with a dynamic_cast as I read in this question: How do I check if an object's type is a particular subclass in C++?

However, one of the answerers to this question suggested that if you have to check the type of a subclass, it probably means there's a design flaw. I can't figure out a clean way to do this otherwise though. Is checking the type bad in this case, and if so why and what are the alternatives?

tadman
  • 208,517
  • 23
  • 234
  • 262
Ramoose
  • 11
  • 2
  • 1
    If you're doing this properly, using object-oriented *design*, then you shouldn't care what particular type it is. Any dependencies in this regard are usually a sign of an incorrect or incomplete design. That answer is correct. Maybe you can add some code for context to help illustrate your particular concern. – tadman Jul 03 '23 at 21:43
  • Normally you'll have a base class of something "serializable" and then subclasses implement the required functions. From there you shouldn't care what you're dealing with, it'll automatically load/save, with one exception: You'll likely need a [factory method](https://en.wikipedia.org/wiki/Factory_method_pattern) to instantiate the correct variant. – tadman Jul 03 '23 at 21:45
  • PSA: Instead of using bare pointers in your vector, consider using a wrapper like [`std::unique_ptr`](https://en.cppreference.com/w/cpp/memory/unique_ptr) which will help manage memory. – tadman Jul 03 '23 at 21:47
  • @tadman My parser takes a file and outputs a Bencode* (i.e. abstract class), and there shouldn't be any restrictions on the subclass type here since in general any of them would be a valid output. However, the file for the particular application I am parsing it for should only have type dictionary, if not it isn't formatted correctly. If this still isn't clear let me know and I'll add some code to try to clarify. – Ramoose Jul 03 '23 at 21:54
  • If you're trying to ensure the structure of something is correct, I'd add a simple `type_id` type property to your base class, and then something that can boil that down to an easily compared structure like `std::vector` If that structure doesn't match expectations, format is invalid, etc. – tadman Jul 03 '23 at 21:55
  • @tadman Could you explain how using a `type_id property` would be preferable to checking type directly? It seems to me that both of these things violate the "Tell, don't ask" principle referenced in the other question. Edited for clarity. – Ramoose Jul 03 '23 at 21:57
  • Bad: Using dynamic type checking to determine types. Good: Using a well-defined API that returns, say, an `int`, or better, an `enum` type you can trivially inspect and compare without worrying about pointers or other mess. For example, in your abstract base class: `virtual BencodeType type() const` which you'll then implement to return the correct `BencodeType` for a given subclass. In your test case you'll want to look for an array containing only, say `BencodeType::Dictionary`. – tadman Jul 03 '23 at 22:00
  • The reason this code is better is because you can then extend this arbitrarily in the future without breaking any existing code. New subclasses just slot in relatively seamlessly, and if you have two implementations of what is effectively the same data type, their `type()` return value can be identical. – tadman Jul 03 '23 at 22:01
  • 1
    One alternative (or addition, they are not mutually exclusive) could be to have `virtual bool is_dictionary() const { return false; }` (+ the 3 other) implemented in the abstract base and override the correct function in your concrete classes to return `true`. – Ted Lyngmo Jul 03 '23 at 22:07

1 Answers1

2

If you find yourself writing

if (dynamic_cast<SomeSubtype*>(myObjectPtr))
   doSomething();
else
   doSomethingElse();

then you may consider adding a virtual function (call it doIt) to the class of *myObjectPtr. The default implementation should doSomethingElse. Then override it in SomeSubtype to doSomething.

Now the ugly if becomes just

myObjectPtr->doIt();

Of course with a purebred OO design you should never find yourself writing such a thing, but purebred OO designs rarely survive in captivity.

If you find yourself doing the above replacement routine so many times that your interfaces become unmanageably big, you probably need to break up your classes into smaller units, each with a limited set of responsibilities.

P.S. Some people argue that any if (or switch) statement at all should be similarly refactored, possibly by introducing a new purpose-written class. This is something I have never tried myself.

n. m. could be an AI
  • 112,515
  • 14
  • 128
  • 243