   Odd bit of refactoring - need some expert help
  I'm in the process of refactoring some of my code, and have been doing it as per the book I bought, Refactoring to Patterns, as well as a couple of websites. So far I've only done simple stuff, splitting longer methods into several smaller ones with the same granularity and so on, which is lovely, but I'm now at a spot where I don't know what to do next - it seems that there are conflicting refactorings I can perform. Using a very simplified, psuedocode-ised form of my actual code:class Building {    void update(float dt) {   if(state 1) {  do some stuff to the physical part of the building  also do some stuff to the graphical part of the building   } else if(state 2) {  just do stuff to the logical part of the building   } else if(state 3) {  do stuff to all the bits of the building   }    }};There are plenty of other methods like update, some of which have conditionals based on state, some of which deal with different parts of the building.As far as I can see, there are two big smells I'd like to remove: Firstly, Building is responsible for too much; physics and graphics and logic should be in separate classes. Secondly, the conditional logic has to go.The refactorings to remove these smells are most likely to turn Building into a composite of components, and to turn the conditional logic into the strategy pattern.What I can't get my head around is what order to do these in, and what the result will look like - the strategies will each need to deal with all the components, and all the components will be modified by all the strategies. Won't this look really ugly? Should each component have its own strategies? If that's the case, won't the number of classes skyrocket? Also bear in mind that Building will be a superclass once I've refactored it; there will be other types of Building that have slightly different appearance and behaviour. Should this be approached using decorators or more finely grained classes?What do you chaps think I should do?   
  Refactoring is a code issue, not really a design problem.Without code, there's nothing to be said.With TDD, you write tests, then code implementation to the tests. If there's a design problem, you fix the tests first, then fix the implementation.With Agile, there's not much emphasis on design. Find the simplest solution to your problem, then code from there.Waterfall model has rigid interfaces and classes. As long as you don't muck up global namespace, you're free to do whatever you want.There is no "right" way. The whole point of refactoring is to work exactly with the code you have. Not a bit more, not a bit less. To be strict - when you refactor, you first make a unit test, refactor, make sure original test still runs. Anything you change in between is not the scope of the discussion.If you think you have design issue, solve it at design level through unit tests, then come back to code to fix it. If there's code issue, don't touch the design, and just improve the code.All this smell and various hype-babble distracts from real issue, and tends to attract people with too much time - you're solving a problem. Unless there's a reason (adding a new feature, performance, etc.), you're wasting time by polishing part of code that doesn't matter.That's a lesson often missed. Purpose of refactoring and Agile approach isn't in producing sleek code, but writing code in such a manner that it's easy to change, along with demonstrating several tools and techniques with which to perform these changes with minimum effort.   ;  Traditional OO wisdom usually says that switch statements like that should be converted into individual classes and polymorphism. However I think this only makes sense when the individual cases are relatively isolated and don't communicate between them much. Otherwise a few switch statements can be much more readable and maintainable.Put bluntly, I don't think it's possible to judge what to do with that code without going into a lot more detail describing the system and what it needs to do.   ;Quote:Original post by AntheusAll this smell and various hype-babble distracts from real issue, and tends to attract people with too much time - you're solving a problem. Unless there's a reason (adding a new feature, performance, etc.), you're wasting time by polishing part of code that doesn't matter.Ah, well that's the thing; The purpose of me doing this is to produce some beautiful code as a sample for an interview. I could go in there and say "It's crap but does the job", but I don't think they'd be impressed with that ;)I understand it's a bit difficult to give advice on code without seeing it, so here's a particularly horrible uncensored sample:void Building::update(float dt) {Ogre::Root* root = Ogre::Root::getSingletonPtr();double timeNow = root->getTimer()->getMilliseconds() / 1000.0;if(mState == Free) {if(timeNow > mTimeSetFree + mMaxFreeTime)mWorld->removeBuilding(this);} else if (mState == Controlled) {if(mHeight <= 0.0f) {if(canBuild())attach();elsesetFree();}} else if(mState == Attached) {float jointDamage = mJoint->getAccumulatedDamage();if (jointDamage > mJointDamageLastFrame)mJointDamageLastFrame = jointDamage;} else if(mState == Controlled) {// translate to appropriate coordinate and height// remember that it should be relative to the Island scene nodeOgre::Vector3 t    = mIsland->getSceneNode()->getPosition();Ogre::Quaternion r = mIsland->getSceneNode()->getOrientation();opal::Matrix44r transform;transform.translate(t.x,t.y,t.z);transform.setQuaternion(r.w,r.x,r.y,r.z);transform.translate(mIsland->getPosition(mXpos), mHeight + mIsland->getHeight(mXpos,mYpos),mIsland->getPosition(mYpos));mSolid->setQuaternion(r.w, r.x, r.y, r.z);mSolid->setPosition(transform.getPosition());}if(mState != Controlled) {// We no longer want to build/set free the building on collisions, so delete the handlerif(opal::CollisionEventHandler* h = mSolid->getCollisionEventHandler()) {mSolid->setCollisionEventHandler(0);delete h;h = 0;}}}Off the top of your head(s), what changes would you make to that to make it nicer? I'm not asking you to write code for me, just some pointers would be nice ;)   ;  Have I misread a brace somewhere or are you doing:} else if (mState == Controlled) {twice?   ;  Do you envision adding more states in the future?  Is this a multi-threaded application?  Do you need to store your state for future use?--random   ;  In this case, the switch isn't to identify different kinds of objects (which would be polymorphism), but to identify different states of the same object.Maybe you should look at the State pattern instead of thinking "ah, switch means I need to do Replace Conditional with Polymorphism".   ;Quote:Original post by hymermanI understand it's a bit difficult to give advice on code without seeing it, so here's a particularly horrible uncensored sample:*** Source Snippet Removed ***Off the top of your head(s), what changes would you make to that to make it nicer? I'm not asking you to write code for me, just some pointers would be nice ;)Hmm... I don't think thats too bad really. High-level gameplay objects often tend to end up looking something like that since they've got to keep their representation in the various subsystems (like physics and graphics in this code) in sync with each other.You might be able to extract another class to manage the lower level graphics calls ("BuildingVisual" or something) and hold one of those as a member, but since the graphics calls are all pretty simple I don't think you'd gain much from it. About the only thing I'd do is wrap the translation/rotation code up in it's own function.   ;Quote:Original post by AntheusRefactoring is a code issue, not really a design problem.Spoken as though you don't refactor.Quote:Original post by OrangyTangTraditional OO wisdom usually says that switch statements like that should be converted into individual classes and polymorphism. However I think this only makes sense when the individual cases are relatively isolated and don't communicate between them much. Otherwise a few switch statements can be much more readable and maintainable.Put bluntly, I don't think it's possible to judge what to do with that code without going into a lot more detail describing the system and what it needs to do.I don't know about that traditional wisdom (never seen it put that way), but as I recall from MY traditional OO wisdom: OO does recommend avoiding switch statements by breaking things down into components that can interact with minimal dependencies between them. Note the subtle difference between my traditional wisdom wording and yours. However you are right on at the end: details are very important.Quote:Original post by legalizeIn this case, the switch isn't to identify different kinds of objects (which would be polymorphism), but to identify different states of the same object.Maybe you should look at the State pattern instead of thinking "ah, switch means I need to do Replace Conditional with Polymorphism".Yeah, refactoring to patterns is a great boon here. As with most things dealing with code and code cleanup, refactoring also deals with code design. Some people seem to overlook the design aspect and focus more on the "cleaning up code" part, but in reality the two are one and the same. I've found over the years that code tends to naturally evolve torwards certain patterns as you refactor. While explicitly implementing those patterns when first noticed is a really big temptation, I tend to avoid such behavior as often the refactoring will improve the resulting pattern by addressing the problem's needs in a clearer manner. Very rarely do I explicitly apply patterns anymore, tending to use more TDD/Refactoring principles to let my code naturally express the patterns that best fit the problems it faces.   ;Quote:Original post by WashuVery rarely do I explicitly apply patterns anymore, tending to use more TDD/Refactoring principles to let my code naturally express the patterns that best fit the problems it faces.Agreed.  I like the "emergent design" aspect of doing TDD and incrementally refactoring as I go.There is one situation where it helps to explicitly apply a pattern though: refactoring old code.  I've been refactoring a huge spaghetti mess of C code into a collection of loosely coupled C++ components.  Its a tough slog.  In this case, sometimes it is easier to say "yeah, this is really a Strategy pattern implemented with a shitload of if statements shotgun blasted throughout the source base" and purposefully refactor things to Strategy.Refactoring to Patterns is a great reference for this.   