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

ゾーン指定ロジックの修正 #1171

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

lvctr
Copy link
Member

@lvctr lvctr commented Jul 30, 2024

@lvctr lvctr requested a review from yamamoto-febc July 30, 2024 23:18
if (c.Zone == "" || c.Zone == defaults.Zone) && pcv.Zone != "" {
c.Zone = pcv.Zone

if c.Zone == "" || c.Zone == defaults.Zone {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2点気になりました。

判定ロジックの正当性について

この箇所では以下の条件全てをANDで満たした場合にプロファイルの値が使われています。

  • c.Zoneが空 or デフォルト値
  • プロファイルでゾーンが指定されている
  • プロファイルがデフォルト値以外 or c.Zoneが空

3つ目の条件がよくわからないのですが、例えばデフォルトのプロファイル ~/.usacloud/default/config.jsonでゾーン指定されていた場合は無視されてしまいませんか?

可読性について

複数の条件分岐が入り混じっており意図が伝わりにくいためリファクタした方が良いと思います。
例えば上記の3つの条件それぞれを別のfuncに切り出して && で繋ぐ方法などもあります。

// 3つ目の条件はいい名前が思い浮かばなかったのでそもそも条件を見直した方がいいかも
if !zoneHasDefaultValue(c.Zone) && profileHasZone(pcv) && 3つ目の条件のfunc名 {
}

@lvctr lvctr closed this Oct 28, 2024
@lvctr lvctr reopened this Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants