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

added method to find rows that match the condition #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

frepe2013
Copy link
Contributor

I have created a pull request, including the #3 and #4.
I have added method findEmptyRow, findByCellValue and findAllByCellValue.

def emptyRow = sheetForFindRowTest.findEmptyRow('A2');
assert emptyRow != null
assert emptyRow.rowNum == 10 // rowNum start 0
assert emptyRow.getCell(0)?.value == null
Copy link
Owner

Choose a reason for hiding this comment

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

もしかして、ここで渡しているA2というラベルの使われ方として、

  • "2"行目以降の
  • "A"列が空の行を探す

という2つの意味を持たせていますか?

@nobeans
Copy link
Owner

nobeans commented Aug 14, 2014

上でコメントしたあたりの仕様などを整理してみますので、しばらくお待ちください。

@nobeans
Copy link
Owner

nobeans commented Aug 14, 2014

結論から言うと、やり方をどこか(README, Wiki)で紹介するか、参考用の学習テストをつけるだけでよく、新規メソッドは不要だと思います。

ざっくり箇条書きで書くと

  • 今回追加しようとしているメソッドは、findRowBy*findAllRowsBy*に分けられる(名前はこちらで改名)
  • 今回、*の部分として、findRowでは2種類、findAllRowsでは1種類しかないが、ユースケースが増えればもっと欲しくなってしまうかもしれない
  • こういう場合のGroovyライクな解決策としては、条件評価用のクロージャを渡せるAPIを用意して、好きに条件を指定してもらう
  • ではそういうのを追加しようかと思ったら、SheetがIterableを実装しているため、すでにfind/findAllが使える(実際、既存実装の中でも利用している)
  • そのままそれを使えば良いのでは。それとも追加メソッドで更に便利な何かをしている?
  • findEmptyRowで新規最終行を追加している点ぐらい。しかし、コメントで書いたように、メソッド名から期待する振る舞いとしては一般的とは言えず、あるユースケースでの特殊解に過ぎない(と思う)
  • また、元の追加メソッドの引数のラベルについて、「行ラベル」と「列ラベル」で目的が違っていてユーザとして仕様が分かりづらい。クロージャに行を渡すから好きに欲しいところでtrueを返して、というのはGroovyとしての基本APIなのでGroovyプログラマにとっては特別な知識が不要でわかりやすい。
  • など

「何行目以降」を指定する部分が若干ごちゃっと見えますが、それ以外には特に問題なく使いやすいAPIかと思います。「何行目以降」の部分のために特別なAPIを用意することもできますが、今のところそこまでの価値は見いだせません。

以下のテストコードはマージ作業ブランチからの抜粋ですが、すべてグリーンです。

    void testFind() {
        def resultRow = sheet.find { row ->
            (row.label.toInteger() >= 4) && (row.F_.value == "Ken")
        }
        assert resultRow.label == "7"
        assert resultRow.F_.value == "Ken"
    }

    void testFind_prefixMatch() {
        def resultRow = sheet.find { row ->
            (row.label.toInteger() >= 4) && row.C_.value?.startsWith("Add")
        }
        assert resultRow.label == "4"
        assert resultRow.C_.value == "Add Feature"
    }

    void testFind_empty() {
        def resultRow = sheet.find { row ->
            (row.label.toInteger() >= 2) && !row.A_.value
        }
        assert resultRow.label == "11"
    }

    void testFindAll() {
        def resultRows = sheet.findAll { row ->
            (row.label.toInteger() >= 4) && row.C_.value?.contains("Spec")
        }
        assert resultRows*.label == ["5", "7"]
        assert resultRows.every { it.C_.value == "Spec Change" }
    }

    void testNewLastRow() {
        // Do this if you want new last row (e.g. when your row finder method call would return null.)
        def resultRow = sheet.createRow(sheet.lastRowNum + 1)
        assert resultRow.label == "13"
    }

意図もハッキリしていて、暗黙的な規約もなくわかりやすいコードかと思いますがどうでしょう。

@frepe2013
Copy link
Contributor Author

なるほど。ここまでシンプルでわかりやすく書けるんですね。これであれば、メソッド追加は不要で良いと思います。

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