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

Should single-line if expressions be allowed? #209

Closed
gyzerok opened this issue Jun 22, 2016 · 25 comments · May be fixed by #648
Closed

Should single-line if expressions be allowed? #209

gyzerok opened this issue Jun 22, 2016 · 25 comments · May be fixed by #648
Labels
0.9 (temporary label for search filtering) discussion

Comments

@gyzerok
Copy link
Contributor

gyzerok commented Jun 22, 2016

Following code

isPalindrome : List comparable -> Bool
isPalindrome list =
    List.map2 (,) list (List.reverse list)
        |> List.foldl
            (\( x, y ) acc -> if x == y then acc else False)
            True

turns to this

isPalindrome : List comparable -> Bool
isPalindrome list =
    List.map2 (,) list (List.reverse list)
        |> List.foldl
            (\( x, y ) acc ->
                if x == y then
                    acc
                else
                    False
            )
            True

What do you think about it guys?

@avh4 avh4 added this to the 1.0.0 public release milestone Jun 28, 2016
@rtfeldman
Copy link

I guess this reduces to "should single-line if-expressions be allowed?"

I'd be fine with that. I've used them before, and they didn't seem particularly more prone to getting out of hand than any other kind of expression.

@avh4
Copy link
Owner

avh4 commented Jul 5, 2016

I think this is worth considering. Though for the example given, I would tend to use let:

isPalindrome list =
    let
        step ( x, y ) acc =
            if x == y then
                acc
            else
                False
    in
        List.map2 (,) list (List.reverse list)
            |> List.foldl step True

I'd love to see a few more examples from real code. Are inline lambdas the main place where this would be useful?

@gyzerok
Copy link
Contributor Author

gyzerok commented Jul 7, 2016

I'm right now on a road of implementing 99 Haskell problems in Elm. And this is just a code for one of those problems. I would try to share more examples while moving through this problems.

However I really like idea with let :)

@simonewebdesign
Copy link

Here's an example from real code:

getAtPath configuration.path model.data
    |> valueToString
    |> (\str ->
            if str == "Yes" ||
               str == "No" then model.translate str else str
        )

turns to this:

getAtPath configuration.path model.data
    |> valueToString
    |> (\str ->
            if
                str
                    == "Yes"
                    || str
                    == "No"
            then
                model.translate str
            else
                str
       )

Definitely seems a bit too aggressive :)

@gyzerok
Copy link
Contributor Author

gyzerok commented Aug 3, 2016

It looks like here is no problem with single/multiline if, but rather with boolean expression formatting.

str
    == "Yes"
    || str
    == "No"

This looks weird to me.

@seanhess
Copy link

I would definitely prefer if/then to be more flexible.

@Zimmi48
Copy link

Zimmi48 commented Dec 26, 2016

I vote for allowing single-line if then else expressions. I'm quite annoyed to get

toString points ++ " point" ++ (if points >= 2 then "s" else "")

turned into

toString points ++ " point"
    ++ (if points >= 2 then
            "s"
        else
            ""
       )

There is no specific syntax for ternary conditions in Elm (as in C) so let short if then else expressions be.

@crazymykl
Copy link

Another vote for single-line if. It's much nicer in let-bindings, especially since those already make functions very tall.


yesno b =
    if b then "yes" else "no"

seems nicer to read than

yesno b =
    if b then
        "yes"
    else
        "no"

@avh4
Copy link
Owner

avh4 commented Aug 10, 2017

Evan has requested that if expressions have blank lines between the clauses #393 which seems to make a single-line version of if more desirable.

@avh4 avh4 changed the title Are if expressions formatted too aggressively? Should single-line if expressions be allowed? Aug 10, 2017
@klazuka
Copy link
Contributor

klazuka commented May 22, 2018

@avh4 any update on this? The first thing I thought after I saw the blank-line-between-if-clauses in the 0.8.0 changelog was "I hope single-line ifs" are supported.

@avh4
Copy link
Owner

avh4 commented May 23, 2018

I haven't had time to revisit this yet.

If people have snippets of real-world code where they would use single-line if statements if available, please post them here! There have been a couple posted already, but I think it would be useful to see more examples of what this would look like in the wild.

@klazuka
Copy link
Contributor

klazuka commented May 23, 2018

I just went through my codebase and pulled out some examples:
https://gist.github.com/klazuka/fdd18790ce50ee291a1a210c63ef48f7

After looking at a lot of ifs in my codebase, it seems to me that the strongest case for single-line if is in places like an Html view or an inner declaration in a let/in expression. In such cases, multi-line if with a blank line before the else makes the code too ragged/disconnected/floaty, IMHO.

I was surprised that single-line looked wrong much more often than I expected.

That being said, I think there's still a case for leaving the choice up to the developer. If they leave it all on a single line, then let it be, similar to how function arguments are only pushed out to separate lines if the developer breaks it up into separate lines.

@klazuka
Copy link
Contributor

klazuka commented May 25, 2018

Here's another real-world example I just found in my codebase...

status quo:

p
    [ style
        [ ( "width", "70%" )
        , ( "padding", "6px" )
        , ( "margin-left", "-6px" )
        , ( "background-color"
          , if isEndUser then
                "#eef3ff"

            else
                "white"
          )
        ]
    ]
    [ text "..." ]

single-line variant:

p
    [ style
        [ ( "width", "70%" )
        , ( "padding", "6px" )
        , ( "margin-left", "-6px" )
        , ( "background-color", if isEndUser then "#eef3ff" else "white" )
        ]
    ]
    [ text "..." ]

Single line seems like a much better choice here.

@klazuka
Copy link
Contributor

klazuka commented May 25, 2018

Sorry to keep piling on, but I'm upgrading elm-json-tree-view for 0.19 and ran into this one while dealing with the toString change with respect to Bool.

multi-line:

viewNodeInternal depth config node state =
    case node.value of
        TString str ->
            viewScalar css.string ("\"" ++ str ++ "\"") node config

        TFloat x ->
            viewScalar css.number (String.fromFloat x) node config

        TBool bool ->
            viewScalar css.bool
                (if bool then
                    "true"

                 else
                    "false"
                )
                node
                config

single-line:

viewNodeInternal depth config node state =
    case node.value of
        TString str ->
            viewScalar css.string ("\"" ++ str ++ "\"") node config

        TFloat x ->
            viewScalar css.number (String.fromFloat x) node config

        TBool bool ->
            viewScalar css.bool (if bool then "true" else "false") node config

@ianp
Copy link

ianp commented Sep 26, 2018

I’d love to see elm-format get to the point where @evancz & co. could start using it on elm/core, and right now the core libs use single line lambdas and single line ifs in a few places, sometimes even both at the same time! E.g. here List.elm#L215

@ymtszw
Copy link

ymtszw commented Nov 10, 2018

I had a possibly interesting experience around this.

I also wanted oneline if-then-else since switching relatively short expression was somewhat common in my codebase, especially in views (e.g. just switching color or border style based on activation state.)

But found elm-format does not allow that.
"Well, okay," I thought, discussion is going in this issue anyway. Time will resolve this.

In the mean time, let's just have this to avoid formatting:

ite : Bool -> a -> a -> a
ite condition then_ else_ =
    if condition then
        then_

    else
        else_

... and I used it casually, everywhere, everywhen I thought I wanted oneline logical switch.

However, recently I realized that this helper function actually have a significant flaw: it eliminates laziness of if-then-else!

It could be OK if both then_ and else_ above are static, immediate values determined at compile-time.
Though if either or both of them are resultants of function calls, unnecessary evaluation(s) will happen at runtime!

(To be clear:

ite condition (funcitonForThen arg1) (functionForElse arg2)

in this case both funcitonForThen AND functionForElse are evaluated.

if condition then
  functionForThen arg1
else
  funcitonForElse arg2

With this, only one of them are evaluated!)

So, allowing oneline if-then-else might have a practical "defensive" value in that it prevents someone like me from introducing such suboptimal escape hatch!

@ianp
Copy link

ianp commented Nov 12, 2018

@ymtszw yup, you probably want your function to have this type signature instead:

its: Bool -> (() -> a) -> (() -> a) -> a
its c t e =
  if c then
    t ()
  else
    e ()

and then invoke it like

ite c (\_ -> t) (\_ -> e)

which is kind of a pain!

@andys8
Copy link
Contributor

andys8 commented Apr 16, 2019

An example with elm-ui and a border on the bottom, if selected:

Current formatting

    el
        [ paddingXY 15 30
        , Border.color styles.colors.blue
        , Border.widthEach
            { top = 0
            , left = 0
            , right = 0
            , bottom =
                if isSelected then
                    4

                else
                    0
            }
        ]
    <|
        text content

Formatting I'd prefer

    el
        [ paddingXY 15 30
        , Border.color styles.colors.blue
        , Border.widthEach
            { top = 0
            , left = 0
            , right = 0
            , bottom = if isSelected then 4 else 0
            }
        ]
    <|
        text content

@holgerl
Copy link

holgerl commented Apr 29, 2019

It has been almost three years now. Can we get a conclusion on this issue?

@andys8
Copy link
Contributor

andys8 commented Apr 29, 2019

My impression is that if somebody submits a pull request for this feature, it will most likely get merged.

  • Richard wrote he is fine with it
  • Evan is using single line if statements in elm/core
  • Aaron said the feature is planned

@andys8
Copy link
Contributor

andys8 commented Jul 9, 2019

Regarding the implementation:

Here is the formatting done.

formatIf (cond, body) =
stack1
[ opening (line $ keyword "if") $ formatCommentedExpression elmVersion importInfo SyntaxSeparated cond
, indent $ formatCommented_ True (formatExpression elmVersion importInfo SyntaxSeparated) body
]
formatElseIf (ifComments, (cond, body)) =
let
key =
case (formatHeadCommented id (ifComments, line $ keyword "if")) of
SingleLine key' ->
line $ row [ keyword "else", space, key' ]
key' ->
stack1
[ line $ keyword "else"
, key'
]
in
stack1
[ blankLine
, opening key $ formatCommentedExpression elmVersion importInfo SyntaxSeparated cond
, indent $ formatCommented_ True (formatExpression elmVersion importInfo SyntaxSeparated) body
]
in
formatIf if'
|> andThen (map formatElseIf elseifs)
|> andThen
[ blankLine
, line $ keyword "else"
, indent $ formatCommented_ True (formatExpression elmVersion importInfo SyntaxSeparated) (AST.Commented elsComments els [])
]
|> expressionParens AmbiguousEnd context

Changing it to print in a single line should be easy. But I guess we want it to be context sensitive: If there are line breaks, they should stay, and if there are none, then it should be printed in single line. I think this information has to be collected when parsing and represented like it's done for other features (e.g. imports)

@andys8
Copy link
Contributor

andys8 commented Jul 17, 2019

Using elm-ui and animations (css transitions):

Current

, htmlAttribute <| style "transition" "all 0.5s ease-out"
, alpha <|
    if isSelected then
        1

    else
        0.6
, moveUp <|
    if isSelected then
        2

    else
        0

Should be

, htmlAttribute <| style "transition" "all 0.5s ease-out"
, alpha <| if isSelected then 1 else 0.6
, moveUp <| if isSelected then 2 else 0

@elkowar
Copy link

elkowar commented Sep 11, 2019

any updates on this? its driving me crazy...

rlefevre added a commit to rlefevre/elm-format that referenced this issue Nov 16, 2019
@rlefevre
Copy link
Contributor

rlefevre commented Nov 16, 2019

It seems there are enough real-world examples to justify testing an implementation, so I made one that only leads to single line expressions if there is no else if nor multi-line expressions in the if condition, then body or else body.

It seems to work quite well. The only drawback I noticed for now is that the following expression:

if if a then True else False then "a" else "b"

which was previously formatted as:

    if                                        
        if a then    
            True    
            
        else    
            False    
    then    
        "a"    
    
    else    
        "b"  

stays formatted as:

if if a then True else False then "a" else "b"

which might be a little confusing.

Given their rarity (the elm-format test that uses one is the only example I ever saw), I'm not sure if nested if conditions should be handled differently or not (maybe there is a way to add parentheses in this example?).

rlefevre added a commit to rlefevre/elm-format that referenced this issue Nov 16, 2019
rlefevre added a commit to rlefevre/elm-format that referenced this issue Nov 16, 2019
rlefevre added a commit to rlefevre/elm-format that referenced this issue Nov 16, 2019
rlefevre added a commit to rlefevre/elm-format that referenced this issue Nov 16, 2019
rlefevre added a commit to rlefevre/elm-format that referenced this issue Nov 17, 2019
rlefevre added a commit to rlefevre/elm-format that referenced this issue Nov 17, 2019
rlefevre added a commit to rlefevre/elm-format that referenced this issue Nov 17, 2019
rlefevre added a commit to rlefevre/elm-format that referenced this issue Nov 17, 2019
rlefevre added a commit to rlefevre/elm-format that referenced this issue Nov 17, 2019
rlefevre added a commit to rlefevre/elm-format that referenced this issue Nov 18, 2019
rlefevre added a commit to rlefevre/elm-format that referenced this issue Nov 18, 2019
rlefevre added a commit to rlefevre/elm-format that referenced this issue Nov 18, 2019
rlefevre added a commit to rlefevre/elm-format that referenced this issue Nov 18, 2019
rlefevre added a commit to rlefevre/elm-format that referenced this issue Nov 22, 2019
rlefevre added a commit to rlefevre/elm-format that referenced this issue Nov 22, 2019
rlefevre added a commit to rlefevre/elm-format that referenced this issue Nov 25, 2019
rlefevre added a commit to rlefevre/elm-format that referenced this issue Nov 25, 2019
rlefevre added a commit to rlefevre/elm-format that referenced this issue Nov 25, 2019
@ymtszw
Copy link

ymtszw commented Jan 10, 2020

if if a then True else False then "a" else "b"

which might be a little confusing.

Inserting parens might indeed looks better.

if (if a then True else False) then "a" else "b"

However, as you probably noticed this should usually be reduced to:

if a then "a" else "b"

I wonder if there are practical cases where "nested ifs" are justified in Elm.
For example, negating a Bool value should better be written with Basics.not, instead of nesting ifs.

-- BAD
if (if a then False else True) then "a" else "b"

-- GOOD
if not a then "a" else "b"

-- GOOD; with a function call
if not (resolvesToBool a) then "a" else "b"

IMHO "nested ifs" cause less concerns in reality since they are usually weeded out in code reviews (or, possibly by linters such as elm-analyse.)

Also as I wrote this, I started to wonder should longer conditional expressions be parenthesized?

-- This:
if conditionalFunction (isActually quite complicated) andOrLong then "what should we do?" else "b"

-- Or this:
if (conditionalFunction (isActually quite complicated) andOrLong) then "what should we do?" else "b"
-- However I would prefer extracting such long conditional values to `let` bindings.

My overall impression is: just leave them without parens.

  • If the conditional part is just a single token or a short enough expression, it is OK
  • Even if it is longer, the syntax highlighting helps us good
  • Nested ifs are assumingly rare enough
  • From implementation POV, drawing a line between "long" and "short" is somewhat subjective and cumbersome, if we are to parenthesize "long" expressions.

@gyzerok gyzerok closed this as completed Jun 1, 2020
@avh4 avh4 added the 0.9 (temporary label for search filtering) label Sep 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.9 (temporary label for search filtering) discussion
Projects
None yet
Development

Successfully merging a pull request may close this issue.