1

According to Origin of "a method should return a value or have side-effects, but not both", a method should either return a value or have side-effect, but not both. However, I often see some situations that I need both the result of modify objects and the process of modifying it, for example, a function that modify an array, and the steps that involved during modifying:

function customSort(myArray){
    let stepCount=0;
    //some sort steps to modify myArray
    return stepCount;
}

const stepCount=customSort(this.myArray);
this.showAnimation(stepCount);

So, should I separate it into 2 methods:

function customSortReallyModifyArray(myArray){
    //some sort steps to modify myArray
}

function customSortCountSortStepOnly(myArray){
    const copy_myArray=JSON.parse(JSON.stringify(myArray));
    let stepCount=0;
    //some sort steps to modify copy_myArray
    return stepCount;
}


this.customSortReallyModifyArray(this.myArray);
const stepCount=customSortCountSortStepOnly(this.myArray);
this.showAnimation(stepCount);

even if it would cause the "sort step" run twice?

wcminipgasker2023
  • 1,505
  • It's not exactly the question you are asking but it's important to note that the 'running it twice' solution could introduce TOC/TOU issues. This kind of bug can also result from being overly aggressive about avoiding the use of variables. – JimmyJames Jul 27 '23 at 18:14

5 Answers5

7

The trouble with trying to follow everybody's idea of what is good coding principles is that you end up unable to actually write anything without violating at least one of those principles. Repeating the same calculation for no good reason violates the DRY (Don't Repeat Yourself) principle.

It's good to distinguish between "functions" that calculate things but have no side effects and "procedures" that do have side effects. Some languages even have two keywords for these.

But sooner or later you find that a procedure needs to report the results of whatever it has done. And if you strictly follow a rule that procedures have no return value, then you end up resorting to ugly work-arounds.

Simon B
  • 9,621
  • TL;DR - Its worth giving this principal some thought - often there will be a way to refactor your code so that it is both more readable and conforms to the principal, however there are also times that a method that both side effects and returns a value is the most readable option. Note: you should also think about the readability of both your code and the calling code. – DavidT Jul 11 '23 at 09:02
  • -1 until this misleading note about the DRY principle gets fixed. The OPs code shows indeed a potential violation of the DRY principle, but not because it repeats the same calculation twice. It is a DRY violation because customSortReallyModifyArray and customSortCountSortStepOnly are probably both implementing parts of the same logic twice (actually, we do not know this for sure, since we don't see their implementation). – Doc Brown Jul 27 '23 at 11:21
  • ... let me say I think this answer contains good advice against following such rules blindly. Pragmatism should always beat dogmatism. Still, in this case, I think there is a solution for this case which isn't too ugly – Doc Brown Jul 27 '23 at 12:42
1

You can follow the principle easily, and still don't have to run or implement the operation twice, by using an object encapsulating the operation:

 sorter = {
     stepCount: -1,
     customSort: function(myArray){
          this.stepCount=0;
          //some sort steps to modify myArray and increase stepCount
     }
 }

Now you can use it like

sorter.customSort(this.myArray);
this.showAnimation(sorter.stepCount);

This will

  • eliminate the need for customSort to return anything directly
  • avoid the need for running the (postentially costly) sort operation twice
  • avoid the need for implementing a separate sort count (as in your question)
  • avoid the need for an ugly second argument (like a callback function) in the signature of customSort
  • keep the sort algorithm "in-place", opposed to this solution

Note this can be easily extended using more member variables. Also, a complex operation like a sort algorithm often consists of more than one function internally, so this gives you a natural place where to put those private functions.

Doc Brown
  • 206,877
  • 1
    +1 this also eliminates the potential risk of TOC/TOU issues. – JimmyJames Jul 27 '23 at 16:37
  • @JimmyJames: yes, and did you notice, it also avoids the temporal coupling issue which the OPs question contains - most probably unintentional: calling customSortCountSortStepOnly after customSortReallyModifyArray will result in a stepcount of zero - so in their final example, the calling order needs to be switched to make the program work properly. – Doc Brown Jul 27 '23 at 18:20
  • Good catch. I love this kind of refactoring for a lot of things. My only qualm around not returning anything on a mutable call is that it precludes method chaining. I can understand why it's good rule, though. I often find myself wishing 'void' methods would implicitly return 'this' in OO languages. – JimmyJames Jul 27 '23 at 18:26
  • If a method returns this/self to support method chaining then it’s not really a return value. – gnasher729 Mar 20 '24 at 08:28
0

It's not clear that your method actually has side effects in this case.

All we can see is that it modifies an array which is passed as an in/out parameter, and returns something to do with how many steps it took to modify the array.

If that modification of the array actually does have side effects, it would probably do better to consider whether it is correct that those side effects exist in the first place, rather than splitting the method into two stages.

Note I say "does have side effects", not merely "could", because ref parameters and in-place mutations are theoretically capable of having side effects if you're only analysing a narrow area of the program, but it doesn't necessarily mean they do have side effects and that the programmer is not analysing the situation more comprehensively.

Steve
  • 8,715
  • "All we can see is that it modifies an array" - modifying the array is a side effect, and it is clearly the side effect the OP is talking of. – Doc Brown Jul 27 '23 at 11:24
  • @DocBrown, but if the array is itself local to the calling scope, then it's a "side effect" only in the most fundamentalist sense that in/out parameters are treated as side effects because they deviate in form or syntax from a "pure" function (even though the actual data transmission is purely between caller and callee, and there is neither input to the callee from outside the scope of the caller, nor output from the callee to anywhere outside the caller). – Steve Jul 27 '23 at 11:53
  • I am pretty sure the the quote "a method should return a value or have side-effects, but not both" refers to this point of view - it is about how to design a function, regardless from how the caller is going to use it, or from which scope the passed array comes from. If one wants such a method to be side-effect free, one has to convert it into an out-of-place sort, like in John Go-Soco's answer. – Doc Brown Jul 27 '23 at 12:34
  • @DocBrown, my point is that a method can't be said to have "side effects" on a program in any real way, if it only exchanges data directly and explicitly with its own caller. I can't accept that an in/out parameter is a side channel of data flow. That it does not conform syntactically to arguments going in and results coming out, is just how some languages are designed with different syntax - it doesn't mean the data flows between methods any differently. – Steve Jul 27 '23 at 16:26
  • Look at the top answer for the OPs link. It contains a good metapher from Bertrand Meyer: "Asking a question should not change the answer.". But a customSort which returns it's step count directly does exactly this - if one wants to ask for the step count again, and tries to call customSort twice, they get zero on the second call. This exactly what the principle tries to avoid, and it will happen even when the array just exists in the local scope of the caller. – Doc Brown Jul 27 '23 at 18:11
  • @DocBrown, this isn't an instance of "asking a question", it's an instruction to perform a mutating action, and report some metadata generated in the process. The function is fully-determinstic - the same inputs produce the same outputs. The fact that a variable used first as input, is implicitly reassigned and overwritten with the output at the conclusion of the call, is simply a feature of the syntax. Anyway, whatever the other objections, this is not a side effect in my view. A side effect must mean data is transmitted other than directly between the caller and callee. – Steve Jul 27 '23 at 18:57
0

For your specific example, you can stick to one paradigm by returning multiple values.

Here, we get a return object where we can get both the modified array and the step count, and we don't have to modify the original myArray in the function (which is the side-effect you wanted to avoid).

function customSort(myArray){
    let result = []
    let stepCount = 0;
    // some steps where result is cloned from myArray, then modified.
    return {
        newArray: result,
        steps: stepCount,
    }
}

Personally, I wouldn't have a problem with the original function if it has clear docs which state the return value and the fact that myArray is mutated.

John Go-Soco
  • 236
  • 1
  • 6
0

Those rules for clean code should be seen as recommendations, not hard laws. If there is a good reason to break a rule, you can do it.

That said, one solution to make the code cleaner is to pass a function that is run on each step; in your case it should count the number of steps. It could also be used to show progress.

function customSort(myArray, stepHandler) {
    // run some steps to sort the array
    // for each step, call stepHandler
}

let stepCount = 0;
customSort(this.myArray, () => { stepcount++; });
this.showAnimation(stepCount);
md2perpe
  • 153