Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add arguments for why pipelines are better than mutable temporary variables #211

Merged
merged 5 commits into from
Sep 14, 2021

Conversation

loreanvictor
Copy link
Contributor

As discussed in #200 , the current README only considers added value of pipelines over constant, properly named and single-use temporary variables. There is an alternative (raised in #200, #207) of using mutable variables with short names, which also achieves all of the benefits of pipelines over temporary variables currently mentioned in the README.

Example from the README, with immutable temporary variables:

const envarKeys = Object.keys(envars)
const envarPairs = envarKeys.map(envar =>
  `${envar}=${envars[envar]}`);
const envarString = envarPairs.join(' ');
const consoleText = `$ ${envarString}`;
const coloredConsoleText = chalk.dim(consoleText, 'node', args.join(' '));
console.log(coloredConsoleText);

... which is wordy and tedious, but can be written with the pipe operator like this:

envars
|> Object.keys(^)
|> ^.map(envar =>
  `${envar}=${envars[envar]}`)
|> ^.join(' ')
|> `$ ${^}`
|> chalk.dim(^, 'node', args.join(' '))
|> console.log(^);

... or, can be re-written using a mutable temporary variable like this:

let _= envvars
_= Object.keys(_)
_= _.map(envar =>
  `${envar}=${envars[envar]}`)
_= _.join(' ')
_= `$ ${_}`
_= chalk.dim(_, 'node', args.join(' '))
_= console.log(_)

As discussed in #200 (see this, as a summary of the discussion), benefits of pipelines over mutable temporary variables are safety and code readability. Subsequently, it would be helpful to add these additional arguments to the README itself, as the current argument against temporary variables (mentioned in the README) is incomplete (it only considers one alternative).

@mAAdhaTTah
Copy link
Collaborator

I got an email about a comment here about being able to use it in expression position; did you delete that? I actually think that's a good point!

@loreanvictor
Copy link
Contributor Author

loreanvictor commented Sep 14, 2021

I got an email about a comment here about being able to use it in expression position; did you delete that? I actually think that's a good point!

Can I delete comments? Either way I got the email as well but can't find it anywhere.

Yes, it is a good point, I even included a segment in the OP of #200 about it, but since the discussion did not focus on that point (and whether the assumptions I made there were correct or not), I simply forgot to include it in the additions.

I think I can add a small bit on that front, however I am unsure whether referencing JSX would be a sound argument for a proposal to JS itself, cause if not, the added value would be pretty marginal between the two styles and perhaps not worthy of inclusion.

@mAAdhaTTah
Copy link
Collaborator

Can I delete comments? Either way I got the email as well but can't find it anywhere.

Yeah, looks like that wasn't you, my b.

I think I can add a small bit on that front, however I am unsure whether referencing JSX would be a sound argument for a proposal to JS itself, cause if not, the added value would be pretty marginal between the two styles and perhaps not worthy of inclusion.

Pattern matching references both Redux & JSX so I think it's reasonable to include here as well. Putting something in an expression position is broadly useful, which is basically why do expressions are popular.

@js-choi
Copy link
Collaborator

js-choi commented Sep 14, 2021

I really appreciate this pull request. I have a lot on my plate right now, and I plan to review and merge it later, if that’s okay.

@kaizhu256
Copy link

having used this design-pattern alot, readability is improved by giving temp-variable a topical/semantic name (and helps avoid unwanted accidental-closure-by-name-clash issue raised earlier).

the name-picking-convention w/ least cognitive-load for me is whatever that best describes the final return/output/serialized value (in this case coloredConsoleText).

-let _= envvars
-_= Object.keys(_)
-_= _.map(envar =>
-  `${envar}=${envars[envar]}`)
-_= _.join(' ')
-_= `$ ${_}`
-_= chalk.dim(_, 'node', args.join(' '))
-_= console.log(_)

+// improve readability by using topical-name to describe final return value
+let coloredConsoleText = envvars
+coloredConsoleText = Object.keys(coloredConsoleText)
+coloredConsoleText = coloredConsoleText.map(envar =>
+  `${envar}=${envars[envar]}`)
+coloredConsoleText = coloredConsoleText.join(' ')
+coloredConsoleText = `$ ${coloredConsoleText}`
+coloredConsoleText = chalk.dim(coloredConsoleText, 'node', args.join(' '))
+coloredConsoleText = console.log(coloredConsoleText)

@fuunnx
Copy link

fuunnx commented Sep 14, 2021

I got an email about a comment here about being able to use it in expression position; did you delete that? I actually think that's a good point!

That was me 😅

Here is the comment I made and retracted (because I remembered it was discussed a bit in #200) :


Also a benefit not yet mentionned in this discussion is the ability to use the pipeline in expression position, e.g:

const doStuff = (envvars) => envvars
    |> Object.keys(^)
    |> ^.map(envar =>`${envar}=${envars[envar]}`)
    |> ^.join(' ')
    |> `$ ${^}`
    |> chalk.dim(^, 'node', args.join(' '))
    |> console.log(^);

return (
  <ul>
    {
      values
       |> Object.keys(^)
       |> [...Array.from(new Set(^))]
       |> ^.map(envar => (
        <li onClick={() => doStuff(values)}>{envar}</li>
       ))
    }
  </ul>
)

vs

const doStuff = (envvars) => {
  let _= envvars
  _= Object.keys(_)
  _= _.map(envar => `${envar}=${envars[envar]}`)
  _= _.join(' ')
  _= `$ ${_}`
  _= chalk.dim(_, 'node', args.join(' '))
  _= console.log(_)
 return _
}

let _ = values
_= Object.keys(_)
_= [...Array.from(new Set(_))]
_= _.map(envar => (
  <li onClick={() => doStuff(values)}>{envar}</li>
))
const uniqKeys = _

return (
  <ul>
    {uniqKeys}
  </ul>
)

README.md Outdated Show resolved Hide resolved
Co-authored-by: Vitor Luiz Cavalcanti <vitorluizc@outlook.com>
Copy link
Collaborator

@js-choi js-choi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ll be happy to merge this in after the Promise.resolve().then(() => console.log(^)) line is addressed. I’m not sure what’s supposed to be happening in it, because the next line three(^) is going to receive a promise that resolves to undefined. Oh, wait, I get it now; three is supposed to return a constant.

README.md Outdated Show resolved Hide resolved
@js-choi
Copy link
Collaborator

js-choi commented Sep 14, 2021

I’m going to make some small formatting changes to this branch before I squash and merge.

@js-choi
Copy link
Collaborator

js-choi commented Sep 14, 2021

With regards to @fuunnx’s comment, do expressions would obviate a lot of that problem, but it still may be worth mentioning.

@js-choi js-choi merged commit 21167e4 into tc39:master Sep 14, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants