-2

Say you have a Car class. Properties that make sense for a Car class might be:

var make;
var model;
var year;
var turnOn; // a function

But the turnOn function is very complicated and ends up needing a static variable and a couple sub functions:

var isTurningOn; // too specific to be a class variable
function turnOn() {
    // 30 lines to start the engine
    // 30 lines to start the air conditioner
    // 30 lines to start the radio
}

So now we have a function that maybe isn't really suitable to be made into its own class and it's polluting the Car class with a variable that only the turnOn function ever uses. Plus the function contains sections of code that should be made into their own functions. So what do you do in situations like this?

user1873073
  • 97
  • 2
  • possible duplicate of [Is it a bad idea to create a class which will only have one instance?](http://programmers.stackexchange.com/questions/213343/is-it-a-bad-idea-to-create-a-class-which-will-only-have-one-instance) – gnat May 05 '14 at 04:21

4 Answers4

3

If a variable is only needed by a single function, the variable should be in the function that uses it. This will help to keep the rest of the class clean. If the rest of the class doesnt use that variable, it shouldnt have to ever care about that variable or know it even exists.

As for the 30 lines each to start the various components, like engine, radio, etc., do you think those components should really instead be in their own class? That would mean the "turnOn" function is trimmed down to 3 lines of code (or 1 line per component to turn on). The car class then also shouldnt be responsible for creating the components like engine, etc. Instead, a constructor of the Car class should be created to accept all of its components as parameters. When you think in the real world, car doesnt create its engine, a factory does. So you could create an "EngineFactory", "RadioFactory", etc. class to create the individual components. All of those components would then be passed to a "CarFactory", which just calls the Car constructor with all of its components.

jordan
  • 501
  • 2
  • 6
  • 1
    Cars usually have ignitions that handle various parts of the process. Modern cars also have engine computers that help get the engine started, and other electronic systems to take care of turning on the radio, AC, etc. The abstractions that you'll be building in your code are presumably for a purpose-- let the purpose guide the level of abstraction you need, and worry less about a 120+ line function. Once you can see how this Car class is actually going to be used, then you can decide how to solve the 120+ line function problem by making the abstractions fit the use. – RibaldEddie May 05 '14 at 03:13
  • Note I never said I was worried about the 120+ line function. But now that you mention it, 120 lines does seem too long for a function :) – jordan May 05 '14 at 03:21
  • The OP did. " Plus the function contains sections of code that should be made into their own functions". – RibaldEddie May 05 '14 at 03:28
  • Ah, gotcha. I thought your comment, including the part to "worry less about a 120+ line function", was directly towards my comment. – jordan May 05 '14 at 03:30
  • Sort of. I just blabber. – RibaldEddie May 05 '14 at 03:42
1

Most of the existing answers sound like they're heading toward Needless Complexity.

I agree that the turnOn function doesn't need to be in its own class, however isTurningOn makes sense as an instance member variable, because it tracks part of the state of each Car instance. Split turnOn into shorter, more specific private methods and leave it at that for now.

Keep things simple until you find that adding complexity simplifies something else. If your Car is just as you describe it: a couple of member variables, a state variable, and a turnOn function, there's absolutely no reason to split it into multiple classes. It's just not that complex yet.

Mike Partridge
  • 6,587
  • 1
  • 25
  • 39
0

120+ lines of code requiring sub-functions could easily become it's own vehicleStarter class rather than needing to be embedded. This is likely the approach I would take here.

If you are committed to directly embedding the function I would just put the function's variables within the body of the function which typically scopes them to just that function in most platforms and at least indicates that these are local variables to readers. As for sub-functions you can handle that in different ways depending upon platforms. If the platform supports functional programming concepts then you can usually find a way to declare a variable as a function and effectively treat them as variables. This certainly works in javascript which is what I'm guessing you are writing based on your example.

Wyatt Barnett
  • 20,685
  • 50
  • 69
  • Splitting off a `VehicleStarter` class from a `Car` creates the possibility that there might exist a `Car myCar` such that `myCar.starter.car != myCar`, or a `VehicleStarter myStarter` such that `myStarter.car.starter != myStarter`. Sometimes that may be a useful, but such splits increase complexity. I would posit that the question of whether to split off components should depend more upon the relationships among different aspects than upon the amount of code involved with them. – supercat May 05 '14 at 02:45
  • Since cars are actual things in the world, it might be helpful to talk to someone who knows how cars actually work. So your model of a car might need to be pretty complex if you care about getting the abstractions right. Abstractions are more important then whether a single function is 120 lines or not. It's better to realize that you need to work on your abstractions than it is to worry about a 120 line function. – RibaldEddie May 05 '14 at 03:10
0

At least for the example you have given it seems approprioate to use the state-pattern to describe your Car class best.

When creating the car you not only pass the needed dependencies (like the engine, when it was created and so on) as arguments into the constructor but also the very first state implementing an Interface ICarState. You could call that for example NoActionState and save it into a private class member like the year or model.

Later when your car is started, you dont change an isTurningOn variable but replace the NoActionState instance with a new instance of EngineStartingState. The advantage is now that you don't have to use many conditions to check for private variables like isTurningOn to react proper to the given state but you can instead let your state classes implement the desired behaviour.

And when your car has the ability to start then i guess the next state after it will have started would be the DrivingState. Another advantage is that you can now also add new states when new functionality is needed without having the change the logics in the other state classes.

This way you solved your problem in a way that the state information you used your isTurningOn variable for is now saved in a state variable containg the current state of your car and the more detailled information for the current state is saved in its own class.

valenterry
  • 2,429
  • 16
  • 21