covid19 icon indicating copy to clipboard operation
covid19 copied to clipboard

非機能要件(可読性・拡張性・設計) 問題点・改善点の整理

Open nard-tech opened this issue 6 years ago • 37 comments

改善詳細 / Details of Improvement

  • 感染者の増加が続く現状を見ると,このサイトが必要とされる状況はしばらく続くと思われる
  • 一方,一時期に比べると issue, PR の数,merge の頻度は落ち着いており,機能を次々に追加していく,バグを直していくというフェーズから抜けつつある (cf. https://github.com/tokyo-metropolitan-gov/covid19/graphs/code-frequency)
  • したがって,今までの大勢による活発な開発によりやむを得ず生じてしまった「技術的負債」の返済をしつつ,より堅牢で拡張性の高い設計になるよう,非機能要件の問題・改善すべき点を整理し,適切な順序・タイミングでリファクタリングを進めていく必要があると考える

という訳で,この issue に非機能要件の問題・改善すべき点を,必要性やデメリットも含め,まとめていきつつ議論を進めたいと思います.

nard-tech avatar Apr 03 '20 04:04 nard-tech

component のディレクトリ構成(案)

  • page/index.vue のみで使われている components/ 以下のファイルを,components/index 以下に移動する(components/flow と同様)
    • components/cards/ 以下のファイルも,components/index/cards/ に移動する
    • components/cards/import している component(主にグラフ描画用の XxxxChart)を components/index/cards/charts/ に移動する
      • (2020-08-22 追記XxxxCard に対応する component は XxxxChart という命名に統一する
    • DataTableComfirmedCasesAttributesTable に,TimeStackedBarChartTestedNumberBarChart に名前を変更すべきでは?
  • page/flow.vue のみで使われている components/ 以下のファイルを,components/flow 以下に移動する(PrinterButton のみ?)
  • (2020-08-22 追記ConfirmedCasesIncreaseRatioByWeekChart.vue は使われていないので削除すべき

nard-tech avatar Apr 03 '20 05:04 nard-tech

component の使い方問題

例:components/cards/TestedCasesDetailsCard.vue 8行目〜

    <data-view
      :title="$t('検査実施状況')"
      :title-id="'details-of-tested-cases'"
      :date="Data.inspection_status_summary.date"
    >
      <template v-slot:button>
        <ul :class="$style.notes">
          <li>
            {{ $t('(注)医療機関が保険適用で行った検査は含まれていない') }}
          </li>
          <li>
            {{
              $t(
                '(注)検査実施人数には、チャーター機帰国者、クルーズ船乗客等は含まれていない'
              )
            }}
          </li>
          <li>
            {{
              $t(
                '(注)速報値として公開するものであり、後日確定データとして修正される場合あり'
              )
            }}
          </li>
        </ul>
      </template>

DataView.vue の実装を考えると,本来は <template v-slot:button> ではなく <template v-slot: description> を使うべき.

このような箇所が ConfirmedCasesDetailsCard.vue, TestedNumberBarChart.vue にも見られる.

追記 (2020-08-15): 解消または該当ファイル削除済み

nard-tech avatar Apr 03 '20 05:04 nard-tech

component の中身が全体的に荒れている感あり

その他,page/index.vue 上の component, chart が全体的に荒れている印象があるので,dry で統一感のある構成にしたい.

nard-tech avatar Apr 03 '20 05:04 nard-tech

JSONファイルとVueの間に中間層を設ける

data/data.json などからデータを取得する際に,中間層を設けることで型推論を可能とし、コードの保守性を上げる.

cf. JSONファイルとVueの間に中間層を設ける (#1100)

実装すると宣言した方がいらっしゃるが,音沙汰なしの状況.

nard-tech avatar Apr 03 '20 05:04 nard-tech

tool/convert.php について

Nuxt.js のプロジェクトなので,JavaScript(または TypeScript)に書き換えたい.

型をしっかり付けるなら「JSONファイルとVueの間に中間層を設ける」とも関連してくるかも?

追記 (2020-08-15): tool/convert.php は削除されたため対応不要

nard-tech avatar Apr 03 '20 05:04 nard-tech

data.json について

分割すべきでは?

  • 現在 7727 行と肥大化している.将来的にデータが溜まっていくとさらに肥大化が進み,管理しにくくなると予想される.
  • 他県版に東京版のコードを merge しようとすると conflict が多発する.data.json を分割しなくても conflict することには変わりないが,conflict 解消時のミスの危険性が増すと思われる.
  • 各グラフのデータの更新タイミングが異なるので,data.json を分割した方がファイルごとの責務が明確になり,Git で管理するときに差分が確認しやすい.
  • ただ,tool/convert.php の書き換えも含め,工数が大きく,データの信頼性を担保したまま分割するのは大変.

property の日本語を英語にすべきでは?

  • 個人的な意見ではあるが,property に日本語を使用しているために日本以外からの PR,日本以外への fork が難しい状況になっており,勿体ないと感じる.

nard-tech avatar Apr 03 '20 05:04 nard-tech

template に data.json をそのまま放り込んでいる箇所がある

template に渡す値は <script></script> 内で絞り込みたい.

: components/cards/ConfirmedCasesAttributesCard.vue

<template>
  <v-col cols="12" md="6" class="DataCard">
    <data-table
      :title="$t('陽性患者の属性')"
      :title-id="'attributes-of-confirmed-cases'"
      :chart-data="patientsTable"
      :chart-option="{}"
      :date="Data.patients.date"
      :info="sumInfoOfPatients"
      :url="'https://catalog.data.metro.tokyo.lg.jp/dataset/t000010d0000000068'"
      :source="$t('オープンデータを入手')"
    />
  </v-col>
</template>

<script>
import Data from '@/data/data.json'
// (略)

export default {
  // (略)
  data() {
    // (略)

    const data = {
      Data,
      patientsTable,
      sumInfoOfPatients
    }
    return data
  }
}
</script>

追記 (2020-08-14): #5250 で対応

nard-tech avatar Apr 03 '20 05:04 nard-tech

非機能要件ということで、各vueファイルの

khsacc avatar Apr 03 '20 07:04 khsacc

tool/convert.php について

すみません、これについてはすでに保守されておらず・・・。 アクセス権を含む(一応Secretで管理しています)ので万一漏れてしまう事を考え別途管理に移行しています。 現在のtoolについては参考までにという形で残す(READMEにその旨記載)か削除で良いかと思います。 本来はオープンソースで管理したいところです・・・。

mikkame avatar Apr 03 '20 07:04 mikkame

@khsacc

非機能要件ということで、各vueファイルの

<script> を機械的に <script lang="ts"> に置き換え,型情報が足りず警告が出るような箇所には型を追加する,というのであれば手早く対応できると思います.

nard-tech avatar Apr 03 '20 07:04 nard-tech

data.json について 分割すべきでは?

この件についてはその通りだと思います。 カードごとのデータにした方が良いかと思います。(metroやnewsのように) 単純に1階層掘って分離してしまえばあまり影響は大きくないのかなというところと data.jsonと平行して管理して随時変更してもいいのかなとも思います。 がしかし、他のプロジェクト(本プロジェクトは全く関係ない)がdata.jsonを引っ張って使っている可能性があるので今の時点で急にdata.jsonのサポートを切ってもいいのかという疑問も残ります。 外部サイトのことを気にしすぎることもないとは思いますが・・・

mikkame avatar Apr 03 '20 07:04 mikkame

@mikkame

他のプロジェクト(本プロジェクトは全く関係ない)がdata.jsonを引っ張って使っている可能性 それについては Slack でアナウンスして,数日後にサポートを切るということにすれば十分だと思います.それ以上は面倒見切れないですよね…

tool/convert.php を見たところ,Excel ファイルをパースして JSON を作っていますが,それは別途管理している現在も変わらないでしょうか? data.json の中にはどこからも使われていない無駄なデータもある気がしますので,そういうものを消していく意味でも,あるいは他のデータを追加するときに誰でも手早く変換スクリプトが書けるようにするためにも,セキュリティが絡むところと絡まないところは適切に分離して,安全な部分については GitHub で管理して頂けるとよろしいかと思います.

nard-tech avatar Apr 03 '20 07:04 nard-tech

@nard-tech はい、その流れ自体は変わりません。 またその上で所定の場所からエクセルをダウンロードする機能が追加されております。(アクセス権部分) その元のエクセルを公開することが難しいという事ですので分離するにもなかなか厳しい状態です。 ご理解いただけると助かります。

mikkame avatar Apr 03 '20 07:04 mikkame

PHP プログラムは、JS or TS に書き換えましょう。何でしたら、私がやります(笑) Excel オブジェクトとの I/F がありますので、TS がいいんでしょうね(適当

あと、セキュリティ観点だと以下のような問題があります。 ・言語ごとにメンテナンスのノウハウが異なる場合、リソースがより多く必要となることが想定される。そしてそれはコミュニケーションチャネルの予期せぬ増加や追加コストの発生といった問題を招く ・言語ごとの実行エンジンのバージョン・ライフサイクルが異なる場合、アップグレードの度にサービスを停止せねばならないとしたら、エンドユーザがサービス停止の不利益を被るタイミングが単一プラットフォームの場合よりも増える ・1個のシステムに複数の言語が混在している場合、一貫性(integrity)を欠き、それが脆弱性の源となり得る

あくまでガバナンス、コンプライアンス、セキュリティなどを論じる際にありがちな一般的な話になってしまいました。正直、この状態では意見としてまだまだ浅いと思います。 ぜひ、エキスパートの皆さんのご意見を頂戴したく

mcdmaster avatar Apr 04 '20 06:04 mcdmaster

@maccostar 当時はとりあえずデータが作れなければならないということだったので得意言語で実装してしまった経緯があります、ですので得意な方TS/JSに書き換えていただけると助かります。 ただ、上記に記載したとおりの課題がございますのですぐに書き換えるということが難しいです。 何かアイデアが有りましたらよろしくお願いします

mikkame avatar Apr 04 '20 07:04 mikkame

@mikkame

所定の場所からエクセルをダウンロードする機能が追加されております。(アクセス権部分) その元のエクセルを公開することが難しい

一般的な設計論の話になりますが,ファイルを

  • (1) 任意の URL にあるファイルをダウンロードする機能
  • (2) (1) を利用して(公開できない Excel ファイルの URL を指定して)Excel ファイルをダウンロードする機能
  • (3) (2) でダウンロードした Excel ファイルをパースして JSON に展開する機能

に分離して,(1) だけ(Excel ファイルの表の構成が推測されても構わないのであれば (3) も GitHub で公開すれば済むと思います.

現在の非公開のダウンロード,パースのスクリプトも PHP で書かれているのでしょうか?

nard-tech avatar Apr 04 '20 10:04 nard-tech

@halsk こちらの issue,discussion タグを付けてください.よろしくお願いします.

nard-tech avatar Apr 04 '20 10:04 nard-tech

@nard-tech お返事ありがとうございます。

現在の非公開のダウンロード,パースのスクリプトも PHP で書かれているのでしょうか

生憎ですがその通りです。

(1)の部分ですが、SharePointのAPIを経由してファイルをダウンロードする機能が実装されております。(実装上はOAuthなんですがSharePointAPIが特殊で自前で書き直した経緯があります) (3)に関しては構造を公開していいのか確認しています。公開できるようならExcelごと置いてしまえるので一番かと思います。 またはダミーExcelとか、今のExcelから出せる項目だけのCSV?を出力するところまでやって 適切な変換をしてもらえるような形にするのも良いかと思います(アイデアレベルです)。 この件については再度確認中ですのでもうしばらくお待ちください。

mikkame avatar Apr 04 '20 10:04 mikkame

ということで、イキナリ #1471 からメンションしました。論点は、プライバシーの扱いについてです。 本プロジェクトの成果である東京都 COVID-19 情報サイトは、東京という都市の世界の中の立ち位置からすれば、世界中の人たちが関心を持って見ていると考えられます。 世界の中でも欧州からのアクセスがあった場合、GRPR という規定を守ることが求められます。これは、欧州拠点の発信者だけでなく、域外にも適用されるものです。 COVID-19 情報サイトでは、サイト来訪者の統計を取るために Google Analytics を使っています。これが個人を識別可能な情報(IP アドレス等)を集めていることを GDPR に沿う形で明示するとともに、サイト来訪者がしかるべきアクション(クッキー拒否など)を取れるような仕組みが厳密には求められます。 こういった要件とどう付き合っていくかは今後の課題になるだろうということで、ここに記しておきます

mcdmaster avatar Apr 04 '20 23:04 mcdmaster

@mikkame

(3) (2) でダウンロードした Excel ファイルをパースして JSON に展開する機能 の件,Excel ファイルの表の構造は現状 tool/convert.php から推測できるので既にバレバレという気もします…

ちょっと今から一旦流れを整理しますね.

nard-tech avatar Apr 05 '20 12:04 nard-tech

@mcdmaster コメントありがとうございます.PHP -> TS の件についてはその通りだと思います.

GDPR の件も把握しました.ガバナンス,コンプライアンスの観点からは確かに重要なことで,ある意味非機能要件ではあると思うのですが,#1471 のようにこれからも活発に議論がなされると思われるので,この issue で本格的に議論されるよりは,専用の issue を立てて頂いた方がよいと思います. この issue だと他の話題と混ざって議論しづらくなってしまいますし.

nard-tech avatar Apr 05 '20 12:04 nard-tech

@mcdmaster この issue を立てた意図としては,現状の問題点を洗い出すとともに,非機能要件の改善に着手するときに重要となる優先度を見極め,適切な手順を決めたい,というところがあります.

プライバシーの件はそれ単体で議論して必要な実装を進めて頂いても問題ないと思います.

もちろん,連携は取らせて頂きたいので,issue を立てたら「cf. #2790」などと書いておいて頂ければと思います.

nard-tech avatar Apr 05 '20 12:04 nard-tech

@nard-tech りょうかいしました。イシューをフォークします、という機能があるといいなと思いました(笑)

それはさておき、非機能要件のおおよその三大要素はパフォーマンス、セキュリティ、運用というふうに市中企業では言われたりします。もちろん、運用のところがインフラや DR だったりのように他の形でのグルーピングもあると思います。 とはいえ、おっしゃるように、セキュリティやそれに密接に関連するガバナンス、コンプライアンス、あるいはユーザ教育といったあたりにまで話を広げると、プロジェクトとしてはいきおい手に余らせてしまいますよね。 なので、いわゆる GRC プラス IT セキュリティ的な話は、都のポリシや標準、手順、ガイドラインも既にあるはずですので、疎かにならない程度に活動を続けていくこととします。その中から、これがオープンソース的じゃん!みたいなモノが出せるのが究極の nice-to-have ですね

mcdmaster avatar Apr 06 '20 00:04 mcdmaster

とりあえず皆さんが思っている問題点は可視化できたと思いますので,とりあえず

  • component のディレクトリ構成を見直す https://github.com/tokyo-metropolitan-gov/covid19/issues/2790#issuecomment-608232372
  • component の使い方に問題がある点を見直す https://github.com/tokyo-metropolitan-gov/covid19/issues/2790#issuecomment-608233597
  • component の中身が全体的に荒れている感あり https://github.com/tokyo-metropolitan-gov/covid19/issues/2790#issuecomment-608234181

については私が着手したいと思います.

終わり次第,

  • TypeScript に統一 https://github.com/tokyo-metropolitan-gov/covid19/issues/2790#issuecomment-608266466

にも着手します.

  • JSONファイルとVueの間に中間層を設ける https://github.com/tokyo-metropolitan-gov/covid19/issues/2790#issuecomment-608234667

についても,進捗がないようであれば私がやってしまおうと思います.

  • tool/convert.php https://github.com/tokyo-metropolitan-gov/covid19/issues/2790#issuecomment-608235564

については,削除してもよろしいでしょうか (@mikkame)

nard-tech avatar Apr 06 '20 05:04 nard-tech

data.json の分割については,変換スクリプトを書き換えて GitHub で公開する件とも関わってきます. 変換スクリプトを書き換えるには,@mikkame さんにお任せするか,https://github.com/tokyo-metropolitan-gov/covid19/issues/2790#issuecomment-609005844 で記した通り,ファイルごとの責務を分割して可能な限り公開して頂いて誰か手の空いている方にお願いすることになると思います. いずれにせよ,@mikkame さんの手を煩わせることになってしまいますが,今後の作業を @mikkame さんに属人化させず誰でも修正・変更できるようにした方が好ましいと思いますので,宜しくお願い致します.

nard-tech avatar Apr 06 '20 05:04 nard-tech

  • JSONファイルとVueの間に中間層を設ける #2790 (comment) #1100

これ、私がやってみたいですね。大人の事情(?)的なガバナンス、コンプライアンス、法令順守なんていうのはここ数年の専門分野だったりするのですけど、元 Java エンジニアとしてはかなり興味を持っています。 とはいえ、もはやロートル(笑)なのでスピード感にはあまり期待しないでください。それじゃ厳しいよ…ということであれば、もちろん進んで他の方にお譲りします

mcdmaster avatar Apr 06 '20 05:04 mcdmaster

変換スクリプトについてですが、元Excelが原則非公開であるとともに、運営側でも今後データの運用が変わる可能性があります。ですので、変換スクリプトの書き換えについてはペンディングにしてもらえますよう、お願いいたします。

kaizumaki avatar Apr 06 '20 05:04 kaizumaki

@mcdmaster

これ、私がやってみたいですね

ぜひお願いします.ただ,https://github.com/tokyo-metropolitan-gov/covid19/issues/2790#issuecomment-609571635 で私がやると宣言した部分と conflict すると思いますので,少しお待ちいただけますか.

nard-tech avatar Apr 06 '20 11:04 nard-tech

@kaizumaki

変換スクリプトについてですが、元Excelが原則非公開であるとともに、運営側でも今後データの運用が変わる可能性があります。ですので、変換スクリプトの書き換えについてはペンディングにしてもらえますよう、お願いいたします。

承知しました.

nard-tech avatar Apr 06 '20 11:04 nard-tech

@nard-tech JSON ⇔ VUE 中間層の待ちにつき、承知しました。 アップデートがありましたら、お知らせください

mcdmaster avatar Apr 06 '20 11:04 mcdmaster