Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactoring: early return 'if else' syntax -> 'if' syntax #167

Open
wants to merge 1 commit into
base: v2
Choose a base branch
from

Conversation

marload
Copy link

@marload marload commented Apr 11, 2020

I think 'if syntax' is better than 'if else syntax' in the early return pattern.
Thanks You!

Copy link
Collaborator

@tomhennigan tomhennigan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 if return else return is preferable vs. if return; return so I would be hesitant about applying this consistently in Sonnet.

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:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Thanks You!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, please feel free to push a new commit with these changes so we can review and get them merged 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants