83. Remove Duplicates from Sorted List

[Zun-U]

This : 83. Remove Duplicates from Sorted List

Next : 82. Remove Duplicates from Sorted List II

[nodchip]

自分は Go 言語はあまり書かないのですが、 Go 言語では変数を複数連続で定義する際、変数名の位置のインデントを合わせることが多いのでしょうか。
このような整え方をすると、変数を追加したときに、周辺のインデントも整える必要がある場合が出てきます。その場合、 diff を取ったときに本来不要な部分まで diff があると表示されます。これはレビューワーにとってノイズとなってしまい、レビューの妨げになる場合があります。
チームの平均的な書き方に合わせることをおすすめします。

[Zun-U]

レビューいただき、ありがとうございます。

Go 言語では変数を複数連続で定義する際、変数名の位置のインデントを合わせることが多いのでしょうか。

こちらは、個人的に見やすいと思い位置を揃えていましたが、
レビュー時に不要なノイズを発生させてしまうことへの考慮が漏れていました。

ご指摘を受け、Go言語に標準で用意されているフォーマッターがあることを知りましたので、今後はそちらを使用し、フォーマットについて統一性を保つようにします。

[nodchip]

インデントにタブとスペースが混在しているように思います。統一されることをおすすめします。

[Zun-U]

レビューいただき、ありがとうございます。
Effective Goではインデントにはタブを使用するとありましたので、
こちらに沿うよう修正いたします。

[naoto-iwase]

この関数の正しさは「同じ値が連続して現れる」前提に依存しています（問題で保証される昇順ソートはそのために十分ですが、必要ではないです）。

ただし変数名でその前提を強調する必要はないと思うので、sortedよりnodeなどの方が読み手に余計な期待を持たせません。sorted := headとあると、以降の実装が.Valの昇順な順序特有のロジックを含むように見える気がします。

つまり問題側でソート済みなのは、同値を連続区間として扱いやすくするためで、ここではその連続区間を1回の走査で圧縮しているだけなので、変数名はnodeなどが無難と考えました。

[Zun-U]

レビューいただき、ありがとうございます。
一つの関数に余計な関心事が紛れ込んでる、ということですよね。

関数自体を変えられるのであれば、メソッドにする、あるいは引数の型にソート済みであることの情報を含ませる、などでなるべく関数の外で制限をかける（型で制約をかける）方法を考えていました。

type Sorted *ListNode

// Sorted型に付随する関数
func (s Sorted) DeleteDuplicates(head *ListNode) *ListNode {
...
}

// あるいは、引数の型で制約する
func deleteDuplicates(head Sorted) *ListNode {
...
}

この関数はソート済みのリストが与えられないと上手く機能しませんよ、ということを伝えたかった（制約を課したかった）のですが、
ちょっと考えすぎたのかもしれません。

[naoto-iwase]

ありがとうございます。
えっと、もしかしたら自分の意図がまっすぐ伝わっていないかもしれないので念の為補足させてください。十分理解されている場合はスルーしていただければと思います...

一つの関数に余計な関心事が紛れ込んでる、ということですよね。

というより、deleteDuplicatesで入力されるheadの連結リストは必ずしもソート済みでなくてもいいと自分は思っています。

この関数はソート済みのリストが与えられないと上手く機能しませんよ、ということを伝えたかった（制約を課したかった）のですが、

そうでしょうか？必ずしもソート済みでなくても上手く機能したと思える入力は自分的にはあります。2つ例を挙げます。

1. 例えば入力される連結リストの値が 1 -> 1 -> 3 -> 3 -> 2 だった場合、この関数はどう動くべきだと考えますか。Valが1つ前より小さくなるタイミングで元の入力が昇順ではないことがわかるので、これを異常とらえエラーを送出しますか。例えば自分は1 -> 3 -> 2となるようにduplicatesを削除して返せば十分だと思っています。このような入力は問題では想定されていないものの、自分の気持ちとしては想定内ですし、エラーを出す必要もないと思っています。（この関数が上手く機能する範疇だと思っています。）

2. さらに、入力が1 -> 3 -> 1 -> 3 -> 3 -> 2 だった場合はどうしましょうか。例えば、1（または3）が不連続に出現することは異常でしょうか？この関数はどこまでの責任を負うべきでしょうか。例えば自分はこの入力に対してもエラーは出さず、1 -> 3 -> 1 -> 3 -> 2と返せば十分だと思います。ただこれだと、上手く機能していると言えるかは微妙かもしれませんね。あるいは1 -> 3 -> 2と返すのも1つの設計とは思います。

そしてStep 2の実装も実際このように動くと思いますが、その上でsortedと命名するのは個人的には変数に対する制約として強すぎるかなと感じた次第です。特に、1つ目は上手く機能すると言える範疇ではないでしょうか。

[Zun-U]

ご返信ありがとうございます！
まったく理解が足りてませんでした。。。
仰る通り、ソートされている必要はなく、重複した値が連続していればこの関数の役割は果たせます。

また、この制約の背景としては、複数人で開発するときを想定して、制約を知らない、あるいはうっかり忘れた人がこの関数を使ったらどうなるか、を考えていました。
それなら関数が動く保証があれば、他の人も安心して使えるかなと思った次第です。
これをコードで表現したいところですが、関数名、引数名が変えられない制約があるので、変数名で表現しましたが、そもそも制約の方向が間違ってましたね...
関数の外でも制約をかけることもできたので、もし制約を課すのなら変数名で表現するのはベストと言い難かったかもしれません。

ただ、個人的には、リストがどうであれ、重複はすべて消去されてほしい（ある時は消去されて、ある時は消去されないみたいに出力がブレてほしくない）ですが、その代わり柔軟性は損なわれますね。
そこはご指摘の通り設計、文脈によると思います。

大変貴重なご意見、ありがとうございます🙇‍♂️
とても勉強になりました！

[naoto-iwase]

elseを使うことで対比的にかけるのはcontinueにない良さと思います。

ケースバイケースですが、本問についてはどっちでもいいかなと感じます。

if sorted.Val == sorted.Next.Valが、例外的であり、先に処理しておく、と考えるならcontinueであり、==の場合と!=の場合の、場合分けをしているという気持ちならelseですね。

[naoto-iwase]

あとは、以下のようにも書けます。

func deleteDuplicates(head *ListNode) *ListNode {
    sorted := head

    for sorted != nil {
        for sorted.Next != nil && sorted.Val == sorted.Next.Val {
            sorted.Next = sorted.Next.Next
        }
        sorted = sorted.Next
    }

    return head
}

[Zun-U]

例を挙げていただきありがとうございます。
else、continueがない書き方もできるんですね。
目からうろこでした！

[naoto-iwase]

入力を破壊する（in-place）か否か、という観点もあります。

Step 2のコードはin-placeになっています。入力が返ってくる関数で引数にも変更が起きると使用者はびっくりするかもしれません。（必ず悪、という意味ではないです）

[t9a-dev]

メソッド名のdeleteDuplicatesという文脈からsortedという単語が出てきたときに混乱してしまいました。
変数の使われ方としてはある時点で処理しているノードだと思います。
自分はnode,target_nodeなどにするかなと思いました。

[Zun-U]

レビューいただき、ありがとうございます。
ご指摘の通り、この関数での処理は与えられたnodeを処理しているだけなので、
制約を中に持ち込むべきではなかったかもしれません。