OOP Gone Wild

[yatur]

In a piece of code I inherited from some smart, well meaning, but a little disorganized East European consultants, I found the following class hierarchy:

RestClient (abstract) JsonClient (abstract) XyzClient* SessionClient ApiClient

* XYZ is our proprietary service. The name was changed to protect the innocent.

It’s been

Comments

[dennisgorelik]

> Don’t do this at home

What exactly do you mean?

1) Do not use inheritance - use composition instead?
2) Do not use that many levels of inheritance?
3) Do not keep dead code?
4) Do not inherit concrete classes from concrete classes?

[yatur]

Pretty much all of the above, with (1) corrected to "do not use implementation inheritance". I use it in tests for simplicity sometimes, but very rately in production code.

[dennisgorelik]

What do you mean under "implementation inheritance"?

Is it when class inherits from interface?

Why not to use it? Occasionally it may be convenient.

[yatur]

> Is it when class inherits from interface?

No. Google it.

[dennisgorelik]

Did you delete all these classes from your codebase yet?

[yatur]

No, I did not. I just discovered them today :)

The trouble is, current implementation is not standard compliant, and we need to fix it. But it is being actively used, so the old version must continue to function.

The project is not that big, so I am leaning towards leaving this project alone, creating a new one and pulling everything I need into it, on a strictly "as needed" basis.

I do not want to remove the code only to find later that it was actually used by some kind of stealth reflection mechanism I did not discover.

[dennisgorelik]

> not standard compliant, and we need to fix it. But it is being actively used, so the old version must continue to function.

Do you mean that this inheritance hierarchy is standard compliant and is not in use?

> leaving this project alone, creating a new one and pulling everything I need into it

So you want to rewrite it from scratch?

[yatur]

> Do you mean that this inheritance hierarchy is standard compliant and is not in use?

No. The software as a whole purports to implement certain protocol, but it is not standard compliant. The inheritance hierarchy appears to be dead code that plays no role in the (non)compliance.

> So you want to rewrite it from scratch?

Close, but not exactly. I am going to pull in existing useful pieces, but only those that are actually needed. I've done it before, Joel or no Joel :) The project size is small enough to make it manageable.

[selfmade]

Make sure names of the classes are not referenced anywhere as plain strings for type instantiation, as well as any substrings like var protocol = "Xyz"; InstanceOf(protocol + "Client");
I've seen such tricks.

[dennisgorelik]

If such instantiation code exists, it should be deleted too.

[yatur]

Yep. I am going to start by writing a bunch of integration tests that ensure it does what it is supposed to do.