Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
refactoring: early return 'if else' syntax -> 'if' syntax #167
Conversation
|
Thanks for the patch! Code readability is really important and we really appreciate you taking the time to help us improve it. For these specific cases we don't have a well established style guide saying whether However I think in this case you have identified a case where we can avoid repeating ourselves which is (in my experience) nearly always a good thing. PTAL at the suggested edit and see if you agree! |
| @@ -125,8 +125,7 @@ def __call__(self, inputs: tf.Tensor, multiplier: types.FloatLike = None): | |||
| self._initialize(inputs) | |||
| if multiplier is not None: | |||
tomhennigan
Apr 17, 2020
Collaborator
So it seems to me we have a pattern of :
if A:
return B + (C * D)
else:
return B + D
I think perhaps in this case, if we want to avoid the extra else perhaps we should extract more of the common part of the two expressions (e.g. the if A block should just scale the thing we add to B.
if A:
D = C * D
return B + D
Concretely in this case it would look like the following (I think we cannot use *= because this is not supported on tf.Variable):
b = self.b
if multiplier is not None:
b = b * multiplier
return inputs + b
So it seems to me we have a pattern of :
if A:
return B + (C * D)
else:
return B + DI think perhaps in this case, if we want to avoid the extra else perhaps we should extract more of the common part of the two expressions (e.g. the if A block should just scale the thing we add to B.
if A:
D = C * D
return B + DConcretely in this case it would look like the following (I think we cannot use *= because this is not supported on tf.Variable):
b = self.b
if multiplier is not None:
b = b * multiplier
return inputs + b
marload
Apr 19, 2020
Author
I agree. Thanks You!
I agree. Thanks You!
tomhennigan
Apr 20, 2020
Collaborator
Sounds good, please feel free to push a new commit with these changes so we can review and get them merged 😄
Sounds good, please feel free to push a new commit with these changes so we can review and get them merged
I think 'if syntax' is better than 'if else syntax' in the early return pattern.
Thanks You!