Minor refactor (no behavior change) to simplify and clean up. #596

Merged
merged 9 commits into from Feb 26, 2017

Conversation

Projects
None yet
2 participants
@dmitshur
Member

dmitshur commented Feb 25, 2017

This PR is a minor refactor (no behavior change) to simplify and clean up some of the code I've recently seen/looked over, and found opportunities to improve. Some of the changes are ones I wanted to make while working on #595, and some others I just did on the fly when making this PR.

Please review commit-by-commit. Each commit contains a readable bite-sized changed, and the commit message describes the motivation and rationale. Please leave comments if you disagree with something or have better suggestions.

There should not be any provable behavior changes. If there are, that's a bug.

dmitshur added some commits Feb 25, 2017

Move sprintError under printError.
This is a more logical arrangement. It goes from top to bottom, where
higher level functionality (handleError) is on top, then its private
helpers (printError), then helper of helper (sprintError). sprintError
is only called by printError.
Simplify handleError signature to accept error value.
Change handleError to accept err error value rather than a func f that
returns error, and always executing f anyway.

It's simpler and more readable for the caller to execute f, rather than
pass it to handleError for execution.

Now handleError responsibility is to really only handle the error,
rather than execute arbitrary code in addition to handling error.
Use simpler, more readable and consistent control flow.
Execute functionality to get an error, handle the error to get an exit
code, and then exit.

Do that instead of trying to exit with the exit code that is the result
of handling an error of executing a closure.

It's equivalent behavior, but the new version is cleaner IMO.

This is also more consistent with the other occurrences of same pattern.
Execute closure first, handle error after.
This leads to more readable code for someone who doesn't have it
memorized what handleError does.
Use err instead of exit code.
It's equivalent functionality, but simpler and more readable to someone
not familiar with handleError. That person doesn't need to jump inside
handleError and understand how it works in order to understand when
exit code will be 0 or non-zero. It's simpler to just use the err !=
nil value right away (without processing it via a middleware func).
Replace 'bytes.NewBuffer(nil) -> new(bytes.Buffer)'.
It's a more idiomatic way of creating a new empty buffer (pointer).

https://godoc.org/bytes#NewBuffer says:

> In most cases, new(Buffer) (or just declaring a Buffer variable) is
> sufficient to initialize a Buffer.
Remove closure for gopherjs doc command.
It was not helpful. There was only one return statement (effectively).
The code is simpler without it.
Simplify opportunities found with gosimple.
	$ gosimple ./...
	tool.go:125:6: 'if err != nil { return err }; return nil' can be simplified to 'return err' (S1013)
	tool.go:404:6: 'if err != nil { return err }; return nil' can be simplified to 'return err' (S1013)
	tool.go:517:59: should use time.Since instead of time.Now().Sub (S1012)
	compiler/compiler.go:171:2: 'if err != nil { return err }; return nil' can be simplified to 'return err' (S1013)
	third_party/importer/export.go:431:5: should use strings.ContainsAny(format, "{}\n") instead (S1003)
	third_party/importer/import.go:55:2: should replace loop with p.typList = append(p.typList, predeclared...) (S1011)

@dmitshur dmitshur requested a review from neelance Feb 25, 2017

@neelance

These are good changes, thx for doing some cleanup. I agree with all except the parts of the last commit that I commented on. Let's talk about that one.

compiler/compiler.go
- }
-
- return nil
+ _, err := w.Write([]byte("$synthesizeMethods();\nvar $mainPkg = $packages[\"" + string(mainPkg.ImportPath) + "\"];\n$packages[\"runtime\"].$init();\n$go($mainPkg.$init, []);\n$flushConsole();\n\n}).call(this);\n"))

This comment has been minimized.

@neelance

neelance Feb 26, 2017

Member

I don't like this simplification, I often even change my code to not do this. I prefer having the primary flow with return nil (all okay) at the toplevel and have return err only in branches for error handling. This also allows cleaner diffs when adding new stuff between this statement and the end of the function. Same goes for other such changes in this commit.

@neelance

neelance Feb 26, 2017

Member

I don't like this simplification, I often even change my code to not do this. I prefer having the primary flow with return nil (all okay) at the toplevel and have return err only in branches for error handling. This also allows cleaner diffs when adding new stuff between this statement and the end of the function. Same goes for other such changes in this commit.

This comment has been minimized.

@dmitshur

dmitshur Feb 26, 2017

Member

Up until about a year ago, I used to feel the same way. But I saw that the Go standard library did it, and I decided to try it after all. Then this simplification grew on me.

Now, when I read Go code, seeing return err at the end of a function is absolutely equivalent and interchangeable with if err != nil { return err }; return nil, except it avoids being unnecessarily more verbose.

I've also started to not mind diffs. Unless the code happens to be very very WIP, diffs are actually pretty rare. So I'd rather look at the shorter equivalent version for a year before a change, and when a chance is neccessary, the diff is not at all that much worse (i.e., I find it very readable too).

In some places, I still prefer to do err := DoSomething(); return err instead of just return DoSomething(), when I want to emphasize that doing the thing is more important than returning its error value. But even that is almost interchangeable for me now, and that's why I wrote return s.BuildFiles(args, pkgObj, currentDirectory) below, instead of err = s.BuildFiles(args, pkgObj, currentDirectory); return err.

But okay, I will back out this change for now since you prefer the if err != nil { return err }; return nil version and we can always change it later if you change your mind.

(/cc @dominikh FYI, this is related to S1013.)

@dmitshur

dmitshur Feb 26, 2017

Member

Up until about a year ago, I used to feel the same way. But I saw that the Go standard library did it, and I decided to try it after all. Then this simplification grew on me.

Now, when I read Go code, seeing return err at the end of a function is absolutely equivalent and interchangeable with if err != nil { return err }; return nil, except it avoids being unnecessarily more verbose.

I've also started to not mind diffs. Unless the code happens to be very very WIP, diffs are actually pretty rare. So I'd rather look at the shorter equivalent version for a year before a change, and when a chance is neccessary, the diff is not at all that much worse (i.e., I find it very readable too).

In some places, I still prefer to do err := DoSomething(); return err instead of just return DoSomething(), when I want to emphasize that doing the thing is more important than returning its error value. But even that is almost interchangeable for me now, and that's why I wrote return s.BuildFiles(args, pkgObj, currentDirectory) below, instead of err = s.BuildFiles(args, pkgObj, currentDirectory); return err.

But okay, I will back out this change for now since you prefer the if err != nil { return err }; return nil version and we can always change it later if you change your mind.

(/cc @dominikh FYI, this is related to S1013.)

This comment has been minimized.

@neelance

neelance Feb 26, 2017

Member

I'm also open to changing my mind, that's why I suggested to talk about it.

@neelance

neelance Feb 26, 2017

Member

I'm also open to changing my mind, that's why I suggested to talk about it.

This comment has been minimized.

@dmitshur

dmitshur Feb 26, 2017

Member

I find these kind of things are best to do without pressure, on your own time in the background, and not let other code improvements be blocked on talking about it. Consider the points I made and see how you feel later on.

My main argument for why the simpler form is better is because it's equivalent, shorter, and done more commonly in the Go project (hence, more idiomatic).

@dmitshur

dmitshur Feb 26, 2017

Member

I find these kind of things are best to do without pressure, on your own time in the background, and not let other code improvements be blocked on talking about it. Consider the points I made and see how you feel later on.

My main argument for why the simpler form is better is because it's equivalent, shorter, and done more commonly in the Go project (hence, more idiomatic).

@dmitshur dmitshur merged commit 9853efb into master Feb 26, 2017

1 check was pending

ci/circleci CircleCI is running your tests
Details

@dmitshur dmitshur deleted the refactor-gopherjs-tool branch Feb 26, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment