[Arch] Suspicious duplicated code

A forum dedicated to the Draft, Arch and BIM workbenches development.
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
Post Reply
User avatar
chennes
Veteran
Posts: 3878
Joined: Fri Dec 23, 2016 3:38 pm
Location: Norman, OK, USA
Contact:

[Arch] Suspicious duplicated code

Post by chennes »

In ArchPanel.py, the function getWires() begins:

Code: Select all

1088        tag = None
1089        outl = None
1090        inl = None
1091        if not hasattr(self,"outline"):
1092            self.execute(obj)
1093        if not hasattr(self,"outline"):
1094            return None
1095        outl = self.outline.copy()
1096        if hasattr(self,"tag"):
1097            tag = self.tag.copy()
1098        if tag:
1099            tag.Placement = obj.Placement.multiply(tag.Placement)
1100
1101        outl = self.outline.copy()
1102        outl.Placement = obj.Placement.multiply(outl.Placement)
Lines 1095 and 1101 are the same (which means that the variable outl is overwritten before first use, the one on line 1095 is never used -- which is how I find myself looking at this code, because static analyzers don't like that). So: is that double copy correct? It's easy enough to silence the warning by changing 1095 to _ = self.outline.copy(), but I don't want to silence the warning if there is in fact something wrong here.
Chris Hennes
Pioneer Library System
GitHub profile, LinkedIn profile, chrishennes.com
carlopav
Veteran
Posts: 2062
Joined: Mon Dec 31, 2018 1:49 pm
Location: Venice, Italy

Re: [Arch] Suspicious duplicated code

Post by carlopav »

Thanks for the finding and for the detailed explanation. I never used much the Arch_Panel tool, but i suspect it wont be a problem to refactor it. Hereafter a proposed refactor of the whole function:

Code: Select all

    def getWires(self, obj):

        """getWires(obj): returns a tuple containing 3 shapes
        that define the panel outline, the panel holes, and
        tags (engravings): (outline,holes,tags). Any of these can
        be None if nonexistent"""

        # compute outl and inl
        if not hasattr(self,"outline"):
            self.execute(obj)
        if not hasattr(self,"outline"):
            return None
        inl = None
        outl = self.outline.copy()
        outl.Placement = obj.Placement.multiply(outl.Placement)
        if len(outl.Wires) > 1:
            # separate outline
            d = 0
            ow = None
            for w in outl.Wires:
                if w.BoundBox.DiagonalLength > d:
                    d = w.BoundBox.DiagonalLength
                    ow = w
            if ow:
                inl = Part.Compound([w for w in outl.Wires if w.hashCode() != ow.hashCode()])
                outl = Part.Compound([ow])
        else:
            inl = None
            outl = Part.Compound([outl.Wires[0]])
            
        # compute tag
        tag = getattr(self, "tag", None)
        if tag:
            tag = self.tag.copy()
            tag.Placement = obj.Placement.multiply(tag.Placement)

        return (outl, inl, tag)
Anyway better to have some more feedback!
follow my experiments on BIM modelling for architecture design
Post Reply