it-swarm-ja.com

レビューアはこのコードレビューで何を参照しましたか?

求人応募のコードを送信し、次のレビューを受け取りました。

  • プロジェクト構造について:物理的な分離はありません。論理的な分離は存在しますが、ベストプラクティスには達していません。物事は「解決に必要な順序によって」分離されているようです。

    コーディング規約について:空白は一貫性がなく、規約の範囲外です。不変性を無視し、「自己」への明示的な参照などを行います。

    readmeのドキュメント作成方法について:Readmeは、使用法、前提条件、またはソリューションの設定方法の説明に関するベストプラクティスに準拠していません。

    保守性、拡張性、スケーラビリティ、またはパフォーマンスに関してアーキテクチャ:アーキテクチャの兆候はありません。アプリは、多数のヘルパークラスからサービスを消費する大規模なビューコントローラーとして説明できます。

    DRYおよびSOLID principlesに関して):いくつかの場所とに重複したコードがありますSOLID原則、ViewControllerはいくつかの責任を負います。

    神の観察:

    依存関係のパッケージマネージャーの使用について:Cocoapodsが使用されました。

    エラー処理とロギング:エラー処理とロギングにある程度の懸念がありました。

レビューアが提示した各問題の詳細な説明を誰かに教えてもらえますか?

誰かがチャレンジへの私の応答に興味がある場合: Github Link

長い文章で申し訳ありませんが、私の経験を共有することで他の人を助けていると思います。

1
Bruno

これらのそれぞれに一度に取り組む:

プロジェクト構造について

これは最良の場合は主観的ですが、プロジェクトは通常、コードを一般的な責任範囲またはそれらが属する「レイヤー」に基づいてサブフォルダーまたはサブプロジェクトに論理的に分割することによって構造化されます。たとえば、モデル、ビュー、コントローラー、コアアプリケーションロジック、共有/共通コンポーネントなどです。

コーディング規約について:空白は一貫性がなく、規約の範囲外です。不変性を無視し、「自己」への明示的な参照などを行います。

コードを書くときは、コーディング標準に厳密に従うことをお勧めします。例: https://github.com/raywenderlich/Swift-style-guide

コーディング標準に関して、最も重要なことの1つは、一貫性です。一貫性のないコードは扱いやすいものではありません。それはただずさんでプロフェッショナルではないように見えます。

不変性については、ここで説明しています: https://en.wikipedia.org/wiki/Immutable_object

readmeドキュメントの実践について

レビュー担当者がReadmeに何を求めているかは私にはわかりませんが、あなたのReadmeは事実上空のようです。おそらく、レビュー担当者は、ユーザーフレンドリーな平易な英語で書かれたユーザーレベルのドキュメントを期待していました(たとえば、アプリを初めて使用する方法、アプリの内容、機能、動作など)

保守性、拡張性、スケーラビリティ、またはパフォーマンスに関してアーキテクチャ

多くの点で、これは「SOLID」と「DRY」の原則に関するポイントと交差しています。しかし、ソリューションが異なる「レイヤー」の論理的な分離に欠けていることも示しています。言い換えれば、レビューアはあなたが "大きな泥だんご" を作成したと思ったのです。

アプリケーションとシステムは、それぞれが互いにきれいに分離された複数のレイヤーで構成されるのが一般的です。例えば:

  • データ層(つまり、永続データを処理するデータアクセス層)
  • ビジネスロジックレイヤー(コアドメインロジック-たとえば、実際にすべての要求を処理し、すべてのデータを処理し、結果を計算するロジックなど)。
  • アプリケーション層(アプリケーション固有のロジック-たとえば、ビジネスロジック層の関数を呼び出すためのユーザー入力に基づいてリクエストを作成する)。
  • プレゼンテーション層(ビューロジック-MVCパターンなど、ユーザーの双方向性、レイアウト、データの表示などの処理)。

レビューアはいくつかの特定の言葉に言及しました:

  • 保守性-コードを保守可能にするには、誰かが読み、理解し、フォローしやすい必要があります。クラスは疎結合である必要があります。関数は循環的複雑度が低い必要があります。簡単にテスト可能であり、可能な限り多くのユニットテストカバレッジを持つ必要があります。繰り返しますが、これはSOLID/DRYと関連しています。

  • 拡張性-新しい機能を追加するために、既存のクラスや関数の多くを変更する必要がないようにコードを設計する必要があることを意味します。 SOLID:オープン/クローズド原則

  • スケーラビリティ/パフォーマンス-おそらくレビューアは、頻繁に使用するとコードが適切にスケールアップしないと考えました。

DRYおよびSOLID原則

これらは一般的なソフトウェア設計の原則であり、時間をかけて学習し、明確に理解していることを確認する価値があります。

SOLID -- https://en.wikipedia.org/wiki/SOLID_(object-ientified_design)

乾燥--- https://en.wikipedia.org/wiki/Don%27t_repeat_yourself

あなたのコードをレビューした人は誰でも、あなたが提出したソリューションのいくつかの違反を明確に特定しました。これらの原則に違反するコードの典型的な「特徴」には、次のものが含まれます(ただしこれらに限定されません)。

  • あまりにも多くのことをしているクラス(つまり、あまりにも多くの責任を持っている)
  • 長い関数(やや主観的ですが、多くの人は30行を超えるものは「長すぎる」と考えています)
  • 関数が多すぎるクラス
  • いくつかの場所で繰り返され、単一のクラスおよび/または関数に合理化できるロジック

私はあなたのコードをざっと見ました、そしてあなたのViewControllerは間違いなくこれらの原則に違反しています、特にあなたのviewDidLoad()メソッドはあまりにも多くのことをしているようです。

全体あなたのコードを見たときに私が得た(そしてレビューアが持っていたと思う)印象は、あなたが多くの経験を持っているという証拠が実際にはないということです複雑さに対処したり、開発者のチームで作成されたコードを操作したりします。

おそらく、チャレンジに基づくあなたに関する彼らの主な懸念は、このチャレンジの何倍もの規模のプロジェクトをチームで行うことができず、それをレイヤーとモジュールに分解できないことです。または、他の開発者が使用できる方法でコードを構造化することもできます。

しかし、学習経験として、あなたはいくつかの良い、価値のあるフィードバックを得たようです。次のステップは、得られたソリューションを採用し、異なるレイヤーを明確に分離して適切に構造化することです。プロジェクトを使用して、SOLIDの原則を適用する方法を学びます。

22
Ben Cottrell

「DRY」は「自分を繰り返さない」という意味です。彼らはあなたに「いくつかの場所で重複したコードがあります...」と言いました。これは悪いです。一度変更する必要がある場合は、潜在的に数十(または数百)の場所で変更する必要があります。 (何年も前に、DRYを本当に理解していない人が書いたコードのオーバーホールをしなければなりませんでした。面白くありませんでした...)

「SOLID」とは、たくさんのものを指します。 ここ を参照してください。彼らは、「ViewControllerはいくつかの責任を負っている」と述べました。これはSOLIDのBozono-noです。

3
John R. Strohm