refactor: encapsulation #3017
refactor: encapsulation #3017
Conversation
| @@ -51,3 +51,7 @@ func (f *folder) scanSubdirsIfHealthy(subDirs []string) error { | |||
| } | |||
| return nil | |||
| } | |||
|
|
|||
| func (f *folder) scanner() *folderScanner { | |||
| return &f.scan | |||
There was a problem hiding this comment.
So I think this is worse.
I think what @calmh ment by encapsulation was doing
func (f *folder) Scan() {
f.scan.Scan()
}
but I might be wrong.
Also, why just not embed scan into folder?
There was a problem hiding this comment.
I dont think this is worse. It is meant to be a fluent api implementing a getter for the folderScanner. The only thing is that I made a mistake using the wrong idioms because I'm not familiar with golang yet.
If I had used your suggestions ...
- it would be confusing with the
func (f *folder) Scan(subdirs []string) error - and the problem with accessing fields directly would not be solved.
I understood @calmh 's hint, to implement to OOP style. With which I fully agree on. This means using methods to access fields to not undermine any future refactoring and to keep stable contracts.
There was a problem hiding this comment.
I guess @calmh needs to clarify what he is after
There was a problem hiding this comment.
@calmh Plz would you have a look at this?
| @@ -191,7 +191,7 @@ func (f *rwFolder) Serve() { | |||
| l.Debugln(f, "skip (initial)") | |||
| f.pullTimer.Reset(f.sleep) | |||
| } else { | |||
| prevVer, prevIgnoreHash = f.onExpiredPullTimerfunc(prevVer, prevIgnoreHash) | |||
| prevVer, prevIgnoreHash = f.updatePreviousVersionAndPreviousIgnoreHashOnExpiredPullTimer(prevVer, prevIgnoreHash) | |||
| } | |||
There was a problem hiding this comment.
Sry for that long name updatePreviousVersionAndPreviousIgnoreHashOnExpiredPullTimer. This gives me a hint there is more than one responsibility in this method. Just stopped here for a while.
|
I think the gains would come from moving the actual scanning functionality into the |
# Conflicts: # lib/model/rofolder.go # lib/model/rwfolder.go
|
The indirect access is the thinking of object oriented programming. The |
| return paused | ||
| defer m.pmut.Unlock() | ||
|
|
||
| return m.devicePaused[device] |
There was a problem hiding this comment.
defer kills potential inlining by the compiler (or so I've been told), and given this is small, it looks like a good candidate for inlining.
There was a problem hiding this comment.
ok. But how can I verify this? (Seems a trap of premature optimization, if it is wrong)
There was a problem hiding this comment.
root@audrius:~# cat test.go
package main
func foo() int {
return 0
}
func main() {
foo()
}
root@audrius:~# go build -gcflags '-m' test.go
# command-line-arguments
./test.go:3: can inline foo
./test.go:7: can inline main
./test.go:8: inlining call to foo
root@audrius:~# cat test.go
package main
func foo() int {
defer func(){}()
return 0
}
func main() {
foo()
}
root@audrius:~# go build -gcflags '-m' test.go
# command-line-arguments
./test.go:4: can inline foo.func1
./test.go:4: foo func literal does not escape
Yet it seems that anything that involves a mutex escapes to heap killing inlining.
# Conflicts: # lib/model/rwfolder.go
|
@AudriusButkevicius please have a look what is still missing. But consider to request to much changes in another PR ;) |
|
LGTM, but I'llleave for @calmh to merge |
|
bump |
|
@st-jenkins retest this please |
| @@ -981,6 +981,20 @@ func (m *Model) GetIgnores(folder string) ([]string, []string, error) { | |||
| return lines, patterns, nil | |||
| } | |||
|
|
|||
| func (m *Model) GetFolderFiles(folderID string) *db.FileSet { | |||
There was a problem hiding this comment.
These look like dead code to me, but apparently not to the metalinter. Am I missing something?
There was a problem hiding this comment.
I just checked metalinter and tried an update. But nothing helps. I dont know.
There was a problem hiding this comment.
It's an exported method. Those can't be dead code eliminated so it makes sense that metalinter doesn't see this, when thinking about it. :)
| @@ -981,6 +981,13 @@ func (m *Model) GetIgnores(folder string) ([]string, []string, error) { | |||
| return lines, patterns, nil | |||
| } | |||
|
|
|||
| func (m *Model) GetIgnoreMatcher(folderID string) *ignore.Matcher { | |||
|
@st-review merge it lib/model: Refactor encapsulation of the folder scanning |
st-review
added
the
frozen-due-to-age
Purpose
Refactoring hints from PR #3007
Testing
compile and run tests