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

[建議] 把 interface 宣告移到使用者處,並移除非必要的 interface #71

Open
lsc36 opened this issue Feb 16, 2021 · 13 comments
Labels
enhancement New feature or request

Comments

@lsc36
Copy link

lsc36 commented Feb 16, 2021

原本作法的問題 / Existed Problem

Go wiki 建議把 interface 宣告在使用它的 package 而非實作它的 package,這樣可以避免不必要的 dependency 以及未來 refactor 的成本。
依此原則,例如 Repository 應該被宣告在 package usecase 而非 package repository
若將來有多個 package 需要共用同一個 interface 宣告,再把它移到獨立的 package 即可。
(一樣原則是放在使用者這邊而非實作者)

另外還有像 UsecaseLogger 之類目前看起來沒有直接用途的 interface,建議可以先移除。

實作細節 / Details of Implement

檢視現有的 interface,如果沒有直接用途就改用一般的 struct,視情況改寫 test。
把有必要存在的 interface 宣告移動到使用它的 package。

期程 / Schedule

  • 討論時間:一週 (review 現有的 code)
  • 實作時間:一週

相關文件 / Documents

@lsc36 lsc36 added the enhancement New feature or request label Feb 16, 2021
@PichuChen
Copy link
Member

我覺得這個做法應該是比較正確的,不過應該不是「例如 Repository 應該被宣告在 package usecase」 而是 usecase 要去實作某個 interface 然後讓 repository 來實作他, usecase 使用這個 interface, 而這個 interface 的名字應該不是 repository,可能是按照實際需求的小 interface 這樣。

@PichuChen
Copy link
Member

然後 Logger 的 interface 我看法是傾向保留,否則就變成要在各個 package 裡面去定義自己的 logger interface,感覺會怪怪的。

相較之下是接下來假如有其他的 Logger 實作的時候新的 Logger 實作不再去定義自己的 interface 而是使用原有的 Logger interface

@PichuChen
Copy link
Member

再來是看起來並不是每個使用者都需要去定義 interface ,不然在 HTTP 的建議實作就會變成要求 application 都實作自己的 ResponseWriter 而不是直接使用 http.ResponseWriter 了

@lsc36
Copy link
Author

lsc36 commented Feb 16, 2021

我覺得這個做法應該是比較正確的,不過應該不是「例如 Repository 應該被宣告在 package usecase」 而是 usecase 要去實作某個 interface 然後讓 repository 來實作他, usecase 使用這個 interface, 而這個 interface 的名字應該不是 repository,可能是按照實際需求的小 interface 這樣。

可以這樣拆分的話最好,usecase 那邊我還沒看得很仔細。

然後 Logger 的 interface 我看法是傾向保留,否則就變成要在各個 package 裡面去定義自己的 logger interface,感覺會怪怪的。

相較之下是接下來假如有其他的 Logger 實作的時候新的 Logger 實作不再去定義自己的 interface 而是使用原有的 Logger interface

目前好像沒有把 Logger 作為 interface 的必要,直接就把現在的實作 logger struct{} 當成 Logger 應該不會有問題?

再來是看起來並不是每個使用者都需要去定義 interface ,不然在 HTTP 的建議實作就會變成要求 application 都實作自己的 ResponseWriter 而不是直接使用 http.ResponseWriter 了

沒錯,不過我想等到需要的時候再調整就好。From Go wiki:

Do not define interfaces before they are used: without a realistic example of usage, it is too difficult to see whether an interface is even necessary, let alone what methods it ought to contain.

@mkfsn
Copy link
Contributor

mkfsn commented Feb 17, 2021

@lsc36

Go wiki 建議把 interface 宣告在使用它的 package 而非實作它的 package,這樣可以避免不必要的 dependency 以及未來 refactor 的成本。

同意。不過當初設計 usecase/repository interface 的原因可以去參考一下 #16 ,或許對於初期來說有點 over engineering 但是當初的想法是先把 business 相關的邏輯(利用 interface )定義出來,然後把 database layer (一樣利用 interface )切開來,至於擺放的位置就沒有放好,非常抱歉 🙇🏻

Logger 的話我覺得可以保留也可以換掉,但是可以的話會希望我們討論完 #21 之後再決定要不要改,畢竟如果現在換掉 Logger interface 然後之後發現換了 logger 要大改,又因為 logger 有可能會分散在 code 各個地方,所以我不確定現在是不是一個好的時機去換掉 Logger interface (雖然看 PR 應該是沒什麼問題就是了 XD )。再提一下,當初做這個 Logger interface 的原因也是為了之後不曉得要用哪個 logging package 而準備的,這樣使用 logger 的地方就不用換 method,而是在實作的地方去改。

感謝您指出 interface 放置位置的問題,這的確是當初沒有考慮到的部分 XDrz

@lsc36
Copy link
Author

lsc36 commented Feb 17, 2021

其實我開了 issue 之後才看到 #21 等討論,如果已經有計畫大改的話那先放著也沒關係。
個人想法是,如果沒有替換多種實作的需求,應該就沒有必要定義 interface。

@SivWatt
Copy link
Contributor

SivWatt commented Apr 10, 2021

好像有點晚才看到這個 XD
針對 Logger 我覺得他其實也是一個 dependency 我認為是要保留 interface 的部分
雖然目前我們的 code 雖然使用 logger 的地方不多 (其實大家的習慣也都不一樣 很正常)

舉個例子 getPopularArticles() 實作裡面就有 cal l到 logger
這樣並沒有不好,但是會發現在go test -v 的情況下 logger 的 output 也會秀出來。
但是我們可能預期在testing 的時候 這些log 不應該寫出來,這個時候我們就可以把這個 dependency mock 掉,如此一來testing output 就比較不會有雜訊。

我的想法是甚至我們會需要多一個 logger_mock_test.go 裡面有一個 MockLogger 讓各個 struct 跟他有 dependency 的 testing code 都可以使用

@PichuChen
Copy link
Member

好像有點晚才看到這個 XD
針對 Logger 我覺得他其實也是一個 dependency 我認為是要保留 interface 的部分
雖然目前我們的 code 雖然使用 logger 的地方不多 (其實大家的習慣也都不一樣 很正常)

舉個例子 getPopularArticles() 實作裡面就有 cal l到 logger
這樣並沒有不好,但是會發現在go test -v 的情況下 logger 的 output 也會秀出來。
但是我們可能預期在testing 的時候 這些log 不應該寫出來,這個時候我們就可以把這個 dependency mock 掉,如此一來testing output 就比較不會有雜訊。

我的想法是甚至我們會需要多一個 logger_mock_test.go 裡面有一個 MockLogger 讓各個 struct 跟他有 dependency 的 testing code 都可以使用

有辦法寫個簡單的範例然後附上 PR 嗎?

@SivWatt
Copy link
Contributor

SivWatt commented Apr 24, 2021

有辦法寫個簡單的範例然後附上 PR 嗎?

Please take a look at #193

@ichiaohsu
Copy link
Contributor

ichiaohsu commented May 15, 2021

如果有這樣的需求,保留原有的 Logger interface,但換為較簡單的 Printf(format string, v ...interface{}) 會不會比較好?目前有使用到的也只有這個函式,要 mock dummy/inject logrus 也比較簡便

@SivWatt
Copy link
Contributor

SivWatt commented May 15, 2021

關於 Logger 的做法真的有很多可以討論,Log Level跟Format估計又可以討論很久。
我們暫時應該是不會去用logrus,按照盡量不使用外部package的原則。
現在可能比較會聚焦在Milestone1的feature上面,所以這邊應該是先不會有動作。

@ichiaohsu
Copy link
Contributor

我理解這邊的原則是盡量不用外部 package。就目前而言 Logger 沒有被使用,而且身為一個過大的 interface 會阻擋未來使用如 logrus 這樣的套件的可能。如果像上面那個 pull request 提到的留著可以做 mock,那也可以縮小其定義範圍,這是我原本提出的用意。

這個 feature 感覺不急,我也是在掃 issue list 看有什麼是我可以幫的上忙的時候看到這個留言串。感謝討論 ~

@PichuChen
Copy link
Member

這個 ISSUE 目前還有在更新嗎? 不然兩週後要把他先關掉了喔?

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

No branches or pull requests

5 participants