Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > comp.lang.python > #12252 > unrolled thread

how to format long if conditions

Back to article view | Back to comp.lang.python

Contents

  how to format long if conditions Arnaud Delobelle <arnodel@gmail.com> - 2011-08-27 08:08 +0100
    Re: how to format long if conditions Steven D'Aprano <steve+comp.lang.python@pearwood.info> - 2011-08-27 17:24 +1000
      Re: how to format long if conditions Arnaud Delobelle <arnodel@gmail.com> - 2011-08-27 11:24 +0100
      Re: how to format long if conditions Ben Finney <ben+python@benfinney.id.au> - 2011-08-27 22:04 +1000
    Re: how to format long if conditions Hans Mulder <hansmu@xs4all.nl> - 2011-08-27 09:50 +0200
      Re: how to format long if conditions Steven D'Aprano <steve+comp.lang.python@pearwood.info> - 2011-08-27 19:05 +1000
        Re: how to format long if conditions Hans Mulder <hansmu@xs4all.nl> - 2011-08-27 12:51 +0200
      Re: how to format long if conditions "Colin J. Williams" <cjw@ncf.ca> - 2011-08-27 11:16 -0400
        Re: how to format long if conditions Hans Mulder <hansmu@xs4all.nl> - 2011-08-27 17:53 +0200
          Re: how to format long if conditions "Colin J. Williams" <cjw@ncf.ca> - 2011-08-27 17:25 -0400
    Re: how to format long if conditions Roy Smith <roy@panix.com> - 2011-08-27 12:39 -0400

#12252 — how to format long if conditions

Hi all,

I'm wondering what advice you have about formatting if statements with
long conditions (I always format my code to <80 colums)

Here's an example taken from something I'm writing at the moment and
how I've formatted it:


        if (isinstance(left, PyCompare) and isinstance(right, PyCompare)
                and left.complist[-1] is right.complist[0]):
            py_and = PyCompare(left.complist + right.complist[1:])
        else:
            py_and = PyBooleanAnd(left, right)

What would you do?

-- 
Arnaud

[toc] | [next] | [standalone]

#12253

Arnaud Delobelle wrote:

> Hi all,
> 
> I'm wondering what advice you have about formatting if statements with
> long conditions (I always format my code to <80 colums)
> 
> Here's an example taken from something I'm writing at the moment and
> how I've formatted it:
> 
> 
>         if (isinstance(left, PyCompare) and isinstance(right, PyCompare)
>                 and left.complist[-1] is right.complist[0]):
>             py_and = PyCompare(left.complist + right.complist[1:])
>         else:
>             py_and = PyBooleanAnd(left, right)
> 
> What would you do?

I believe that PEP 8 now suggests something like this:

        if (
                isinstance(left, PyCompare) and isinstance(right, PyCompare)
                and left.complist[-1] is right.complist[0]):
            )
            py_and = PyCompare(left.complist + right.complist[1:]
        else:
            py_and = PyBooleanAnd(left, right)


I consider that hideous and would prefer to write this:


        if (isinstance(left, PyCompare) and isinstance(right, PyCompare)
            and left.complist[-1] is right.complist[0]):
            py_and = PyCompare(left.complist + right.complist[1:]
        else:
            py_and = PyBooleanAnd(left, right)


Or even this:

        tmp = (
            isinstance(left, PyCompare) and isinstance(right, PyCompare) 
            and left.complist[-1] is right.complist[0])
            )
        if tmp:
            py_and = PyCompare(left.complist + right.complist[1:]
        else:
            py_and = PyBooleanAnd(left, right)


But perhaps the best solution is to define a helper function:

def is_next(left, right):
    """Returns True if right is the next PyCompare to left."""
    return (isinstance(left, PyCompare) and isinstance(right, PyCompare) 
        and left.complist[-1] is right.complist[0])
    # PEP 8 version left as an exercise.


# later...
        if is_next(left, right):
            py_and = PyCompare(left.complist + right.complist[1:]
        else:
            py_and = PyBooleanAnd(left, right)

-- 
Steven

[toc] | [prev] | [next] | [standalone]

#12258

On 27 August 2011 08:24, Steven D'Aprano
<steve+comp.lang.python@pearwood.info> wrote:
> Arnaud Delobelle wrote:
>
>> Hi all,
>>
>> I'm wondering what advice you have about formatting if statements with
>> long conditions (I always format my code to <80 colums)
>>
>> Here's an example taken from something I'm writing at the moment and
>> how I've formatted it:
>>
>>
>>         if (isinstance(left, PyCompare) and isinstance(right, PyCompare)
>>                 and left.complist[-1] is right.complist[0]):
>>             py_and = PyCompare(left.complist + right.complist[1:])
>>         else:
>>             py_and = PyBooleanAnd(left, right)
>>
>> What would you do?
>
> I believe that PEP 8 now suggests something like this:
>
>        if (
>                isinstance(left, PyCompare) and isinstance(right, PyCompare)
>                and left.complist[-1] is right.complist[0]):
>            )
>            py_and = PyCompare(left.complist + right.complist[1:]
>        else:
>            py_and = PyBooleanAnd(left, right)
>
>
> I consider that hideous and would prefer to write this:
>
>
>        if (isinstance(left, PyCompare) and isinstance(right, PyCompare)
>            and left.complist[-1] is right.complist[0]):
>            py_and = PyCompare(left.complist + right.complist[1:]
>        else:
>            py_and = PyBooleanAnd(left, right)
>
>
> Or even this:
>
>        tmp = (
>            isinstance(left, PyCompare) and isinstance(right, PyCompare)
>            and left.complist[-1] is right.complist[0])
>            )
>        if tmp:
>            py_and = PyCompare(left.complist + right.complist[1:]
>        else:
>            py_and = PyBooleanAnd(left, right)
>
>
> But perhaps the best solution is to define a helper function:
>
> def is_next(left, right):
>    """Returns True if right is the next PyCompare to left."""
>    return (isinstance(left, PyCompare) and isinstance(right, PyCompare)
>        and left.complist[-1] is right.complist[0])
>    # PEP 8 version left as an exercise.
>
>
> # later...
>        if is_next(left, right):
>            py_and = PyCompare(left.complist + right.complist[1:]
>        else:
>            py_and = PyBooleanAnd(left, right)
>

Thanks Steven and Hans for you suggestions.  For this particular
instance I've decided to go for a hybrid approach:

* Add two methods to PyCompare:

class PyCompare(PyExpr):
    ...
    def extends(self, other):
        if not isinstance(other, PyCompare):
            return False
        else:
            return self.complist[0] == other.complist[-1]
    def chain(self, other):
        return PyCompare(self.complist + other.complist[1:])

* Rewrite the if as:

        if isinstance(right, PyCompare) and right.extends(left):
            py_and = left.chain(right)
        else:
            py_and = PyBooleanAnd(left, right)


The additional benefit is to hide the implementation details of
PyCompare (I suppose this could illustrate the thread on when to
create functions).

-- 
Arnaud

[toc] | [prev] | [next] | [standalone]

#12261

Steven D'Aprano <steve+comp.lang.python@pearwood.info> writes:

> I believe that PEP 8 now

Specifically the “Indentation” section contains::

    When using a hanging indent the following considerations should be
    applied; there should be no arguments on the first line and further
    indentation should be used to clearly distinguish itself as a
    continuation line.

> suggests something like this:
>
>         if (
>                 isinstance(left, PyCompare) and isinstance(right, PyCompare)
>                 and left.complist[-1] is right.complist[0]):
>             )
>             py_and = PyCompare(left.complist + right.complist[1:]
>         else:
>             py_and = PyBooleanAnd(left, right)

That gives a SyntaxError. I think you mean one of these possible PEP 8
compliant forms::

    if (
            isinstance(left, PyCompare) and isinstance(right, PyCompare)
            and left.complist[-1] is right.complist[0]):
        py_and = PyCompare(left.complist + right.complist[1:]
    else:
        py_and = PyBooleanAnd(left, right)

or maybe::

    if (
            isinstance(left, PyCompare) and isinstance(right, PyCompare)
            and left.complist[-1] is right.complist[0]
            ):
        py_and = PyCompare(left.complist + right.complist[1:]
    else:
        py_and = PyBooleanAnd(left, right)

> I consider that hideous

I think both of those (once modified to conform to both the Python
syntax and the PEP 8 guidelines) look clear and readable.

I mildy prefer the first for being a little more elegant, but the second
is slightly better for maintainability and reducing diff noise. Either
one makes me happy.

> and would prefer to write this:
>
>         if (isinstance(left, PyCompare) and isinstance(right, PyCompare)
>             and left.complist[-1] is right.complist[0]):
>             py_and = PyCompare(left.complist + right.complist[1:]
>         else:
>             py_and = PyBooleanAnd(left, right)

That one keeps tripping me up because the indentation doesn't make clear
where subordinate clauses begin and end. The (current) PEP 8 rules are
much better for readability in my eyes.


Having said that, I'm only a recent convert to the current PEP 8 style
for indentation of condition clauses. It took several heated arguments
with colleagues before I was able to admit the superiority of clear
indentation :-)

-- 
 \      “I am too firm in my consciousness of the marvelous to be ever |
  `\       fascinated by the mere supernatural …” —Joseph Conrad, _The |
_o__)                                                     Shadow-Line_ |
Ben Finney

[toc] | [prev] | [next] | [standalone]

#12254

On 27/08/11 09:08:20, Arnaud Delobelle wrote:
> I'm wondering what advice you have about formatting if statements with
> long conditions (I always format my code to<80 colums)
>
> Here's an example taken from something I'm writing at the moment and
> how I've formatted it:
>
>
>          if (isinstance(left, PyCompare) and isinstance(right, PyCompare)
>                  and left.complist[-1] is right.complist[0]):
>              py_and = PyCompare(left.complist + right.complist[1:])
>          else:
>              py_and = PyBooleanAnd(left, right)
>
> What would you do?

I would break after the '(' and indent the condition once and
put the '):' bit on a separate line, aligned with the 'if':


           if (
               isinstance(left, PyCompare)
               and isinstance(right, PyCompare)
               and left.complist[-1] is right.complist[0]
           ):
               py_and = PyCompare(left.complist + right.complist[1:])
           else:
               py_and = PyBooleanAnd(left, right)

It may look ugly, but it's very clear where the condition part ends
and the 'then' part begins.

-- HansM

[toc] | [prev] | [next] | [standalone]

#12256

Hans Mulder wrote:

[...]
> It may look ugly, but it's very clear where the condition part ends
> and the 'then' part begins.

Immediately after the colon, surely?



-- 
Steven

[toc] | [prev] | [next] | [standalone]

#12259

On 27/08/11 11:05:25, Steven D'Aprano wrote:
> Hans Mulder wrote:
>
> [...]
>> It may look ugly, but it's very clear where the condition part ends
>> and the 'then' part begins.
>
> Immediately after the colon, surely?

On the next line, actually :-)

The point is, that this layout makes it very clear that the colon
isn't in its usual position (at the end of the line that starts
with 'if') and it is clearly visible.

With the layout Arnaud originally propose, finding the colon takes
longer.  (Arnaud has since posted a better approach, in which the
colon is back in its usual position.)

-- HansM

[toc] | [prev] | [next] | [standalone]

#12267

On 27-Aug-11 03:50 AM, Hans Mulder wrote:
> On 27/08/11 09:08:20, Arnaud Delobelle wrote:
>> I'm wondering what advice you have about formatting if statements with
>> long conditions (I always format my code to<80 colums)
>>
>> Here's an example taken from something I'm writing at the moment and
>> how I've formatted it:
>>
>>
>> if (isinstance(left, PyCompare) and isinstance(right, PyCompare)
>> and left.complist[-1] is right.complist[0]):
>> py_and = PyCompare(left.complist + right.complist[1:])
>> else:
>> py_and = PyBooleanAnd(left, right)
>>
>> What would you do?
>
> I would break after the '(' and indent the condition once and
> put the '):' bit on a separate line, aligned with the 'if':
>
>
> if (
> isinstance(left, PyCompare)
> and isinstance(right, PyCompare)
> and left.complist[-1] is right.complist[0]
> ):
> py_and = PyCompare(left.complist + right.complist[1:])
> else:
> py_and = PyBooleanAnd(left, right)
>
> It may look ugly, but it's very clear where the condition part ends
> and the 'then' part begins.
>
> -- HansM

What about:
           cond=  isinstance(left, PyCompare)
                  and isinstance(right, PyCompare)
                  and left.complist[-1] is right.complist[0]
           py_and= PyCompare(left.complist + right.complist[1:])if cond
                   else: py_and = PyBooleanAnd(left, right)
Colin W.

[toc] | [prev] | [next] | [standalone]

#12268

On 27/08/11 17:16:51, Colin J. Williams wrote:

> What about:
> cond= isinstance(left, PyCompare)
>       and isinstance(right, PyCompare)
>       and left.complist[-1] is right.complist[0]
> py_and= PyCompare(left.complist + right.complist[1:])if cond
>       else: py_and = PyBooleanAnd(left, right)
> Colin W.

That's a syntax error.  You need to add parenthesis.

How about:

cond = (
     isinstance(left, PyCompare)
     and isinstance(right, PyCompare)
     and left.complist[-1] is right.complist[0]
}
py_and = (
          PyCompare(left.complist + right.complist[1:])
     if   cond
     else PyBooleanAnd(left, right)
)

-- HansM

[toc] | [prev] | [next] | [standalone]

#12305

On 27-Aug-11 11:53 AM, Hans Mulder wrote:
> On 27/08/11 17:16:51, Colin J. Williams wrote:
>
>> What about:
>> cond= isinstance(left, PyCompare)
>> and isinstance(right, PyCompare)
>> and left.complist[-1] is right.complist[0]
>> py_and= PyCompare(left.complist + right.complist[1:])if cond
>> else: py_and = PyBooleanAnd(left, right)
>> Colin W.
>
> That's a syntax error. You need to add parenthesis.
>
> How about:
>
> cond = (
> isinstance(left, PyCompare)
> and isinstance(right, PyCompare)
> and left.complist[-1] is right.complist[0]
> }
> py_and = (
> PyCompare(left.complist + right.complist[1:])
> if cond
> else PyBooleanAnd(left, right)
> )
>
> -- HansM

I like your 11:53 message but suggest indenting the if cond as below to 
make it clearer that it, with the preceding line, is all one statement.

Colin W.

#!/usr/bin/env python
z= 1
class PyCompare:
     complist = [True, False]
     def __init__(self):
         pass
left= PyCompare
right= PyCompare
def isinstance(a, b):
     return True
def PyBooleanAnd(a, b):
     return True
def PyCompare(a):
     return False
z=2

def try1():

       '''    Hans Mulder suggestion  03:50  '''
       if (
           isinstance(left, PyCompare)
           and isinstance(right, PyCompare)
           and left.complist[-1] is right.complist[0]
       ):
           py_and = PyCompare(left.complist + right.complist[1:])
       else:
           py_and = PyBooleanAnd(left, right)

def try2():
       '''        cjw response - corrected  11:56  '''
       cond=  (isinstance(left, PyCompare)
              and isinstance(right, PyCompare)
              and left.complist[-1] is right.complist[0])
       py_and= (PyCompare(left.complist + right.complist[1:]) if cond
               else PyBooleanAnd(left, right))

def try3():
     '''       Hans Mulder 11:53   '''
     cond = (
         isinstance(left, PyCompare)
         and isinstance(right, PyCompare)
         and left.complist[-1] is right.complist[0]
         )  # not }
     py_and = (
              PyCompare(left.complist + right.complist[1:])
              if   cond
              else PyBooleanAnd(left, right)
             )
def main():
     try1()
     try2()
     try3()
if __name__ == '__main__':
     main()
     pass

[toc] | [prev] | [next] | [standalone]

#12269

In article <mailman.457.1314428909.27778.python-list@python.org>,
 Arnaud Delobelle <arnodel@gmail.com> wrote:

> Hi all,
> 
> I'm wondering what advice you have about formatting if statements with
> long conditions (I always format my code to <80 colums)
> [...]
>         if (isinstance(left, PyCompare) and isinstance(right, PyCompare)
>                 and left.complist[-1] is right.complist[0]):
>             py_and = PyCompare(left.complist + right.complist[1:])
>         else:
>             py_and = PyBooleanAnd(left, right)

To tie this into the ongoing, "When should I write a new function?" 
discussion, maybe the right thing here is to refactor all of that mess 
into its own function, so the code looks like:

   if _needs_compare(left, right):
         py_and = PyCompare(left.complist + right.complist[1:])
    else:
         py_and = PyBooleanAnd(left, right)

and then

def _needs_compare(left, right):
   "Decide if we need to call PyCompare"
   return isinstance(left, PyCompare) and \
          isinstance(right, PyCompare) and \
          left.complist[-1] is right.complist[0]

This seems easier to read/understand than what you've got now.  It's an 
even bigger win if this will get called from multiple places.

[toc] | [prev] | [standalone]

Back to top | Article view | comp.lang.python

csiph-web