Is altering a function argument inside the function bad style?-CodePudding

Today the following code of mine was critcized as being bad style:

export const myFunction = (myArgument: TypeOfObject[]) => {
  if (!myArgument) {
    myArgument = [];
  }
  // do something with myArgument
}

This is TypeScript, so to the best of my knowledge the following is happening: myFunction is getting passed by value a pointer called myArgument. If that pointer is pointing to undefined, I re-point it to [], otherwise I leave it as it is. So neither am I changing the object that it is pointing to nor am I changing the pointer itself for the context of the caller.

This has been criticized as bad style with the explanation that the function argument should not be altered. The following alternative has been suggested:

export const myFunction = (myArgument: TypeOfObject[]) => {
  let myArgumentSecond: TypeOfObject[];
  if (!myArgument) {
    myArgumentSecond = [];
  } else {
    myArgumentSecond = [...myArgument];
  }

  // do something with myArgumentSecond (instead of with myArgument)
}

What is your opinion on this? Is the second version really better?

Note that while this is TypeScript here, I think this is really a general programming question and I fail to understand what is supposed to be wrong with the first version.

CodePudding user response：

// If you are using this in a purely TypeScript project, that should be
// default argument value

export const myFunction1 = (myArgument: TypeOfObject[] = []) => {
  // do something with myArgument
  myArgument
  // ^? (parameter) myArgument: TypeOfObject[]
}
// because function doesn't say it can be called with non-array.

// If it can be called with non-array, it should say so
export const myFunction2 = (myArgument?: TypeOfObject[] | null | undefined) => {
  myArgument ??= [];
  myArgument
  // ^? (parameter) myArgument: TypeOfObject[]
}

// if you use type-casting you SHOULD use another variable as TypeScript doesn't handle them
export const myFunction3 = (a: number | string) => {
  a = Number(a) % 3 as 0 | 1 | 2
  ;; a
  // ^? (parameter) a: number
  let b = Number(a) % 3 as 0 | 1 | 2
  //  ^? let b: 0 | 1 | 2
}

interface TypeOfObject {};

CodePudding user response：

I believe the key difference between your code and the suggestion is that you are guarenteed to never mutate the argument. That is, whatever argument you send to the function is guaranteed to never be changed (in a shallow sense, as you can still change the items inside the array). Whether or not one or the other is better is a matter of discussion and intent. For example:

function sortInplace(x) { // Displays intent that you want to mutate 
  x.sort()
}

function sortNotInplace(x) {
  return [...x].sort()
}

a = [4,3,2,1]

sortInplace(a) // mutates 'a', this function is impure

console.log(a) // [1,2,3,4]

b = [4,3,2,1]
c = sortNotInplace(b) // copies 'b' then sorts the copy, and returns the sorted copy, function is pure

console.log(b) // [4,3,2,1]
console.log(c) // [1,2,3,4]

It is often nice to have functions that are "pure", meaning they never change anything "on the outside", as the opposed "impure" functions can cause hard to find bugs.

The cost of purity is usually runtime performance as you generally have to do more copying, although you can often get by with very little performance penalty in many cases. The suggested code will create an entirely new array. If your input array is large, then the computer has to, well, create an entirely new large array, which can be inefficient.

This is only my opinion, which may or may not fit for whomever reads this:

I believe your first snippet is fine (it is what I would prefer), the most important thing is that you are always aware whether you are mutating the input or not, then properly signalling that it will mutate the input if that is the case (or change the function is mutation is undesired). It is also worthwhile to consider whether your peers have agreed on a coding style. If the suggested code is what your peers have agreed to then I would say you should follow that (this is most relevant for a software team). The suggested code makes it extremely obvious that the input array won't be changed (but then again, if the function is small and easy to read it may be obvious anyways).