This repository has been archived by the owner on Mar 21, 2024. It is now read-only.
refactor account.Create #924
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
freewind
commented
May 9, 2018
freewind
commented
- extract to some smaller methods with meaningful name
- rename some variable names to make it better understanding
Paladz
reviewed
May 14, 2018
| if existed := m.db.Get(aliasKey(normalizedAlias)); existed != nil { | ||
| aliasKey := aliasKey(normalizedAlias) | ||
|
|
||
| if existedAccountId := m.db.Get(aliasKey); existedAccountId != nil { |
There was a problem hiding this comment.
这里的话感觉叫existed或者accountId都可以吧？另外貌似ID要全大写，不然golint会报错
There was a problem hiding this comment.
叫existed也可以，只是对于我来说（初次阅读该代码），existedAccountId会让我更清楚从这里期待拿到的是什么，这两者我都可以接受，只要有existed这个词就好。ID大写否则报错这个我不知道，如果是这样的话就大写。
Paladz
reviewed
May 14, 2018
| if err != nil { | ||
| return nil, errors.Wrap(err) | ||
| } | ||
|
|
||
| account := &Account{Signer: signer, ID: id, Alias: normalizedAlias} | ||
| rawAccount, err := json.Marshal(account) | ||
| err = m.saveToDb(account, aliasKey) |
There was a problem hiding this comment.
newAccount， saveToDb， 和下面的findDuplicated 这3个function写了的想法是为了代码复用，这个是很好的可以理解。但是这3个function都只有在一处地方使用，并没有多处调用，所以这里有一点疑惑
There was a problem hiding this comment.
这里我可以解释一下。我们把代码抽成函数出来，实际上至少基于两个方面的考虑：
-
复用：这个好理解，不抽成函数，别处无法调用
-
抽象：给一段代码一个更高层面的意义，暂时隐藏其实现细节。作用就是当我们在读一个函数的时候，通过里面的变量、方法和函数名，就能够大略知道每个步骤的意义，每个步骤的抽象层次应该是差不多的。过于细节的就应该尽量想法抽象出去（抽方法并给出合适的命名）
对于提高代码的可读性来说，第2个方面是我们更看重的。这里抽出来的这几个方法，也就从2考虑的
|
freewinde大哥最近养身体中，等他康复后继续交流 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.