kikito / middleclass

Object-orientation for Lua
https://github.com/kikito/middleclass
MIT License
1.77k stars 190 forks source link

Make isSubclassOf and isInstanceOf accept strings #52

Closed timothymtorres closed 6 years ago

timothymtorres commented 6 years ago

First thank you for a pretty robust class library!

I have one helpful feature suggestion for the library that I've used in my projects. The ability to check class names by string without having to require the class object as a dependency. I hope this makes sense?

If I have multiple different classes, and I want or need to use isInstanceOf to check for other classes, I would need to require the classes I'm checking, for every class module. This will unnecessarily bloat the dependencies when all it should need to do is compare against the class name.

kikito commented 6 years ago

While I think this would be technically possible to implement, I don't agree with the reason for doing it:

I would need to require the classes I'm checking, for every class module. This will unnecessarily bloat the dependencies when all it should need to do is compare against the class name.

My counterargument is: Even if you are using them only to check whether they are a parent, that is still a dependency and should not be treated differently from using the parent class in other ways. Your subclass is coupled with this ancestor, and your code should make it explicit with a require.

Another counterargument is that you should be using isSubclassOf and isInstanceOf sparingly. If you use them often (so that doing a require is painful) there is probably another way of writing your program without them. Could you describe how you are using them, right now?

timothymtorres commented 6 years ago

Another counterargument is that you should be using isSubclassOf and isInstanceOf sparingly.

Indeed. But as the size of my project has grown I have noticed more use of those methods, when previously there was no need to call them.

Even if you are using them only to check whether they are a parent, that is still a dependency and should not be treated differently from using the parent class in other ways. Your subclass is coupled with this ancestor, and your code should make it explicit with a require.

I agree with this counterargument but under the assumption that a developer is using isSubclassOf to search for a parent class. In which case, the parent superclass dependency would be a necessity. (this would probably apply to majority of use cases)

Under a different set of circumstances this may not be applicable. In my case, I am passing around an instance of an object as a parameter to different classes and checking to see what kind of object it is with the isInstanceOf method. Loading the different classes of potential objects as dependencies is optional. (although for some areas it might be a really good idea)

There is also the possibility that object methods are not being used themselves but the object may be getting stored, deleted, or something else.

There are over +40 items in a game I am making. The player inventory does not use any of the item methods, with the exception of one function. It merely stores and manipulates the data location for items. But sometimes I need to search an item for a specific type via a string arg. I see no point in requiring every item subclass or even the item class itself. Here is a line of code regarding this,

https://github.com/timothymtorres/ZomboTropolis/blob/develop/game/code/player/human/inventory.lua#L27

In a 2nd example, I pass the object instance as an arg, where it could be possibly one of two types of classes. (Player or Building) This time I do use the object methods to manipulate the object in the module. I don't see how adding Player or Building as a required dependencies helps? The instance already has all the methods being used. Even if I did require the classes themselves, they would not be utilized. (unless using isInstanceOf to check the Class itself instead of the string that I'm suggesting here)

https://github.com/timothymtorres/ZomboTropolis/blob/develop/game/code/server/network.lua#L47

timothymtorres commented 6 years ago

That pull request is how I implemented it in my project. I'm fairly certain it doesn't break backwards compatibility either. Feel free to decline or make changes to it. I found it useful feature for minor cases so I thought it was worth sharing.

kikito commented 6 years ago

I have decided not to include this functionality in middleclass, I explained my reasons on your PR. I am very thankful for the effort you invested in trying to improve middleclass nonetheless. Maybe next time!