Code Quality Issues

Everything about development and the OpenMW source code.
Chris
Posts: 1274
Joined: 04 Sep 2011, 08:33

Re: Code Quality Issues

Post by Chris » 02 Aug 2017, 09:34

mattwla wrote:
01 Aug 2017, 23:56
I'm curious how adding a new subclass of MWWorld::Class could mess with current uses of getTypeName.
It would need to ensure all uses of the function correctly handle the new type. If some code gets the type name, the caller is going to do different things for different types, so if it can suddenly be a new type you need to find all places that use that function and ensure it behaves properly with the new type name.
raevol wrote:
02 Aug 2017, 00:05
This sounds like a perfect case to use polymorphism... are we not doing that?
I don't think it would help much, since most types don't have a parent/child (or an "X is a Y") relationship. Actor is the main exception, but aside from that... weapon, book, potion, clothing, ingredient, etc... they're sibling types distinct from each other.

User avatar
raevol
Posts: 2400
Joined: 07 Aug 2011, 01:12
Location: Caldera

Re: Code Quality Issues

Post by raevol » 03 Aug 2017, 01:49

Chris wrote:
02 Aug 2017, 09:34
It would need to ensure all uses of the function correctly handle the new type. If some code gets the type name, the caller is going to do different things for different types, so if it can suddenly be a new type you need to find all places that use that function and ensure it behaves properly with the new type name.
But so....
Chris wrote:
02 Aug 2017, 09:34
most types don't have a parent/child (or an "X is a Y") relationship. [snip] ... weapon, book, potion, clothing, ingredient, etc... they're sibling types distinct from each other.
This is a textbook example of when you should use polymorphism. I don't claim to have extensive knowledge of the codebase, but if we're using classes and not using polymorphism in these kinds of cases, something is very wrong.

Chris
Posts: 1274
Joined: 04 Sep 2011, 08:33

Re: Code Quality Issues

Post by Chris » 03 Aug 2017, 07:31

raevol wrote:
03 Aug 2017, 01:49
Chris wrote:
02 Aug 2017, 09:34
most types don't have a parent/child (or an "X is a Y") relationship. [snip] ... weapon, book, potion, clothing, ingredient, etc... they're sibling types distinct from each other.
This is a textbook example of when you should use polymorphism.
I should clarify. MWWorld::Class is polymorphic. The issue hinted at in number 4, why you shouldn't use getTypeName, is because it moves type-specific behavior out of the type classes. It allows different parts of the code to act differently depending on the object type it has, so if a new object type is added you need to look all over to see what various parts of the code is doing with the type, rather than leaving type-specific behavior in the class that defines the type.

Using polymorphism to deal with the TypeName, as users of getTypeName would, essentially amounts to dynamic_cast, which is bad (because it's slow and inefficient, indicating a design flaw if you need it).

User avatar
raevol
Posts: 2400
Joined: 07 Aug 2011, 01:12
Location: Caldera

Re: Code Quality Issues

Post by raevol » 03 Aug 2017, 08:51

Hmm I see, I am definitely in over my head, and will defer to your expertise. Sorry for the noise!

Post Reply

Who is online

Users browsing this forum: No registered users and 1 guest