13
gitpush
5y

I just wrote this code, let's see if the dude who is going to work with it after me will rant about checking state right after setting it as null.

I wonder if he will know why I did this ...

Comments
  • 8
    I can see what you did but dont know why. For that purpose you should write a short comment
  • 2
    @zemaitis good point, I'll add that, thanks man :D
  • 4
    I don't get it...
  • 1
    @irene because if the props i received for this component are not accepted by my conditions I don't want my component to update therefore I return null, but if it matches a condition then I return an object -> causing render() to be called again.

    That's how react native work, if I return an empty object it will render
  • 4
    Arcanic stuff with no comments, side-effects, what-ifs.

    I sure hope the next maintainer will scratch its head.
  • 2
    @netikras I define an object = null, then right after that I check if it has a value. This is dump but just to make that if someone adds another if condition before this one, the object is always checked if null instead of having the "if condition" in that picture hidden down the code
  • 2
    @karasube added comment don't worry lool
  • 1
    @gitpush

    if (nextProps.appState.state != prevState.state) {

    ^^-- before this condition?
  • 1
    @netikras yup before that condition
  • 1
    @irene ya that function is not yet complete I'm still working on it
  • 2
    @gitpush So if your conditional logic is that sensitive to newState, why use it in a scope where you can't control it's value?

    Why can't you isolate its scope?

    Also why do you init it as am empty object and reinit it afterwards with another object? What diff does it make if it's null before init'ing it as a full object with properties? Why can't you transition null->new {aa:bb, etc} and do null -> new {} -> new {aa:bb, etc} instead? [EDIT scratch that. Missed the fact that you're nesting. New screenshot below]

    bear in mind I'm not a js dev so I might not now smth. But smth tells me this is an overcomplicated spaghetti
  • 1
  • 1
    @gitpush Sorry, missed the fact that you're nesting
  • 1
    @irene @netikras
    This function in react native is the first step to determine if the component has updated values to read. by design it must return null to indicate nothing was updated or else even an empty object will cause the component to re-render

    To prevent a million "return" in my code, I keep just one and handle the object I need to return by assuming that: Before I know what I received I'm going to assume no changes happened, therefore newState = null. Then I read and check, if I find something I want I'll update state.
    No matter what happens "newState" must not be be set to null inside any of the conditions, if it is meant to be null, then my conditions must not assign a value, and not be like: So I assign a value but then it must be null.

    Hope that cleared it
  • 1
    @netikras as for the function you wrote in your screenshot, "nextProps" hold many objects that I mighit need and it is not limited to appState.state, it might be appState.user, or appState.settings

    So having a separate function to extract them makes no sense to me unless you have a different point of view, please let me know
  • 1
    @irene dude that photo has an incomplete function
    -_-
  • 2
    @netikras this doesn’t feel right.
    This component gets appState from props, which is a shallow copy of global state, I guess.
    And then it has an internal state which duplicates the same value(but different object)
    The appState.state shouldn’t be the same object as the internal state in the 1st place...
    No offence, but I feel this is a naive implementation on MobX.
    If there’re lots of global states involved, I would use MobX to set appState as observable
  • 1
    @gitpush Well you do have the newState in that function:
    `getStateForProps(nextProps, *newState*)`
    so pull wtv values you like

    I don't think having separate functions is a bad idea. A single function defines and performs an action. When you read a code you know what parts of your function are doing what. So if you can separate different parts of the function -- why not separate them into different functions?
    #1 easier to read
    #2 safer, as each part has its own scope
    #3 can be updated separately
  • 1
    @irene since when you started working on React 🤔
  • 1
    @sunfishcc I didn't know about mobx to be honest :\ It looks a lot like using Vue + Typescript decorators. Thanks for suggesting it :D

    @netikras Good point, I didn't think of it that way
    @irene I didn't, I'm using a template we wrote at work, and to indicate that function needs a return value, we put return null, and I didn't reach the point to change that.
  • 1
    @irene this logic itself is an anti-pattern of React.
    It’s like, you have an apple, now you need a pen, but they give you another apple, and it’s the same apple you already got. Why...🤦‍♂️
  • 1
    @gitpush and #4 - you do not have to riddle your code with comments as you now have function names to explain what (and sometimes - why) you do.

    Which one is easier to read?

    // does shit. when a is not MY_CONSTANT it has to be YOUR_CONSTANT to mimic APIs parameter and ...................
    if (a != 5) {
    a=6;
    ...
    ...
    ......
    a=14
    }

    vs.

    a= doShit(a);

    doShit(a){
    if (canDoShit(a)){
    a = 6
    ...
    .....
    return getDefaultShit();
    }
    }
  • 1
    @irene if only I can share with you the response of your question which I sent to the one who wrote it. It is as stupid as: I like it that way :\
  • 1
    @irene https://youtu.be/Ct6BUPvE2sM
    Please refer to pineapple pen video.
  • 1
    @gitpush it’s easier setup than redux. You can use it without decorators, if your don’t want to change webpack config file.

    @irene
    Having duplicated states is a bad idea.
    If you want to pass all states into child component, just use spread operator
    <Child {...this.state} />
    You child component will automatically update when any prop changes.
    The rerender is triggered by shallow comparison for state and props.

    These static functions are relatively new, there’s an article called
    “You probably don’t need derives state” for a good reason
  • 0
    @irene I think the last sentence from React team is pretty self explanatory 🤔
  • 2
    @gitpush you still breathing with all those questions 😂 🤣
  • 0
    @lazyDev I'm trying my best to do that XD
  • 0
    "Yes, but no".
  • 0
    so that discussion was long, and I skimmed. but the gist I got was "why initialize to null, and then immediately check it?"

    I'd like to ask a different question. and maybe I'm wrong, please correct me if I am.

    you initialize to null and then immediately check it. but you don't check that it's null, you check that it's falsy, even though null is not the only falsy value. this seems like a bug. why not "if ( newState === NULL)" ?
Add Comment