柳屋

ソースコードのリファクタからよもやままで

rubyをリファクタしてみました

f:id:ultrasevenstar:20170905144339j:plain

今回は打って変わってrubyです

Problem 4 - PukiWiki

こちらの問題をrubyで書いてみたそうです。

元々はPHPとJavascriptをやってたのででrubyはあまり得意ではないので 間違ったところがあればご容赦を

def calc?(num)

 num.to_s == num.to_s.reverse

end

p_array = []

(999).downto(100).each do |i|

  (999).downto(100).each do |j|

    result = i * j

    p_array << result if calc?(result)

  end

end

puts p_array.max

意図を探る

f:id:ultrasevenstar:20170905144641j:plain

出題者の意図を探るってのがあります。 言葉違えば、要望を汲み取るってことですかね。

今回のソースについてもそこがあって、 このソースの実行結果は 906609 になります。 確かに答えとしては正解なんでしょうが、違和感がありました。

問題文に

2桁の数の積で表される回文数のうち, 最大のものは 9009 = 91 × 99 である.

とあって式も提示されてるのであれば答えとしても式を提示するべきではないかなと思う次第です。

多分そこってディレクションの領域でもあるかなと。資料などから要望を読み解く 所謂忖度ですね

なので〇〇✖︎△△=906609の形にした方がよりベターではないかと思う次第です

関数にするかどうか

f:id:ultrasevenstar:20170905144938j:plain

def calc?(num)

 num.to_s == num.to_s.reverse

end

calc?関数が定義されていますが 中身を見ると一行だけです。 それにこの関数が使われているのが一箇所のみ

どうなんでしょうか。 よく言うのが 3回同じことを書いたらリファクタリング ってあるので、それを一つの指針としても良いかも知れません。

今後の拡張性とかは考えるべきですが、問題が問題だけに拡張する可能性は低いかと なのでこの関数はメイン処理に組み込んで良いと思います

不要な処理

今回の処理を見ていると、回文数であれば配列p_arrayに格納し、最後に配列の中身の最大数を取ってきています。

調べてみたところ配列の要素数は2470です。 これ今は3桁やからまだなんとかなってますが、これが4桁5桁になってくと メモリ消費が指数的に増えていくのは明らかです。

それに最初に指摘した計算式まで入れるとなると色々と面倒になっていきます。

今回の処理については最大数を変数に格納すれば良いので、そちらに変えていきましょう。

# 関数としては不要なので削除
- def calc?(num)
- 
-  num.to_s == num.to_s.reverse
- 
- end

- p_array = []
+ max = 0
+ addend1 = 0
+ addend2 = 0

(999).downto(100).each do |i|

  (999).downto(100).each do |j|

    result = i * j

-    p_array << result if calc?(result)
# 回文数でなければ早期リターン
+    next if result.to_s != result.to_s.reverse
# 最大値判定
+    next if result < max
  
+    max = result
+    addend1 = i
+    addend2 = j

  end

end

- puts p_array.max
+ puts max

更なる改良

f:id:ultrasevenstar:20170531070938j:plain

一旦これでソース自体は綺麗になったかとは思うんですが、 これで終わってたら文字数的にも足らないのでさらなる改良をしてみたいと思います。

今はハードコートで3桁固定になっていますが、引数で指定された桁数の回文数を表示させるよう改修してみようと思います。

まずは関数にし、引数を取るようにします

+ def palindromicNumber(num)
    max = 0
    addend1 = 0
    addend2 = 0
〜省略〜
      
+ end

次にハードコートされてる999100を動的に作成するようにしましょう

def palindromicNumber(num)

    max = 0
    addend1 = 0
    addend2 = 0

+    # 引数で設定された桁の最大値と最小値を格納する変数
+    range_max = ""
+    range_min = ""

    # 10の指数で範囲を取得
+    range_max = 10 ** (num) - 1
+    range_min = 10 ** (num - 1)


-    (999).downto(100).each do |i|
-      (999).downto(100).each do |j|

+   (range_max).downto(range_min).each do |i|
+      (range_max).downto(range_min).each do |j|

        result = i * j

        next if result.to_s != result.to_s.reverse
        next if result < max
      
        max = result
        addend1 = i
        addend2 = j

      end

    end

    puts max
end


palindromicNumber(3)

完成形

修正後のソースは以下となりました ただ4桁やとやはり時間がかかるので、range_max range_minの値を工夫する必要はありそうです。 これに関してはまた次回

def palindromicNumber(num)

    max = 0
    addend1 = 0
    addend2 = 0

    range_max = ""
    range_min = ""

    range_max = 10 ** (num) - 1
    range_min = 10 ** (num - 1)

    (range_max).downto(range_min).each do |i|
      (range_max).downto(range_min).each do |j|

        result = i * j

        next if result.to_s != result.to_s.reverse
        next if result < max
      
        max = result
        addend1 = i
        addend2 = j

      end

    end

    puts max
    puts addend1
    puts addend2
end


palindromicNumber(3)

よければ読者登録お願いします

;(function(document){ var pres = document.getElementsByTagName("pre") for(var i=pres.length; i--; ){  var el = makeOl(pres[i]) pres[i].appendChild(el) } function makeOl(pre){ if (pre.className.indexOf("gist") !== -1) { return } var ol = document.createElement("ol") , li = document.createElement("li") , df = document.createDocumentFragment() , br = pre.innerHTML.match(/\n/g) || 0 ol.className = "preLine" ol.setAttribute("role", "presentation") // no lang, no line-number if( pre.className && ! /lang-./.test(pre.className) ){ br.length += 1 } for(var i=br.length; i--; ){ var li2 = li.cloneNode(true) df.appendChild(li2) } ol.appendChild(df) return ol } })(document)