暇つぶしにどうぞ

Question

#概要
$hrtというサイズ１１の配列があります。
それぞれの項目には数値が入り
$hrt[0] と $hrt[10] は0固定です。

やりたい事は
項目が0と0で囲まれた数値の総和が4だった時、以下の処理を行いたいです。

0,1,1,1,1,0 一番右の1の位置を返す
0,1,1,2,0 2の位置を返す
0,1,2,1,0 2の位置を返す
0,2,1,1,0 2の位置を返す
0,2,2,0 スルーして、次を探す
0,1,3,0 1の位置を返す
0,3,1,0 1の位置を返す
0,4,0 4の位置を返す
#例
右の１の位置を返す
[0,1,1,1,1,0,0,0,1,1,0]] # return 4

0,1,4,0は総和が５なので無視。
0,1,2,1,0の場合は2の位置を返す。
[0,0,1,4,0,1,2,1,0,0,0] # return 6

0,2,2,0は無視、0,3,1,0(⇒7)と0,4,0(⇒9)があるが、数値が若い方を生かす
[0,2,2,0,1,0,3,1,0,4,0] # return 7

条件に合う物がない時は-1を返す。
[0,4,4,0,2,2,0,1,1,3,0] # return -1]
#一応出来てはいます
実はソースはもう出来ていて正常に動作はします。
ですが……あまりに『……』な為、
・こうした方がスマートだ
・こういう関数があるよ
・バグあるじゃない！
等の意見を頂きたいのです。
#現状のソース

$hrt = [0,1,1,0,2,0,1,3,0,1,0] #内容は例

def yyyy
    cnt = 0
    i = 1
    while i <= 9
        if $hrt[i] >= 1 then
          cnt = 0
          j = i
          while i <= 9
            break if $hrt[i] == 0
            cnt = cnt + $hrt[i]
            i = i + 1
          end
          if cnt == 4 then
            i = i - 1
            if i - j == 3 then #011110
            elsif i - j == 2 then #01120 01210 02110
              while $hrt[i] != 2
                i = i - 1
              end
            elsif i - j == 1 then #0130 0310 0220
              next if $hrt[i] == 2 #0220の場合対象外
              if $hrt[i] == 3 then
                i = i - 1
              end
            else #040
            end
            break
          end #if cnt == 4 then
        end #if $hrt[i] >= 1 then
      i = i + 1
    end #while i <= 29
    return -1 if i >= 10
    return i
end

時間がある時に暇つぶしにでもどうぞ。
以上、よろしくお願いします。

ーー追記ーー
Rubyのバージョンは1.9.4になります。

Comments

[torisan]

明確な回答はありませんが、
可読性を良くする、適した関数を使う、等の課題は含んであると考えています。

[kei344]

コードはコードブロックで囲んでいただけませんか？ ```（バッククオート3つ）で囲み、前後に改行をいれるか、コードを選択して「<code>」ボタンを押すとコードブロックになります。

[torisan]

kei344さん
こんな感じですかね？ご指摘ありがとうございます。

[kei344]

編集ありがとうございます。

Answer 1

Ruby 2.3系に対応しました。lambda_driverとactivesupportが必要です(2.4.0以上の場合はactivesupportは不要)。gem instnall lambda_driver activesupportでインストールしてから実行してください。

主な変更点: chunkではなくslice_beforeで切るようにしました。lambda_driverで関数合成するようにしました。Enumerable#sumは2.4.0からのため、2.3系についてはactivesupportの実装を使うようにしました。

# frozen_string_literal: true
require 'lambda_driver'
require 'active_support/core_ext/enumerable' if RUBY_VERSION < '2.4.0'

def yyyy(hrt)
  hrt.each_with_index
    .slice_before(&:first >> :zero?)
    .find { |a| -> (x) { x.sum == 4 && x != [0, 2, 2] } < a.map(&:first) }
    &.inject do |r, vi|
      case vi.first
      when 1 then vi.last
      when 2, 4 then break vi.last
      else r
      end
    end || -1
end

puts yyyy([0,1,1,1,1,0,0,0,1,1,0])
puts yyyy([0,0,1,4,0,1,2,1,0,0,0])
puts yyyy([0,2,2,0,1,0,3,1,0,4,0])
puts yyyy([0,4,4,0,2,2,0,1,1,3,0])
puts yyyy([0,1,1,0,2,0,1,3,0,1,0])

バージョン比較の部分は2.10.0とか出たときは対応できないバグがありますが、その前に3.0.0が出ることを期待して…。

---

【質問のコードのレビュー】

すいません、全く違う作りにしてしまった理由というか、質問のコードについて指摘事項を羅列します。

• Ruby 1.9.4は既にサポートされていません。バグが原因で動かない場合や脆弱性が発生した場合、誰も助けることはできません。サポートされたRubyのバージョンを使用してください。現在サポートされているのは、2.3系、2.2系、2.1系(ただしセキュリティ修正のみ)です。
• グローバル変数は、グローバル変数を使わなければ実現できない等のどうしても必要な理由が無い限り、使用しないでください。
• ループは一方向のみにしてください。添字のiが増えたり減ったりすることはわかりにくく、バグの温床になります。高速なアルゴリズムを実現するなどの目的が無い限り、増える方向、または、減る方向のみに絞ってください。
• 添字のループはInteger#timesやRange#eachを使ってください。whileは数がわからないと言う事情が無い限り、極力避けてください。かといって、代わりにfor inを使うことは避けてください。for inはeachとの違いが理解できない限り、使用しないでください。for inで無ければならないことはほとんどありません。
• 配列を順番に見るときは、Array#eachを使ってください。添字が必要であればArray#each_with_indexを使ってください。
• i = i + 1はi += 1と書いてください。他の部分も、複合代入演算子を使用してください。
• 同じ行に式が続くので無ければ、thenは不要です。
• 何もしないelseは不要です。
• if X == a then ... elsif X == b then ... elsif X == c then ... else ... endと言う形の場合は、case文を使用してください。
• 単純な添字ループであればiでもかまいませんが、複雑な処理になる場合は、何を意味する物かを示すように変数名を与えてください。jについても同様です。また、cntはカウンタと言うよりも数の和です。totalなどにしたほうがわかりやすいでしょう。
• 配列の長さが固定でない場合を想定してください。
• マジックナンバーを使用しないでください。最初にhrt_size = $hrt.sizeなどし、9はhrt_size - 2、10はhrt_size - 1などと表現してください。
• 一つのメソッドにおいて、行数が多く、また、インデントが深すぎです。メソッドの分離を検討してください。

色々書いてしましたが、私が10年以上前に最初に書いていたRubyのコードもこんな物でした。焦らずに、少しずつ慣れていけば良いのですから。

Comments

[torisan]

回答ありがとうございます。
バージョンは1.9.4になります。
記述がありませんでしたね、すみません。

[torisan]

ものすごい量の教示ですね。なんだか申し訳ないです。
>Ruby 1.9.4は～
ちょっと調べてみます。
＞グローバル変数は、～
質問の内容はめちゃくちゃ大きい処理の一部を抜粋して書いています。
ほとんどどこでも使用している変数の為、$付きにしています。
＞ループは一方向のみに～
これは正直自分でも思いました、気を付けます。
＞添字のループはInteger#timesや～
＞配列を順番に見るときは、Array#eachを～
今はほとんどfor文を使っていますね、どこからどこまで、というのがわかりやすいので。
配列はeachを使うといいと言うのは聞いたことはあるのですが。。
＞i = i + 1はi += 1と～
＞同じ行に式が続くので無ければ、thenは不要です。
これは私が見やすいのでわざと付けてます。
なんたらかんたら＝なんたらかんたら＋１ のようにやたら長い時は+=を使いますが。
＞何もしないelseは不要です。
普段は付けませんが、今回の場合はこの方が理解しやすいかなと思い付けました。
＞if X == a then ... elsif～
これはそうですね。Case文を使うようにします。
＞単純な添字ループであればiでも～
私の癖ですね。i,j,kなどを添え字や位置を表す変数にする事が多いです。
cntは和になっているような……？totalは参考にします。
＞配列の長さが固定でない場合を想定～
＞マジックナンバーを使用しない～
直接の質問に関係ないので、できるだけ簡易的に書かせていただきました。
＞一つのメソッドにおいて、行数が多く～
メソッドは処理ごとに分けており、元ソースには既に結構な数があります。
これ以上分けると却って見辛くなるような気がして……。
インデントは……endの後ろのコメントが物語ってますね……気を付けます。

Answer 2

他の言語でもよさそうなので参加

#!/bin/sh
ary=("0 0 2 2 1 0 0 0 3 1 0")
cc=0; ttl=0; p1=0; p2=0; p3=0; p4=0
ans=()
for xx in ${ary[@]}
do
        if  [ $cc -gt 0 ]; then
                [ $xx -eq 0 -a $ttl -eq 4 ] && ans+=("$p1 $p2 $p3 $p4")
                [ $xx -eq 0 ] && ttl=0 && p1=0 && p2=0 && p3=0 && p4=0
                [ $xx -eq 1 ] && p1=$cc
                [ $xx -eq 2 ] && p2=$cc
                [ $xx -eq 3 ] && p3=$cc
                [ $xx -eq 4 ] && p4=$cc
        fi
        (( cc++ ))
        (( ttl+=xx ))
done
[ ${#ans[@]} -eq 0 ] && echo "Ans: -1" && exit
arr=()
fa=0
for xx in $(seq 0 $(( ${#ans[@]} - 1 )))
do
        arr=(${ans[$xx]})
        if [ ${arr[0]} -gt 0 -a ${arr[1]} -eq 0 -a ${arr[2]} -eq 0 -a ${fa} -eq 0 ]; then
                fa=${arr[0]}
        elif [ ${arr[0]} -gt 0 -a ${arr[1]} -gt 0 -a ${fa} -eq 0 ]; then
                fa=${arr[1]}
        elif [ ${arr[0]} -gt 0 -a ${arr[2]} -gt 0 -a ${fa} -eq 0 ]; then
                 fa=${arr[0]}
        elif [ ${arr[3]} -gt 0 -a ${fa} -eq 0 ]; then
                 fa=${arr[3]}
        fi
done
if [ ${fa} -eq 0 ]; then
 echo "Ans: -1"
else
 echo "Ans: ${fa}"
fi

Comments

[torisan]

回答ありがとうございます。
Bashという言語そのものを初めて見たので解読に時間がかかりましたが
１～４の最終アドレスを先に保管しておき、後で判定を行う方法ですね。
特徴的で面白い記述だと感じました。

Answer 3

書かれている仕様を整理したらこんな感じでしょうか。

• 位置を返す数字の優先度は高い順に4->2->1（4があったら2,1の位置を返すことはない)
• 3の位置を返すことはない
• 返す位置は同じ数字のなかで一番右側(最後に現れる位置を返す)
• 2だけ上の条件の例外(2ゾロ目は無視)

Ruby得意な人ならもう少し綺麗になるかも？

def yyyy(hrt) 
  pos = posN = sum = 0
  for i in 0..hrt.length-1 do
    n = hrt[i]
    sum = 5 if posN == 2 and n == 2
    if [4, 2, 1].include?(n) then
      if posN <= n then
        pos = i
        posN = n
      end
    end
    sum += n
    if n == 0 then
      return pos if sum == 4
      pos = posN = sum = 0
    end
  end
  return -1
end

puts yyyy([0,1,1,1,1,0])           == 4  ? true : false
puts yyyy([0,1,1,2,0])             == 3  ? true : false
puts yyyy([0,1,2,1,0])             == 2  ? true : false
puts yyyy([0,2,1,1,0])             == 1  ? true : false
puts yyyy([0,2,2,0])               == -1 ? true : false
puts yyyy([0,1,3,0])               == 1  ? true : false
puts yyyy([0,3,1,0])               == 2  ? true : false
puts yyyy([0,4,0])                 == 1  ? true : false
puts yyyy([0,1,1,1,1,0,0,0,1,1,0]) == 4  ? true : false
puts yyyy([0,1,4,0])               == -1 ? true : false
puts yyyy([0,0,1,4,0,1,2,1,0,0,0]) == 6  ? true : false
puts yyyy([0,2,2,0,1,0,3,1,0,4,0]) == 7  ? true : false
puts yyyy([0,4,4,0,2,2,0,1,1,3,0]) == -1 ? true : false

Comments

[torisan]

回答ありがとうございます。
細かい条件を見事に切り分け、例外的な記述が最小限になっていますね。
３を無視して数字の大きい方を取るという方法は気が付きませんでした。

Answer 4

class SumNumber

  FAIL = -1
  SUM = 4

  def initialize
    @group = []
    @counter = 0
    @position = 0
  end

  def analyze(target)
    target.each_with_index do |item, index|
      divide_group(index) if item == 0
      grow_group(item) if item != 0
      return @position if @position > 0
    end
    FAIL
  end

  def grow_group(item)
    @group << item
    @group_count += item
  end

  def divide_group(ind)
    calc_pos(ind) if @group_count == SUM
    @group = []
    @group_count = 0
  end

  def calc_pos(ind)
    pos = search_position(@group)
    @position = (ind - 1) - pos if pos != FAIL
  end

  def search_position(pos)
    return FAIL if (pos[0] == 2) && (pos[1] == 2)
    return 1 if (pos[1] == 2) || (pos[1] == 3)
    return 2 if pos[0] == 2
    0
  end
end

target = [0,1,1,1,1,0,0,0,1,1,0] #=> 4
# target = [0,0,1,4,0,1,2,1,0,0,0] #=> 6
# target = [0,2,2,0,1,0,3,1,0,4,0] #=> 7
# target = [0,4,4,0,2,2,0,1,1,3,0] #=> -1

sum_number = SumNumber.new
puts sum_number.analyze(target)

---

動作しているなら質問文のコードで問題ないですが、
あえて違うスタイルの提示に意味もあるかと思い、
「手続き的な制御文の入れ子を外す」ことをテーマに、
ほかの部分はなるべくシンプルに書きました。

Comments

[torisan]

回答ありがとうございます。
確かに入れ子になっている部分は自分も気になっていました。
こういう書き方もあるんですね。
あと、数字の１が入っている部分を無視して判定している所が
個人的には気に入りました。

Answer 5

暇つぶしにJavaScriptで書きました。

class sumFourIndexOf {
  constructor(array) {
    this.arr = array
    this.arrZero = this.createArr(array.length)
    this.arrZeroIndex = this.createArrayZeroIndex(this.arr)
    this.arrPairIndex = this.searchFourIndexPair(this.arr, this.arrZeroIndex)
    this.result = this.getResultIndex(this.arr, this.arrPairIndex)
  }
  createArrayZeroIndex(array) {
    return array.reduce((arr, item, index) => {
      if (item === 0) {
        arr.push(index)
      }
      return arr
    }, [])
  }
  getSumArray(array, firstIndex, lastIndex) {
    return array.slice(firstIndex + 1, lastIndex)
  }
  isFour(array, firstIndex, lastIndex) {
    const sumArr = this.getSumArray(array, firstIndex, lastIndex)

    if (sumArr.length === 0 || this.isIgnore(sumArr)) {
      return false
    }

    const sum = sumArr.reduce((a, b) => {
      return a + b;
    }, 0)

    if (sum === 4) {
      return [firstIndex, lastIndex]
    }

    return false
  }
  isIgnore(array) {
    if (array.indexOf(2) < 0) {
      return false
    }

    return array.filter((item) => {
      return item === 2
    }).length >= 2
  }
  searchFourIndexPair(array, arrayZero) {
    return arrayZero.reduce((before, item, index, arr) => {
      const result = this.isFour(array, item, arr[index + 1])
      if (result) {
        before.push(result)
      }
      return before
    }, [])[0] || -1
  }
  createArr(len) {
    let arr = [];
    for (let i = 0; i < len; i++) {
      arr[i] = 0
    }
    return arr
  }
  getResultIndex(array, pairIndex) {
    if (pairIndex === -1) {
      return -1
    }
    const firstIndex = pairIndex[0],
      lastIndex = pairIndex[1],
      sumArr = this.getSumArray(array, firstIndex, lastIndex),
      arrBefore = this.arrZero.slice(0, firstIndex + 1),
      arrAfter = this.arrZero.slice(lastIndex),
      arr = arrBefore.concat(sumArr, arrAfter)

    if (arr.indexOf(2) >= 0) {
      return arr.indexOf(2)
    }

    if (arr.indexOf(3) >= 0) {
      return arr.indexOf(1)
    }

    if (arr.indexOf(4) >= 0) {
      return arr.indexOf(4)
    }

    return arr.lastIndexOf(1)
  }
}

var test1 = new sumFourIndexOf([0, 1, 1, 1, 1, 0, 0, 0, 1, 1, 0]);
var test2 = new sumFourIndexOf([0, 0, 1, 4, 0, 1, 2, 1, 0, 0, 0]);
var test3 = new sumFourIndexOf([0, 2, 2, 0, 1, 0, 3, 1, 0, 4, 0]);
var test4 = new sumFourIndexOf([0, 4, 4, 0, 2, 2, 0, 1, 1, 3, 0]);

console.log(test1.result); // 4
console.log(test2.result); // 6
console.log(test3.result); // 7
console.log(test4.result); // -1

Comments

[torisan]

javascriptは専門外ですが、indexOfだけ調べさせていただきました。
恐らく、対象の配列を作成して、２があるか、３があるか、４があるか、と調べていき、それに対応するリターン値をセットするという事ですね。(そのまますぎますか？)
回答ありがとうございました。

Answer 6

Javaの暇つぶしです。

public class isFour {
    
    static int check(int[] args) {

        if (args.length != 11) {
            throw new IllegalArgumentException();
        }

        for (int i=0, cnt=0, prev=0, pos=-1; i<11; ++i) {
            int value = args[i];
            if (value == 0) {
                if (cnt == 4) {
                    return pos;
                }
                cnt = 0;
            } else {
                cnt += (value == 2 && prev == 2) ? 11 : value;
                pos = (value > 1 || cnt == 1) ? i : pos;
            }
            prev = value;
        }
        return -1;

    }
    
    public static void main(String[] args) {
        System.out.println(check(new int[] {0,0,0,0,0,0,0,0,0,0,0}));
        System.out.println(check(new int[] {0,1,1,1,1,1,1,1,1,1,0}));
        System.out.println(check(new int[] {0,2,2,2,2,2,2,2,2,2,0}));
        System.out.println(check(new int[] {0,3,3,3,3,3,3,3,3,3,0}));
        System.out.println(check(new int[] {0,4,4,4,4,4,4,4,4,4,0}));
        System.out.println(check(new int[] {0,1,1,1,1,1,1,1,1,1,0}));
        System.out.println(check(new int[] {0,2,2,2,2,2,2,2,2,2,0}));
        System.out.println(check(new int[] {0,3,3,3,3,3,3,3,3,3,0}));
        System.out.println(check(new int[] {0,2,2,0,2,2,0,2,1,2,0}));
        System.out.println("---");
        System.out.println(check(new int[] {0,1,1,1,1,0,1,3,0,1,0}));
        System.out.println(check(new int[] {0,2,1,1,0,1,3,0,1,1,0}));
        System.out.println(check(new int[] {0,1,2,1,0,1,3,0,1,2,0}));
        System.out.println(check(new int[] {0,1,1,2,0,1,3,0,3,1,0}));
        System.out.println(check(new int[] {0,2,2,0,1,1,2,0,1,3,0}));
        System.out.println(check(new int[] {0,2,2,0,1,1,1,0,1,3,0}));
        System.out.println(check(new int[] {0,2,2,0,1,1,1,0,3,1,0}));
        System.out.println(check(new int[] {0,2,2,0,1,3,1,1,0,4,0}));
    }
}

###バグ修正（追記）
バグを修正しました。仕様をよく読んでいなかったのが原因。

その前に、Bertrand Meyerの「契約による設計」に従い、事前条件チェックをちゃんとやります。

if (args == null && args.length != 11) {
    throw new IllegalArgumentException();
}
if (args[0] != 0 || args[10] != 0) {
    throw new IllegalArgumentException();
}

数列の積を使ってパターンを判定するようにしました。

static int check(int[] args) {

    for (int i=0, cnt=0, prod=1, pos=-1; i<11; ++i) {
        int value = args[i];
        if (value == 0) {
            if (cnt == 4) {
                return pos;
            }
            cnt = 0;
            prod = 1;
        } else {
            prod *= value;
            cnt += (value == 2 && prod == 4) ? 11 : value;
            pos = ((value == 2 || value == 4) || 
                    prod == 1 || (prod == 3 && value == 1)) ? i : pos;
        }
    }
    return -1;

}

このコードが良くないのは、強引なやり方をしていてよく分からないことと、仕様変更に弱いことです。

正規表現を使う方が、仕様変更に強い（正規表現だけを変えれば良い）し、わかりやすいと思いましたので、掲載します。

static final String SEQ_PATTERN = 
                  "^.*0111(1)0|011(2)0|01(2)10|0(2)110|0(1)30|03(1)0|0(4)0.*$";

static int check(int[] args) {

    StringBuilder buff = Arrays
        .stream(args)
        .collect(
            StringBuilder::new, 
            (b,v) -> b.append(String.valueOf(v)), 
            (c,d) -> c.append(d)
    );
    String seq = new String(buff);
    Matcher m = Pattern.compile(SEQ_PATTERN).matcher(seq);
    if (m.find()) {
        for (int i=1; i<=m.groupCount(); ++i) {
            if (m.group(i) != null) {
                return m.start(i);
            }
        }
    }
    return -1;
}

正規表現の書き方が悪いとか、ストリームAPIの使い方がよくないと思っています。しかし、仕様変更があってもコードを変更しなくても良いところが気に入っています。

Comments

[torisan]

回答ありがとうございます。
Javaはほとんど扱ったことが無いのですが
このソースの場合、
0,1,1,1,1,0 -> 左端
0,3,1,0 0,1,3,0 -> 3の位置
になりませんか？見当違いだったらすみません。

[退会済みユーザー]

そですね。仕様の読み違いです。posへの代入条件を直して対応します。それとは別に、汎用性があるのは正規表現かなと思っています。

[退会済みユーザー]

final String SEQ_PATTERN = "^.*0111(1)0|011(2)0|01(2)10|0(2)110|0(1)30|03(1)0|0(4)0.*$"

[torisan]

正規表現での記述は確かにわかりやすいですね。
わざわざ修正ありがとうございました。