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 codes #151

Closed
wants to merge 8 commits into from

Conversation

@osamingo
Copy link

commented Aug 1, 2019

The title said all.

osamingo added some commits Aug 1, 2019

@osamingo

This comment has been minimized.

Copy link
Author

commented Aug 1, 2019

@dsnet Please review it if you are free.

@dsnet

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

Thanks for the PR. Some of the changes made:

  • are un-doing deliberate style choices made by the original author
  • are already fixed by another contributor (i.e. #148 and #149)

After those two, there isn't much left to this PR.

@dsnet dsnet closed this Aug 1, 2019

@@ -322,7 +322,7 @@ func coalesceInterveningIdentical(groups []diffStats, windowSize int) []diffStat
hadX, hadY := prev.NumRemoved > 0, prev.NumInserted > 0
hasX, hasY := next.NumRemoved > 0, next.NumInserted > 0
if ((hadX || hasX) && (hadY || hasY)) && curr.NumIdentical <= windowSize {
*prev = (*prev).Append(*curr).Append(*next)
*prev = prev.Append(*curr).Append(*next)

This comment has been minimized.

Copy link
@dsnet

dsnet Aug 1, 2019

Member

Already covered by #149.

}
return Result{NumDiff: 2} // Not Equal, not Similar

This comment has been minimized.

Copy link
@dsnet

dsnet Aug 1, 2019

Member

This is deliberate.

Both branches have equal probability of being executed. They are kept at the same indentation for that purpose.

@@ -11,7 +11,7 @@ import "reflect"
func IsZero(v reflect.Value) bool {
switch v.Kind() {
case reflect.Bool:
return v.Bool() == false
return !v.Bool()

This comment has been minimized.

Copy link
@dsnet

dsnet Aug 1, 2019

Member

This is deliberate. It keeps consistency with the code below.

return textEllipsis
} else if outx.Len() < outy.Len() {
case outx.Len() < outy.Len():

This comment has been minimized.

Copy link
@dsnet

dsnet Aug 1, 2019

Member

This is deliberate.

Both branches have equal probability of being executed. Keeping them on the same indent highlights this fact.

case diffIdentical:
isZero = value.IsZero(r.Value.ValueX) || value.IsZero(r.Value.ValueX)
case diffRemoved:
case diffIdentical, diffRemoved:

This comment has been minimized.

Copy link
@dsnet

dsnet Aug 1, 2019

Member

This is actually a bug, fixed in #148.

@@ -365,7 +365,7 @@ func (s diffStats) String() string {
// Pluralize the name (adjusting for some obscure English grammar rules).
name := s.Name
if sum > 1 {
name = name + "s"
name += "s"

This comment has been minimized.

Copy link
@dsnet

dsnet Aug 1, 2019

Member

Fixed in #149.

@@ -170,7 +170,7 @@ func canonicalName(t reflect.Type, sel string) ([]string, error) {
if !ok {
return []string{name}, fmt.Errorf("does not exist")
}
var ss []string
ss := make([]string,0,len(sf.Index))

This comment has been minimized.

Copy link
@dsnet

dsnet Aug 1, 2019

Member

I'm not fond of up-front slice allocation unless the code has shown a performance benefit from doing so.
None of these slice usages are in the hot-path. Thus, the readability loss is not worth the supposed performance gains.

@osamingo osamingo deleted the osamingo:refactoring-by-linters branch Aug 2, 2019

@osamingo

This comment has been minimized.

Copy link
Author

commented Aug 2, 2019

@dsnet
No problem, thanks for your review. I feel nice other PRs were merged :)
Please publish the new release, for Go module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.