0

私は OOP にまったく慣れていないので、私が作成したこのクラスの設計が本当に不十分ではないかと心配しています。OOP のいくつかの原則に従わないようです。

  1. 独自のデータは含まれていませんが、値は yaml ファイルに依存しています。
  2. そのメソッドは特定の順序で呼び出す必要があります
  3. 多くのインスタンス変数とメソッドがあります

ただし、機能します。堅牢ですが、ページ要素を追加するたびにソース コードを変更して新しい getter メソッドを追加する必要があります

これは、自動化されたテスト スイートで使用される html ドキュメントのモデルです。一部のメソッドをサブクラスに入れることができると考え続けていますが、そうするとクラスが多すぎるのではないかと心配しています。

どう思いますか？

class BrandFlightsPage < FlightSearchPage

  attr_reader :route, :date, :itinerary_type, :no_of_pax,
              :no_results_error_container, :submit_button_element

  def initialize(browser, page, brand)
    super(browser, page)

    #Get reference to config file
    config_file = File.join(File.dirname(__FILE__), '..', 'config', 'site_config.yml')

    #Store hash of config values in local variable
    config = YAML.load_file config_file

    @brand = brand        #brand is specified by the customer in the features file

    #Define instance variables from the hash keys
    config.each do |k,v|
      instance_variable_set("@#{k}",v)
    end

  end

  def visit
    @browser.goto(@start_url)
  end

  def set_origin(origin)
    self.text_field(@route[:attribute] => @route[:origin]).set origin
  end

  def set_destination(destination)
    self.text_field(@route[:attribute] => @route[:destination]).set destination
  end

  def set_departure_date(outbound)
    self.text_field(@route[:attribute]  => @date[:outgoing_date]).set outbound
  end

  def set_journey_type(type)
    if type == "return"
      self.radio(@route[:attribute]  => @itinerary_type[:single]).set
    else
      self.radio(@route[:attribute]  => @itinerary_type[:return]).set
    end
  end

  def set_return_date(inbound)
    self.text_field(@route[:attribute]  => @date[:incoming_date]).set inbound
  end

  def set_number_of_adults(adults)
     self.select_list(@route[:attribute]  => @no_of_pax[:adults]).select adults
  end

  def set_no_of_children(children)
    self.select_list(@route[:attribute]  => @no_of_pax[:children]).select children
  end

  def set_no_of_seniors(seniors)
    self.select_list(@route[:attribute]  => @no_of_adults[:seniors]).select seniors
  end

  def no_flights_found_message
    @browser.div(@no_results_error_container[:attribute] => @no_results_error_container[:error_element]).text
    raise UserErrorNotDisplayed, "Expected user error message not displayed" unless divFlightResultErrTitle.exists?
  end

  def submit_search
    self.link(@submit_button_element[:attribute] => @submit_button_element[:button_element]).click
  end
end
4

1 に答える 1

0

このクラスが Facade として設計されている場合、それは (あまりにも) 悪い設計ではありません。関連のないさまざまな動作ホルダーに依存する関連操作を実行するための一貫した統一された方法を提供します。

このクラスは本質的にさまざまな実装の詳細すべてを結合しているため、関心の分離が不十分であるように思われます。

最後に、メソッドを特定の順序で呼び出す必要があるという事実は、ステート マシンをモデル化しようとしているという事実を示唆している可能性があります。この場合、おそらく複数のクラス (「ステート」ごとに 1 つ) に分割する必要があります。「メソッドが多すぎる」または「クラスが多すぎる」というポイントはないと思います。実際には、各クラスが提供する機能が一貫して意味をなす必要があります。どこに線を引くかは、あなたとあなたの特定の実装のドメイン要件次第です。

于 2012-08-24T10:58:29.247 に答える