2

I am new to programming (self teaching Javascript so I am missing some of the basics). I'm not sure if this falls under a style or best-practice question.

When writing a function that will be used often, and that has many slightly varied output expectations, should an internal if-statement be run to determine the output, or should the function be broken down into specific functions.

eg. What is better (try to ignore the contents!)

1:

function changeColour(colour){
  this.removeColour();  
  switch(colour){
    case "red":
      this.colour('red');
    break;
    case "blue":
     this.colour('blue');
   break;
 }
}

or

2.

function makeRed()
function makeBlue().. etc
  • 3
    In option 2, how do you decide which function to call? (I'm trying to make the point that somewhere you are still going to need that switch clause). – Rotem Nov 10 '14 at 11:44
2

In this case - a simple function that takes only one argument and can be handled with a simple switch statement - you should do that. You gain nothing from breaking this down into separate functions except extra code to maintain. A function should perform one operation clearly and succintly - a simple switch statement such as yours certainly does not break that rule.

By breaking your logic into specific functions for the different colors, you also weaken your design by complicating your program logic in other places: When you change colors, you will have to arbitrarily know (never good) or include logic to determine which function you need to call that point. With your parametrized function it will be much easier to call the function at run-time and dynamically manipulate colors without having to know which particular function you need to call.

Do a "cost/benefit analysis" when designing your functions: Is this function becoming too long and complex? Is trying to do too many things at once? Does it correcly express the idea you want to implement? Will the next developer who deals with this code understand it easily? Be able to modify it easily? Use it in other situtations? Your current function with the switch meets all these criteria (except for the last one - more on that below.)

However, what does merit some consideration is your first line:

 this.removeColour(); 

I understand that you need to remove the old color before setting the new one and it is convenient to wrap the call to removeColour together with setting the new color. But this might be an example of what we call "bundling" - combining several not necessarily related operations into one function.

In your case, this is probably a non-issue - you must clear the old color - AFAIK (I am not a JavaScript guy) - before setting the new one, so removeColour belongs in this function - it is an essential element of the changeColouroperation. So my point here is simply didactic. But it is all too common to find code that is guilty of "bundling" in such manner when that is not the case: Simply bundling a group of unrelated operations together for convenience's sake.

Such a practice leads to difficult to maintain, not robust code: The time will come when you need to execute only one or some of the unrelated operations, and not all of them. When that happens you will have to either duplicate code or change the bundled code and break it out into separate functions - extra work/time/money. That is one (of many) sorts of scenarios when you should be thinking about writing separate functions to handle different operations.

Something else to consider is this line:

this.colour('blue');

Perhaps another example of "bundling": By setting the color of this within this function, you have restricted its scope and usability: Now the function only works for this but not for other widgets. You might want to consider just having the function return a string with the name of color.

return 'blue'...

And have the caller use the result to set this.colour(colorName):

colorName = changeColor(blue);
this.colour(colorName):

Then you (or someone else) could also write:

colorName = changeColor(red);
aWidget.color(colorName):

etc..

Should you do so, besides having to modify the function to return a string and dealing with the removeColor operation, you should probably change the name of the function to GetColorName(colour) - because you are not actually changing the color in the function, but simply getting the string representation of the desired color. A function name should clearly describe what the function is doing.

0

Although, to some degree, the choice of how granular to make the function definitions is to do with how you feel about readability, it also to do with maintainability. If you end up with a codebase that has a lot of functions that all do a very similar thing (in your example, the latter level of granularity) then if you have to make a change to the way the repeated functionality works you'll have to go into every one of the similar functions to make the same change and you might just miss one.

If you can put this similar functionality into one readable, short function, then I would be tempted to do so. Just make sure it is short and (as an arbitrary example) fit within 50 lines os so and encodes only one main idea.

0

Usually, you want to have exactly one place where your actual code is executed, which is what your first example is. However, sometimes you want easier code to both read and write. In your case, you might want to do something of this sort:

function changeColour(colour) {
  this.removeColour();  
  switch(colour){
    case "red":
      this.colour('red');
    break;
    case "blue":
     this.colour('blue');
   break;
 }
}
function makeRed() {
  changeColour("red");
}
function makeBlue() {
  changeColour("blue");
}

This way, makeRed() and makeBlue() both have guaranteed results and are easy to read and write. Additionally, if there's a bug in your code, you're likely to squash it by only looking into changeColour(colour), which saves time and helps make your code bug-free.

We use this same practice with languages like Java, which only allow a fixed number of parameters per method, in order to make several methods with different parameters that do the same thing. For more, see https://stackoverflow.com/a/2632401/3939277, which does a good job of outlining this practice.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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