【Swift】以下のイケていないコードをリファクタリングするならどうしますか？

Question

以下のコードをイケてるまで行かなくてもマシなレベルで書くとしたら良い書き方はありますか？

知りたい点

Swiftのswitch文でもうちょいよい書く方法を知りたい。
Swiftならではのswitchの使い方など。
以下のコードだと return 0　や　return 1 などDRYではないことが気にはなっています。

補足点

説明は便宜上わかりやすく書き直していて実際の実装はenumなどもう少しマシには書いてある状態。
といった感じです。

雑に質問投げて申し訳ないです。

func testMethod(number: Int, tags: [String], products: [String]) -> Int {
    if tags.isEmpty {
        switch number {
        case 0:
            return 1
        case 1...tags.count:
            return 5
        default:
            return 0
        }
    } else {
        switch number {
        case 0:
            return 1
        case 1...tags.count:
            if products.isEmpty { return 0 }
            return 1
        case 11...20:
            return 5
        default:
            return 0
        }
    }
}

ｓ

Comments

[fuzzball]

どこらへんが「イケてない」と思っているのか書いて下さい。

[pamipani]

例えば　`case 0: return 1 ` や `default: return 0` など処理が重複していてDRYではない点。なんか頑張ればもう少し短くかけるかなと思ってやってみたものの思いつかなかったなど。

[fuzzball]

バグも含めてリファクタリングしないといけないのでしょうか？

[fuzzball]

例えば testMethod(number: 1, tags: [], products: []) で落ちますが、そういうのも残したままでリファクタリングしないといけないのでしょうか？

[MasakiHori]

自分でそれをする時は絶対に必要というわけではないですが、他者にリファクタリングを依頼する場合はユニットテストが絶対に必要です。　まずはユニットテストを書いてください。　もしくはユニットテストが作成可能な仕様書を書いてください。

[pamipani]

絶対w

[MasakiHori]

ユニットテストのない書き換えをリファクタリングと呼んではだめですよ　それはただの書き換えです　偉い人に怒られます

Answer 1

ベストアンサーもう出ちゃってて完全に出遅れちゃってる上に、もうみんな色々回答書いてて重箱の隅を突くような真似しかできないけども。switch文ってそれなりの行数取るからswitch文を丸々関数にするのも手ですよね。それだけで割と見通しが良くなる。
あとできるだけtagは使わないようにするとか。tagって増えれば増えるほど管理がめんどくさいですし。

Swiftならではのswitch文と言えばenumとの絡め手かなぁ。上記コードだと前後関係分からないからenumの書き方もなんとも言えないのだけども。ちょいちょい使うのはいくつかある状態をenumで定義しておいてそのenumに対してswitch文を書くとか。
ラストに思いついたのは上記の例だと行数少ないからやらないけど、switch文中の各caseは全部関数にしちゃうとかもよくやります。

Answer 2

あくまでも、提示されたコードを見ただけで書き直すとしたら、early returnを優先的に意識して書きます。
ただ、他の方もおっしゃているように、仕様や要件によっていくらでも書き方は変わります。

func testMethod(number: Int, tags: [String], products: [String]) -> Int {
    if number == 0 {
        return 1
    }

    if tags.isEmpty {
        return 0
    }

    if number <= tags.count {
        if products.isEmpty {
            return 0
        }
        
        return 1
    }

    if number >= 11 && number <= 20 {
        return 5
    }

    return 0
}

もとのコードでは、特に、caseが以下のようになっているのが嫌な感じがしました。
「例えば、tags.countが15の時は、上を通るんだよね？」っていう確認をしたくなるからです。

case 1...tags.count:
case 11...20:

Comments

[pamipani]

なるほど、雑な質問に答えてもらってありがとうございます！
```
if tags.isEmpty {
return 0
}
```
ここ 0返してしまうと説明例の処理と異なりそうな気がするのですが。
tagsがEmptyでもnumberの値によって返す値も変わってくるのです。

[pamipani]

こんな雑な質問に付き合っていただきありがとうございます！early returnを心がけます！

Answer 3

リファクタリングの前にデバッグすることをお勧めします。

【追記】

switch文の中の重複を気にしているようですが、tags.isEmptyであればtags.count == 0なので、

if tags.isEmpty {
    return number == 0 ? 1 : 0;
} else {
    :
}

で済むと思います。
（case 1...tags.count:を通ると落ちるので、ここには絶対来ないものと仮定しています）

Comments

[pamipani]

良さそうなヒントが！なるほど。

[fuzzball]

>>以下のコードだと return 0　や　return 1 などDRYではないことが気にはなっています。

switchから出せばいいです。
mesoshiさんの回答のように。

Answer 4

いろいろなパターンを見たいということならば????のはどうでしょう。
可読性も悪く、自分では使いませんが、、、（※落ちる部分は削除しました）

func testMethod(number: Int, tags: [String], products: [String]) -> Int {
    switch (number, tags.count) {
    case (0, _): return 1
    case (_, 0): return 0
    case (1...tags.count, _): return products.isEmpty ? 0 : 1
    case (11...20, _): return 5
    default: return 0
    }
}

動きを確認したコード

func testMethod(number: Int, tags: [String], products: [String]) -> Int {
    if tags.isEmpty {
        switch number {
        case 0:
            return 1
//        case 1...tags.count:
//            return 5
        default:
            return 0
        }
    } else {
        switch number {
        case 0:
            return 1
        case 1...tags.count:
            if products.isEmpty { return 0 }
            return 1
        case 11...20:
            return 5
        default:
            return 0
        }
    }
}



func testMethod1(number: Int, tags: [String], products: [String]) -> Int {
    switch (number, tags.count) {
    case (0, _): return 1
    case (_, 0): return 0
    case (1...tags.count, _): return products.isEmpty ? 0 : 1
    case (11...20, _): return 5
    default: return 0
    }
}

print(testMethod(number: 0, tags: [], products: []))
print(testMethod1(number: 0, tags: [], products: []))
print("---------")

print(testMethod(number: 1, tags: [], products: []))
print(testMethod1(number: 1, tags: [], products: []))
print("---------")

print(testMethod(number: 1, tags: ["a"], products: []))
print(testMethod1(number: 1, tags: ["a"], products: []))
print("---------")

print(testMethod(number: 1, tags: ["a"], products: ["a"]))
print(testMethod1(number: 1, tags: ["a"], products: ["a"]))
print("---------")

print(testMethod(number: 11, tags: ["a", "b"], products: ["a"]))
print(testMethod1(number: 11, tags: ["a", "b"], products: ["a"]))
print("---------")

print(testMethod(number: 21, tags: ["a", "b"], products: ["a"]))
print(testMethod1(number: 21, tags: ["a", "b"], products: ["a"]))
print("---------")

Answer 5

そのコードがイケてるかイケてないか、って判断は、コード断片だけでなく
全体や要件、開発チームの事情なども合わせて考えないと分からないと考えます。
（請負開発屋の意見です）

それはさておき、いくつか。

tagsが空かどうかというのは、最初に切り分けるべき条件とは思えない。
それが必要なときに改めて切り分ければよいのでは？

numberの値がenumに出来るような有限集合ではなく、あくまで数値範囲であれば、
switchよりif/else ifの方が柔軟性が高い気がする。

Comments

[pamipani]

なるほど 普通に if/else if か。
ちょっと書いてみまっす。

Answer 6

リファクタリングする際に参考にできる考え方はデザインパターンです。
Martin Fowlerさん本
とかDustin Boswellさん , Trevor Foucherさん本
とか結城センセの本
が個人的にオススメです。
switchで多くの場合をケース分けする際にもStateパターンとかがあるので、満足できるのではないでしょうか。

Comments

[pamipani]

リーダブルコードはさすが読んだことある
一番上のリファクタリングは知らなかったので今購入した
ありがとう。

ただ自分が今知りたいのは
Swiftのswitch文でもうちょいよい書く方法を知りたい。
説明は便宜上わかりやすく書き直していて実際はenum(state)などもう少しマシには書いてある状態。
といった感じです。

[t_obara]

書籍にも記載がありますが、まずどのような前提でのコードなのかを提示した上で、そのような課題を持ったコードに対してどのようにリファクタリングすべきか具体的な事例を交えて示唆を与えるものです。
簡易コードを提示していただいたとしても、背景情報がないと、他の方々からもコメントがある通り、あまり満足のいく回答は得られないのではないでしょうか。
表面的な回答で言えば、先に記載した通り、まずStateパターンの適用をトライして見ては？となります。