[PATCH 4/9] Split complex conditions into multiple lines with named parts

Anne Mulhern amulhern at redhat.com
Tue Apr 15 11:48:36 UTC 2014





----- Original Message -----
> From: "Vratislav Podzimek" <vpodzime at redhat.com>
> To: "anaconda patch review" <anaconda-patches at lists.fedorahosted.org>
> Sent: Tuesday, April 15, 2014 2:28:26 AM
> Subject: Re: [PATCH 4/9] Split complex conditions into multiple lines with	named parts
> 
> On Mon, 2014-04-14 at 17:57 -0700, Brian C. Lane wrote:
> > On Mon, Apr 14, 2014 at 06:03:19PM +0200, Vratislav Podzimek wrote:
> > > This way they are better readable.
> > > 
> > > Signed-off-by: Vratislav Podzimek <vpodzime at redhat.com>
> > > ---
> > >  pyanaconda/ui/gui/spokes/custom.py | 19 +++++++++++--------
> > >  1 file changed, 11 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/pyanaconda/ui/gui/spokes/custom.py
> > > b/pyanaconda/ui/gui/spokes/custom.py
> > > index 0008e14..de8af13 100644
> > > --- a/pyanaconda/ui/gui/spokes/custom.py
> > > +++ b/pyanaconda/ui/gui/spokes/custom.py
> > > @@ -1701,9 +1701,9 @@ class CustomPartitioningSpoke(NormalSpoke,
> > > StorageChecker):
> > >              return
> > >  
> > >          # adjust container to size of remaining devices, if auto-sized
> > > -        if container and not container.exists and \
> > > -           self._storage_playground.devicetree.getChildren(container)
> > > and \
> > > -           container.size_policy == SIZE_POLICY_AUTO:
> > > +        have_children =
> > > self._storage_playground.devicetree.getChildren(container)
> > > +        auto_size = container.size_policy == SIZE_POLICY_AUTO
> > > +        if container and not container.exists and have_children and
> > > auto_size:
> > >              cont_encrypted = container.encrypted
> > >              cont_raid = get_raid_level(container)
> > >              cont_size = container.size_policy
> > > @@ -1902,11 +1902,14 @@ class CustomPartitioningSpoke(NormalSpoke,
> > > StorageChecker):
> > >              self.window.show_all()
> > >              return
> > >  
> > > -        if (set(disks) != set(self._device_disks) or
> > > -            name != container_name or
> > > -            dialog.raid_level != self._device_container_raid_level or
> > > -            dialog.encrypted != self._device_container_encrypted or
> > > -            dialog.size_policy != self._device_container_size):
> > > +        new_disks = set(disks) != set(self._device_disks)
> > > +        new_name = name != container_name
> > > +        new_raid = dialog.raid_level !=
> > > self._device_container_raid_level
> > > +        new_encrypted = dialog.encrypted !=
> > > self._device_container_encrypted
> > > +        new_size = dialog.size_policy != self._device_container_size
> > > +        something_changed = new_disks or new_name or new_raid or \
> > > +                            new_encrypted or new_size
> > > +        if something_changed:
> > >              self._applyButton.set_sensitive(True)
> > >  
> > >          log.debug("new container raid level: %s", dialog.raid_level)
> > > --
> > > 1.9.0
> > 
> > I'm not sure I like this change. It *looks* cleaner, but when I'm
> > reading through it now I have 2 levels of things to remember instead of
> > one.
> Do you mean the first change, the latter one or both?
> 
> --
> Vratislav Podzimek
> 
> Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic
> 
> _______________________________________________
> anaconda-patches mailing list
> anaconda-patches at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
> 

I didn't want to get anything started, but now that it has been...
It feels less clear to me _unless_ you separated that long condition (2nd chunk)
into it's own method, in which case it would feel more clear.

- mulhern


More information about the anaconda-patches mailing list