✅ Refactoring A Class With Too Many Responsibilities
Right now, I am trying to refactor a level editor class, which currently has way too many responsibilities, and I want to fix that. My core issue comes from trying to restrict access to certain methods between my level editor and other scripts. For example, here are methods for drawing a tile in a level (in my LevelEditor class):
-private DrawCommand (what to do when user issues a command to draw at the target position, including validation logic. Calls DrawWithLog)
-private DrawWithLog (try to draw a tile while logging changes to undo history, calls DrawAtRegion)
-public SmartDrawIngame (force draw, but also funnels logic through some singletons that may need to update internals, Calls ForceDrawIngame)
-private ForceDrawIngame (outside level edit UI, other scripts can request drawing at a specific spot using other private methods. Calls DrawAtRegion)
-private DrawAtRegion (actually erases anything that conflicts in different tilemaps, and draws. Calls SetTile)
-private SetTile (draw the tile and associated metadata)
There are several other features like this, and I want to split up responsibilities across a couple of classes (so I can also add other features). Any recommendation on how to split this without exposing everything publicly?
6 Replies
tl;dr class with too many responsibilities. How to split without effectively making everything public (given code is too coupled for internal to work)
refactoring is always an interesting problem, but i don't know what kind of advice you really can get on this;
usually refactoring involves applying stuff like single responsibility principle, finding well known workflows, isolating components/behaviors;
i would probably start at the top or at the bottom, for example if SetTile is where the rubber touches the asphalt then that method could go in a separated class with other strictly low level methods; or maybe it's the reverse, the class should be only for low level methods and it's the others that should go in separated classes;
but also the fact that in the same class DrawCommand has validation logic seems out of place;
or for example if DrawWithLog interacts with undo history then there's to analyze if there is enough indirection with stuff that manages undo;
also what "without exposing everything publicly" means, i mean you have to have some public methods, at most you could have internal classes, but is there a real issue here? a class with a public SetTile method would be fine to me (except maybe for the name, what does 'Set' means? add? update? it's kind of a vague prefix);
last but not least, are you already using some kind of overarching patterns/architecture?
thanks for the input.
I wanted to keep a lot of these methods private to avoid inadvertently making low level modifications without triggerent events that depend on modifying the level. But i can’t do this if I want to split these responsibilities.
The method names aren’t the actual names. I’m also going to modify the function names a bit as I split them up.
In terms of overarching pattern, the only major pattern here is singleton. This class is a central point of contact to block multiple classes from attempting to modify the level without proper validation.
This is not to defend my choices, but to explain how I got into this situation.
two of the ways to avoid calling methods you shouldn't call are using interfaces to hide the implementation and moving a class in a separate project so that it can be called by other classes in the same assembly
but the main tool remains discipline, avoiding shortcuts; there is no alternative to that, probably
i think breaking up my class into several classes with different responsibilities is best, so one class is just known as a general point of contact for (editor mode changes), another is a point of contact for (non-editor mode changes), and a third contains common toolbox functions for both (and is clearly not to be accessed outside of that namespace)
ty for the advice
(if you want to close the thread use
/close)