These are not exactly the same. #25

Closed
bengitscode opened this Issue Jun 19, 2017 · 4 comments

Comments

Projects
None yet
4 participants
@bengitscode

bengitscode commented Jun 19, 2017

The example snippets should be edited to perform very same actions, so that the "that's too verbose. This is better" snippet is just a refactoring of the same code.

https://cl.ly/2W3b2U0Q2k3H

@jordanallain

This comment has been minimized.

Contributor

jordanallain commented Jul 28, 2017

maybe i'm missing something about this but they appear the same to me?

@ArjunRayGA

This comment has been minimized.

ArjunRayGA commented Sep 7, 2017

@bengitscode you're right. these two are not equivalent.

as long as lib.modifierFunc is

const modifierFunc = function () {

}

the verbose example will resolve the value pojo without modifying it, ie the lib.modifierFunc(pojo) invocation does nothing

readJSON('./example.json')
  .then((pojo) => {
    lib.modifierFunc(pojo)  // modify object
    return pojo // explicitly returns pojo
  })
  .catch((err) => { // handle error conditions
    console.error(err)
  })

whereas in the shortened example, undefined is returned because lib.modifierFunc doesn't take any arguments and doesn't return anything explicitly.

readJSON('./example.json')
  .then(lib.modifierFunc) // modify object --> returns what callback(prev) returns
  .then(x => console.log(x))
  .catch(console.error)  // handle error conditions

In order for this example to make any kind of sense, we'd need the following:

lib.modifierFunc

const modifierFunc = function (obj) {
    // modify object
    return obj
}

verbose example

readJSON('./example.json')
  .then((pojo) => {
    pojo = lib.modifierFunc(pojo)  // modify object
    return pojo // explicitly returns pojo
  })
  .catch((err) => { // handle error conditions
    console.error(err)
  })
@jordanallain

This comment has been minimized.

Contributor

jordanallain commented Oct 1, 2017

i was going to make this change but i have a couple of questions about that last example, added line numbers to make it easier for me to point them out.

1 readJSON('./example.json')
2 .then((pojo) => {
3    pojo = lib.modifierFunc(pojo)  // modify object
4    return pojo // explicitly returns pojo
5  })
6  .catch((err) => { // handle error conditions
7    console.error(err)
8  })

on line 3 should we consider using a variable name that doesn't match the parameter to cut back on potential confusion? maybe: modifiedPojo = lib.modifierFunc(pojo) then return modifiedPojo

Also, on lines 6 and 7 was there a reason you changed it from .catch(console.error) // handle error conditions? your version is a bit more explicit which is maybe what your impetus was, just wondering.

jordanallain pushed a commit that referenced this issue Dec 4, 2017

jordanallain
Alter modifierFunc to return argument passed to it
- Alter the modifierFunc that is used in an example snippet.
- References #25
@caleb-pearce

This comment has been minimized.

Contributor

caleb-pearce commented Dec 7, 2017

Ya I think changing the variable name to show that it's been modified is a nice touch. Another option is

1 readJSON('./example.json')
2 .then((pojo) => {
3   return lib.modifierFunc(pojo) // return result of calling modifier function on pojo
5  })
6  .catch((err) => { // handle error conditions
7    console.error(err)
8  })

but that might actually be less clear.

@danman01 danman01 closed this in 73fabf1 Feb 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment