current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 6 down vote favorite

I have a large file that I want to refactor. My idea is to make helper classes with helper methods so that I can modularize methods.

Is that a good idea? For instance before refactoring:

public void collision(PhysicsCollisionEvent event) {
    if (event.getObjectA() instanceof BombControl) {
        final Spatial node = event.getNodeA();
        effect.killAllParticles();
        effect.setLocalTranslation(node.getLocalTranslation());
        BombControl bc = (BombControl) event.getObjectA();
        effect.emitAllParticles();
    } else if (event.getObjectB() instanceof BombControl) {
        final Spatial node = event.getNodeB();
        effect.killAllParticles();
        effect.setLocalTranslation(node.getLocalTranslation());
        effect.emitAllParticles();
    }
}

After refactoring:

public void collision(PhysicsCollisionEvent event) {
    CollisionHelper.collisionHelp(event, effect);
}

Is that a good idea or must I use a more elaborate recfactoring? I think it would work but I don't know of specific refactoring patterns.

share|improve this question
1  
In response to "I don't know of specific refactoring patterns," sourcemaking.com/refactoring provides a handy guide to many different types of refactoring. –  cbojar Jun 20 '14 at 18:37
1  
Is that file your entire app, or do you already have other classes? –  Winston Ewert Jun 20 '14 at 18:46
    
I'm not sure which Java version you are constrained to use, but since Java 7 you can switch on String instead of all the else if in onAction. –  toto2 Jun 21 '14 at 1:22

2 Answers 2

up vote 5 down vote accepted

A couple of simple things I notice right off the bat.

  1. If you have a method which only calls another method, why bother having the first method at all? I'm personally against these one-line wrapper methods and see them as unnecessary noise.
  2. Your methods should be verb phrases, in general (e.g., CollisionHelper.helpCollision or even performCollision, in this case, since it's doing all of your processing).
  3. I believe there's a standard for replacing instanceof usage with an actual method from the Class API, as follows: BombControl.class.isInstance(event.getObjectA()). You can see more on this Stack Overflow Q&A. (Apparently you're using it correctly here, however.)

From the comments: you obviously should move the entire huge method body to the helper method, otherwise you have the same problem. Just extract 'til you drop.

share|improve this answer
    
Thank you for the answer, I suppose you didn't say any obvious pitfall my my "idea" where some other methods in the class are very large and can be modularized into helper methods that are in helper classes so that one method that is 800 rows now can call 8 methods in 8 helper classes so the original files gets more concise. –  Programmer 400 Jun 20 '14 at 18:40
1  
isInstance() is for when you have am instance of Class and don't know the exact class type. In this case, you know the exact class you are checking the type against, so instanceof is correct. It is also more legible than accessing the class variable. –  unholysampler Jun 20 '14 at 18:43
1  
@909Niklas: Are these helper classes going to be used by many different classes? If you just want to break a large method into smaller methods, there is no reason to move them into a different class. Just mark the sub-methods as private. –  unholysampler Jun 20 '14 at 18:47
1  
@909Niklas Sure. If you pull out the entire method body into a helper method, then you have the same problem. But if you keep extracting until you drop, you should be good. :) –  asteri Jun 20 '14 at 19:11
1  
@unholysampler Thanks for that clarification. –  asteri Jun 20 '14 at 19:13
up vote 3 down vote

I also hate large files. My rule of thumb is a maximum of 300 lines per class.

You mentioned that you wanted to put methods in helper classes. Also you said you were not sure about refactoring patterns.

Use the simplest refactoring: OO!

You should not just break things into helper methods and helper classes, but into real classes. You have a full page of member variables declaration at the top of UFOSpaceWorld; those variables should be divided to appropriate classes. It is always difficult to correctly break a program in classes. You might have to try many different ways of breaking things down in classes before you find something that is satisfying. Make sure you use git or a version control system so that you can always go back.

share|improve this answer
    
Thanks for the answer. I use xp-dev for svn repo, chatted with the author of the library and we now have decent physics code. –  Programmer 400 Jul 4 '14 at 14:46

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.