Dear Atul,

The chain of responsibility was an interesting idea but IMHO in our specific case it does not contribute to a simpler solution. I like your idea to have the BItem derived classes read themselves. Why don’t we simply use a factory method like we use PacketFactory with packets? Won’t that be simpler and achieve the same effect?

The attached patch includes your suggestion to get rid of the ItemType member in the base class, my suggestion to use typeid() instead of type(), your suggestion to put the read functions in the corresponding classes and my suggestion to use the factory method. It’s your turn now ;-)

I also removed all castFrom() static functions. There were 32 of them and only one was used once and I don’t think anymore that they make much sense. What happened is that most of the functions were moved out of the BDecoder class. I’m wondering whether we can delete it.

I feel like there are too few tests in BDecoder. Now it would also make sense to create unit tests for each BItem subclass.

In the tests I had to rewrite some code because type() is now removed. After a check that the object is of the correct type I do dynamic_cast which performs the same check again. Faster would be to perform static_cast but my feeling is that we’d better use dynamic_cast and keep a higher level of safety because if a typeid() check ever gets removed then the static_cast is not safe anymore and may crash the software. What so you think?

A lot of the old code has been deleted and there is only little new, so that seems like a good refactoring work ;-)

Best regards,

Peter

atul wrote:Hi all

Here is how the pattern would help in clean up BItem… BItem::type() and related stuff is not necessary

All tests pass ;-)

1. I also plan to give a close look to isMyMarker() so if ‘e’ marker is handled consistently ( for all most all cases )

2. Some ctors could be made private

Also this has the original code… The improved readInt will further shrink the code and make it more readable…

But this illustrates the concept….

Let me know

—- cheers atul