Giuseppe Vallarelli has posted comments on this change. Change subject: WIP: Multiple Gateways Feature ......................................................................
Patch Set 10: (1 inline comment) .................................................... File vdsm/sourceRoute.py Line 90: Line 91: self.routes = self._buildRoutes() Line 92: self.rules = self._buildRules() Line 93: Line 94: if self.isStatic: I'm glad you're aware of the issue, it's much easier to split it up right now, by keeping all together in my opinion is less maintainable. When new requirements and new code will be added it will be much easier to stick to the wrong path, putting everything together. Keeping everything in one class will never make the code simpler. If you split up in 2 classes StaticSourceRoute and DymanicSourceRoute you will use the if isStatic, only once when you need to decide which object to create. Later on in the codebase you simply care that you got a kind of SourceRoute which is able to answer to configure, remove and so on, the if isStatic is really unnecessary complexity. If you keep it, in the future is highly probable that we will need to use this distinction between kind of routes (if route.isStatic: do this else do that) to trigger new behaviour and satisfy new requirements. Line 95: self._addRoutesStatic() Line 96: self._addRulesStatic() Line 97: else: Line 98: self._addRoutesDHCP() -- To view, visit http://gerrit.ovirt.org/15207 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0224d896724b9cdc44215e92f0da0be71fd19038 Gerrit-PatchSet: 10 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Assaf Muller <[email protected]> Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: Assaf Muller <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Giuseppe Vallarelli <[email protected]> Gerrit-Reviewer: Livnat Peer <[email protected]> Gerrit-Reviewer: Mark Wu <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
