Like I said, for me it runs twice. your code does the same thing that was happening for me with my original code.
I was able to reproduce in a CRA app. Looking into it more it looks like that is a side effect of StrictMode since version 18.
I didnt like that it was running twice is why I used the ref. I would have removed that if I was going to run in production.
Is this creating a problem? If not, then ignore it. If it is causing problems with your app, I’d rather remove the StrictMode container (probably in the root index.js) than start monkeying around with the code. The “I’ll just use this code for dev but get rid of it for prod” is a classic setup line for a bug, or several. On the rare chance I do do that, I put it behind an environment flag and then run a unit test that tests both environments.
And please don’t put pictures of code. It makes it much more difficult to help.
the above is what I was doing in my code. Im not referencing obj in the outer scope or mutating it…
Yes, you are. Even if you weren’t, you are committing the sin of variable name shadowing - it is a code smell - given enough time, it will lead to a bug. I know what you are saying, “It’s just my project, I know what I mean.” First of all, as we say in music, “The way you practice is the way you perform.” Secondly, if I were doing a code review for a hiring candidate, and I saw that, it would definitely be a checkmark against them. The point isn’t to write code that is clear and makes sense to you, it’s the rest of your team that matters.
OK, let’s ignore the shadowing. You are mutating an input parameter, another major code smell, that will lead to buggy, difficult to maintain bug (I speak as someone who has had to spend hours tracking down a few bugs because someone did exactly this - once it was even me that did it).
Ive seen others use it as well to declare empty objects
The point isn’t that you are declaring an empty object. The point is that you are defaulting. OK, there are definitely cases where you want to default to an empty object - I literally did it twice yesterday at work. That is not the issue.
The issue is that you are not testing this properly:
somefunction()
You didn’t pass in the reference. So, your function sees and undefined first parameter and uses the default. It creates an empty, anonymous object, mutates it, logs it, then throws it away. In that case, what is the purpose of that function? The more meaningful way to test this code is:
somefunction(obj)
What is the purpose of that function? To mutate the object? In that case, the default parameter makes no sense. Even if you wanted to write a mutating function, this wouldn’t work.
One of the principles of functional programming is that functions should not have side effects. Mutating parameters is a side effect. This will cause bugs. It will also cause things like React and Redux (and possibly other things) to not work properly.
Last month, I had to write some code in the service layer, the part of the app that does the fetching from the backend. I found that the backend was sending all the properties in snake_case. I hate snake_case for variable names in JS so I wrote a function that could transform the object to change have camelCase property names:
const originalObj = {
some_prop: 0,
some_other_prop: 1,
some_other_prop2: 2,
last_prop_i_promise: 3,
}
const convertObjPropsFromSnakeToCamel = oldObj => {
const newObj = {}
for (oldProp in oldObj) {
const words = oldProp.split('_')
const newProp = words[0].toLowerCase() + words.slice(1).map(w => w.charAt(0).toUpperCase() + w.slice(1).toLowerCase()).join('')
newObj[newProp] = oldObj[oldProp]
}
return newObj
}
const convertedObj = convertObjPropsFromSnakeToCamel(originalObj)
console.log('originalObj...')
console.log(JSON.stringify(originalObj, null, 2))
// {
// "some_prop": 0,
// "some_other_prop": 1,
// "some_other_prop2": 2,
// "last_prop_i_promise": 3
// }
console.log('convertedObj...')
console.log(JSON.stringify(convertedObj, null, 2))
// {
// "someProp": 0,
// "someOtherProp": 1,
// "someOtherProp2": 2,
// "lastPropIPromise": 3
// }
Note, this isn’t the actual function - that was written in TypeScript and used a bunch of lodash, but this is the basic idea.
The important point here is that my function does not mutate the input parameter. i create a new one and manipulate that, and then return the new object. The original object is completely unaffected.
Why did I do this? Did it matter? I was just using this in the service layer and the original object wouldn’t be used anywhere else. What was the harm of mutating it?
None, really. Except that it is a really bad habit to get into, a violation of some basic principles. (Plus, I think our linter or sonar would start setting off alarms.) But what if … what if someone, somewhere else in the code, saw that function in the utils folder and decided to use it somewhere else. What if they assumed (as the should be able to) that it would not mutate the input parameters. Or, what if they saw that it did and didn’t care but then 6 months later someone else came along …
Just write code following basic principles. Don’t argue about them, just accept them. Read Clean Code. Read blogs. Unless you want to work alone forever, learn to write clean code. That is one of the most important things I judge code by. A lot of people can write code that works. I like working with people that write code that is easy to understand and maintain.
One of the nicest compliments I received recently was from a junior developer that did a code review on one of my PRs. He hung back after our standup and said, “I have to say, that PR was beautiful. It was a confusing task and I was worried but your code was so easy to understand.” I had to hide a blushing smile that was trying to burst out of my face. That is what I always try to do. I did it without a single code comment. It was all just logically organized code and good variable names. Those are two of the hardest things to teach but are two of the most important things.
Im changing the object that I declared as a default parameter and then using that to set the state…
OK, I’m confused.
So, you’re saying that for this:
Promise.all([...data]).then((res, data = {}) => {
You’re saying that the callback for Promise.all().then will never have a second parameter so it will always create a new object?
Maybe that is true, maybe then will never pass a second parameter - I can’t remember. But even if that is true, this is not how you declare variables. That is even a bigger sin than what I thought you were doing. That is so much worse than what I thought you were doing. That is just a mess. The purpose of a parameter list is to pass parameters. The purpose of a default parameter is a fallback in case the parameter is undefined. The purpose of these is not to declare new variables (not in that sense).
FFS, this is exactly what I am talking about. Now I think I understand what you were trying to do. The way you wrote your code was deceptive. If you’d written const data = {};
as the first line of that callback, instead of that rube goldberg in the parameters list, it would have made perfect sense what you were doing. Instead, you created a riddle. You combined two tools to do something neither was intended to do instead of using the one tool that was specifically designed to do exactly this. Good code tells a story, it tells you what it is doing. That code lied to me. It lies to everyone that reads it.
If you saw other people doing this, you either misunderstood what they were doing, or they were very bad coders.