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

Vratislav Podzimek vpodzime at redhat.com
Tue Apr 15 11:58:09 UTC 2014


On Tue, 2014-04-15 at 07:48 -0400, Anne Mulhern wrote:
> 
> 
> 
> ----- 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.
> >
> I didn't want to get anything started, but now that it has been...
You should definitely start something if you think the change is doing
something wrong. ;)

> 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.
Seems like its 2:1 so I'm, gonna drop this patch. From my point of view,
it adds something like encapsulation and splits the logic into more
smaller pieces -- 'if something_changed do something' and at this place
in the code I don't care about how something_changed gets set I only
care about the code below if it really does the stuff that should be
done if something has changed.

-- 
Vratislav Podzimek

Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic



More information about the anaconda-patches mailing list