オセロプログラムの添削をお願いします

プログラミングの基礎ばかりを学習し、実践経験が全くない者です。
そこでこの度、実際にオセロのプログラムコードをJavaScriptで書いてみました。

しかし、このように一つの形にしたプログラミングは今回が初めてで、
普段からプログラミングに慣れ親しんでおられる方には、
非常に汚いコードに見えることと思います。

「こういう書き方はまずい」、「自分だったらこう書く」といったような
添削をしていただけると、今後プログラミングをしていく上で非常に助かります。

長いプログラムコードではありますが、何卒よろしくお願いいたします。

###ソースコード

HTML5
1<script type="text/javascript">
2<!-- 
3	// 黒と白の画像を変数に格納
4	var black = 'url("http://i.imgur.com/CLEpdaJ.png")';
5	var white = 'url("http://i.imgur.com/VZgXNBM.png")';
6	
7	var turn = 1; // 1が黒、2が白
8	
9	var turnflg = false;
10	var endflg = false;
11	
12	/* 10×10のフィールドを作る */
13	var field = [
14		[9, 9, 9, 9, 9, 9, 9, 9, 9, 9],
15		[9, 0, 0, 0, 0, 0, 0, 0, 0, 9],
16		[9, 0, 0, 0, 0, 0, 0, 0, 0, 9],
17		[9, 0, 0, 0, 0, 0, 0, 0, 0, 9],
18		[9, 0, 0, 0, 0, 0, 0, 0, 0, 9],
19		[9, 0, 0, 0, 0, 0, 0, 0, 0, 9],
20		[9, 0, 0, 0, 0, 0, 0, 0, 0, 9],
21		[9, 0, 0, 0, 0, 0, 0, 0, 0, 9],
22		[9, 0, 0, 0, 0, 0, 0, 0, 0, 9],
23		[9, 9, 9, 9, 9, 9, 9, 9, 9, 9]
24	]; 
25	
26	/* ------------------------------------------------------- */
27	
28	function showBoard() {
29		var element = document.getElementById("othello");
30		
31		// 盤面を描写する処理
32		for(var y = 1; y <= 8; y++){
33			for(var x = 1; x <= 8; x++) {
34				field[y][x] = document.createElement("div");
35				field[y][x].id = "p" + y + x;
36				
37				field[y][x].style.border = "1px solid black";
38				field[y][x].style.cssFloat = "left";
39				field[y][x].style.height = "60px";
40				field[y][x].style.width = "60px";
41				field[y][x].style.background = "green";
42				field[y][x].style.cursor = "pointer";
43				
44				field[y][x].addEventListener("click", clickEvent, false);
45				
46				// 8ピース毎に改行する
47				if((x % 8) == 1){
48					field[y][x].style.clear = "both";
49				}
50				element.appendChild(field[y][x]);
51			}
52		}
53		
54		// 黒と白の初期配置
55		field[4][5].style.backgroundImage = black;
56		field[5][4].style.backgroundImage = black;
57		field[4][4].style.backgroundImage = white;
58		field[5][5].style.backgroundImage = white;
59	}
60	
61	// 石を返すことができるかどうかを判別する
62	function checkAble(y, x){
63		
64		var pos_y = [-1, -1, 0, 1, 1, 1, 0, -1];
65		var pos_x = [0, 1, 1, 1, 0, -1, -1, -1];
66		
67		// 選択した箇所に石が置かれていない場合のみ処理する
68		if(field[y][x].style.backgroundImage == "none"){
69			var stone;
70			turn == 1 ? stone = black : stone = white
71			
72			// 選択した箇所の周囲8箇所に石が置けるかどうか判別する
73			for(var pos = 0; pos < 8; pos++){
74				var count = 0;
75				var n = y + pos_y[pos];
76				var m = x + pos_x[pos];
77				
78				// 周囲8箇所が壁、自分の石、空白の場合、以降の処理を行わない
79				if(field[n][m] == 9 || field[n][m].style.backgroundImage == stone
80					|| field[n][m].style.backgroundImage == "none"){
81					continue;
82				}
83				
84				// 周囲8箇所の延長線上に相手の石がある間繰り返す
85				while(field[n][m].style.backgroundImage != stone){
86					n += pos_y[pos];
87					m += pos_x[pos];
88					
89					count++;
90					
91					// 相手の石を挟んで自分の石がある場合、間の相手の石をひっくり返す
92					if(field[n][m] != 9 && field[n][m].style.backgroundImage == stone){
93						while(count >= 0){
94							n -= pos_y[pos];
95							m -= pos_x[pos];
96						
97							field[n][m].style.backgroundImage = stone;
98							count--;
99						}
100						
101						// 手番を交代する
102						turnflg = true;						
103					}
104					
105					// 周囲8箇所の延長線上が壁、または相手の石を挟んで自分の石がない場合、
106                    //何もせず処理を終了する
107					if(field[n][m] == 9 || field[n][m].style.backgroundImage == "none"){
108						break;
109					}
110				}
111			}
112		}
113	}
114	
115	function clickEvent() {
116		var element = document.getElementById(this.id);
117		var y, x;
118		
119		// 設定したdividから配列の要素を得る
120		y = parseInt(element.id.substr(-2, 1));
121		x = parseInt(element.id.substr(-1, 1));
122		
123		checkAble(y, x);
124		
125		// ゲームが終了するまでターンを入れ替える
126		if(!endflg && turnflg){
127			turn == 1 ? turn = 2 : turn = 1
128			turnflg = false;
129		}
130	}
131	
132	onload = function() {
133		showBoard();
134	}
135-->
136</script>
137
138// オセロプログラムを表示
139<div id="othello"></div>

###補足情報
仕様としては、一人で黒と白の石を持ち、相手の石を挟めるマス目にだけ置くことが可能で、
交互に打ち合うだけのものです。

行動規範の内容に同意します

回答3件

元のコードを少し変更してみました。

グローバル変数を減らした。
手番 (黒、白) を BLACK, WHITE の定数で指定するようにした。
field[][] には、石の状態 (integer) だけを保持するようにした。

TODO:
ゲーム終了(勝敗がついた), パス(石を置ける場所がないので相手に手番を渡す) の処理をすること。
石をひっくり返す時にアニメーションする。
石を置ける場所をヘルプ表示する。

html
1<!DOCTYPE html>
2<html>
3  <head>
4    <meta http-equiv="content-language" content="ja">
5    <meta charset="UTF-8">
6</head>
7<html>
8  <body>
9    <div id="othello"></div>
10    <script type="text/javascript" src="reversi.js"></script>
11  </body>
12</html>

javascript
1var STONE_BLACK = 'url("http://i.imgur.com/CLEpdaJ.png")';
2var STONE_WHITE = 'url("http://i.imgur.com/VZgXNBM.png")';
3var BLACK = 1;
4var WHITE = 2;
5
6var turn = BLACK;    // 先手は黒
7
8// 10×10のフィールドを作って、石の色を保持する。
9// 0: 石がない, BLACK: 黒石, WHITE: 白石、9: 壁
10var field = [
11    [9, 9, 9, 9, 9, 9, 9, 9, 9, 9],
12    [9, 0, 0, 0, 0, 0, 0, 0, 0, 9],
13    [9, 0, 0, 0, 0, 0, 0, 0, 0, 9],
14    [9, 0, 0, 0, 0, 0, 0, 0, 0, 9],
15    [9, 0, 0, 0, 0, 0, 0, 0, 0, 9],
16    [9, 0, 0, 0, 0, 0, 0, 0, 0, 9],
17    [9, 0, 0, 0, 0, 0, 0, 0, 0, 9],
18    [9, 0, 0, 0, 0, 0, 0, 0, 0, 9],
19    [9, 0, 0, 0, 0, 0, 0, 0, 0, 9],
20    [9, 9, 9, 9, 9, 9, 9, 9, 9, 9]
21];
22// -------------------------------------------------------
23// 盤面を表示する。
24function showBoard() {
25    var element = document.getElementById("othello");
26    // 盤面を描写する処理
27    for (var y = 1; y <= 8; y++) {
28        for (var x = 1; x <= 8; x++) {
29            var f = document.createElement("div");
30            f.id = "p" + y + x;
31            f.style.border = "1px solid black";
32            f.style.cssFloat = "left";
33            f.style.height = "60px";
34            f.style.width = "60px";
35            f.style.background = "green";
36            f.style.cursor = "pointer";
37            f.addEventListener("click", clickEvent, false);
38
39            // 8ピース毎に改行する
40            if ((x % 8) == 1) {
41                f.style.clear = "both";
42            }
43
44            // 石を置く。
45            if (field[y][x] !== 0) {
46              var image = (field[y][x] == BLACK)? STONE_BLACK : STONE_WHITE;
47              f.style.backgroundImage = image;
48            }
49            element.appendChild(f);
50        }
51    }
52}
53
54// player が（y、x）に石を置くことができるかどうかを判別する。
55// 置ける場合は、盤面を更新する。
56function playable(y, x, player) {
57    var ans = false;
58    var opponent = (player == BLACK) ? WHITE : BLACK;
59    var delta_y = [-1, -1, 0, 1, 1, 1, 0, -1];
60    var delta_x = [0, 1, 1, 1, 0, -1, -1, -1];
61
62    // 選択した箇所に石が置かれていない場合のみ処理する
63    if (field[y][x] === 0) {
64        // 選択した箇所の周囲8箇所に石が置けるかどうか判別する
65        for (var pos = 0; pos < 8; pos++) {
66            var count = 0;
67            var n = y + delta_y[pos];
68            var m = x + delta_x[pos];
69
70            // 周囲8箇所が壁、自分の石、空白の場合、以降の処理を行わない
71            if (field[n][m] === 9 || field[n][m] === player || field[n][m] === 0) {
72                continue;
73            }
74            // 周囲8箇所の延長線上に相手の石がある間繰り返す
75            while (field[n][m] === opponent) {
76                n += delta_y[pos];
77                m += delta_x[pos];
78                count++;
79
80                // 相手の石を挟んで自分の石がある場合、間の相手の石をひっくり返す
81                if (field[n][m] === player) {
82                    ans = true;  // 石が置けた
83                    while (count >= 0) {
84                        n -= delta_y[pos];
85                        m -= delta_x[pos];
86                        var f = document.getElementById("p" + n + m);
87                        f.style.backgroundImage = (player == BLACK)? STONE_BLACK : STONE_WHITE;
88                        field[n][m] = player;
89                        count--;
90                    }
91                }
92            }
93        }
94    }
95    return ans;
96}
97
98function clickEvent() {
99    var element = document.getElementById(this.id);
100    // 設定したdividから配列の要素を得る
101    var y = parseInt(element.id.substr(-2, 1));
102    var x = parseInt(element.id.substr(-1, 1));
103
104    var check = playable(y, x, turn);
105    if (check) {
106        turn = (turn == BLACK)? WHITE : BLACK;
107    }
108    // TODO: PASS (置ける場所がない)、ゲーム終了の判定を追加すること！
109}
110
111onload = function() {
112    // 黒と白の初期配置
113    field[4][5] = BLACK;
114    field[5][4] = BLACK;
115    field[4][4] = WHITE;
116    field[5][5] = WHITE;
117    showBoard();
118};

蛇足：
https://github.com/katoy/sample-enchant から
enchantjs で作ったオセロ http://homepage2.nifty.com/youichi_kato/src/github/enchant/reversi/index.html を参照できます。

投稿2015/10/11 14:43

katoy

総合スコア22324

the_saidaa

2015/10/12 07:35

なんと！質問を投稿してからこの僅かの間に、自分の書いたコードをベースにして、新たな機能まで追加してくださるとは！感謝もそうですが、驚きのほうが大きいです。加えて、ソースコードがとても読みやすいです。特に、field配列にint型のみが格納されている部分は是非とも参考にしたいところ、ここは自分でコードを書いていても、かなり気に掛かっていた部分でした。とはいえ、最初からこのソースコードを見るようなことはしません。もう一度、自分なりに修正を加えていく上で、その答え合わせのような位置付けで参考にしたいと思います。本当にありがとうございました。本当にお疲れ様でした。

katoy

2015/10/12 07:52

改良したコードには "新たな機能まで追加" はしていません。追加するとよいと思うことを列挙しているだけです。 the_saidaa さんが修正・機能追加したコードを是非再投稿していただけるると、さらにまたいろいろ指摘やコメントが集まると思います。 (個人的には javascript でなく coffeescript で書くと、コードが短くなるとおもう)

行動規範の内容に同意します

もう解決済みですが個人的に思ったことを。
即時関数内に閉じ込めることでグローバル変数を減らせます。
次にDOM操作に関して、ループ内でDOMツリーに追加するのではなく、DocumentFragmentを使用し最後にまとめて追加することでパフォーマンスが上がるかと。

投稿2015/10/11 14:53

Cf_cwd

総合スコア730

the_saidaa

2015/10/12 07:19

>即時関数内に閉じ込めることでグローバル変数を減らせます確かに、「とりあえず動けばいいや」的な考えで、何の考えもなしにグローバル変数を定義してしまったものは多いです･･･以後、気を付けます。 >DocumentFragmentを使用し最後にまとめて追加することでパフォーマンスが上がるお恥ずかしながら、この回答を見るまでDocumentFragmentの存在を知りませんでした･･･逐一、8×8の64回に分けて要素を追加するよりも、 1回にまとめて追加したほうが断然いいですね。 Cf_cwdさんの回答だけでも、一気に知識の幅が広がったように思います。解決済みの質問にまでコメントを寄せていただき、本当にありがとうございました。

Cf_cwd

2015/10/12 14:14

いえいえ。あとは思い付きですが、石は恐らく共通だと思うので一度作成した物をcloneNode()を使用してコピーするようにするともしかするとパフォーマンスが上がるかもしれません。(DOM操作の回数が減るので) あと今さらっとコードを見ていて、複数のstyleを変更している箇所があることに気が付きました。これはsetAttribute("style",value)の形にすることでスマートにできますね。ですからあらかじめsetAttribute()したdiv要素を作成しておき、ループ内でcloneNode()するようにするとよさそうですね

Cf_cwd

2015/10/12 15:07

ちょっとちゃんと読んでみるとsetAttribute()だとfloatをclearする時、不都合があるかな？検証してないな……

Cf_cwd

2015/10/12 15:43

三項演算子後のセミコロンがないことが気になります、またこういった書き換えができるかと。 turn == 1 ? stone = black : stone = white ↓ stone = (turn == 1) ? black : white; 先ほど色々と自己流に書き換えてみたのですが不具合のようなものが出ていて現状出せません。(かなり大雑把に弄ったことによって細かな抜けがあると予想) ですので変更した内容を箇条書きに。・全ての変数を即時関数内に閉じ込めた(クロージャとして動作する) ・turnをturnStateと変更し、黒を1、白を-1として扱う (そうすることで黒と白を入れ替えるとき turnState = -turnState; と書くことで実現できる) ・backgroundImageというオブジェクトを定義・画像をbackgroundImage.black、backgroundImage.whiteに代入・isWall()、isMyStone()、backgroundImage.isNull()といった関数を定義することで、if文内を整理できます・三項演算子を整理・cloneNode()、setAttribute()による方法が動作することを確認他にもありますが連投かつ、長文ですのでここまで

行動規範の内容に同意します

ベストアンサー

拝読させていただいたところ、真っ先に浮かんだのはCheckAble関数の修正です。

まずは、条件分岐について。
「参照先は壁か」「参照先は空か」「参照先には自分の石があるか」という条件は、このアルゴリズムにおいて頻出し、かつ重要なものです。
なので、これらを全て関数にして共通化し、かつ名前を付けて分かりやすくしてみましょう。
「参照先は壁か」→isWall
「参照先は空か」→isEmpty
「参照先に自分の石があるか」→isExistMine

javascriptでも同様かわかりませんが、私が良く書くJavaやC#では、真偽値(true/false)を返す関数(メソッド)にはisを先頭に付ける、という慣習があります(Rubyでは?を最後に付けます。言語によって違いがあるようです）。

変数およびメソッドの名前も、改良の余地がありそうです。
自分の石を表す変数としてstoneが利用されていますが、単に「石」と呼ぶ場合、それは自分の石と相手の石どちらとも取れます。ここでは、stone = 自分の石として用いているようなので、その意図が一見して分かるようにすると、可読性が上がります。
例）stone → myStone

石をひっくり返すかを判定する関数として、checkAble（可能か確認する）という名前をつけていますが、これだけだと「何について」可能か確認するのか分かりません。これも、意図を込めた名前にすると良いでしょう。
例）checkAble→checkReversible

最後に、checkAble関数の仕事を確認してみましょう。checkAbleは、名前の上では「ひっくり返せる石があるかどうかを確認すること」が仕事ですが、コードでは「実際に石をひっくり返す」作業も担当しています。
今回のように、一つのメソッドに複数の作業を混在させ、かつ同一のデータ（ここではfield[n][m]）を各作業で利用すると、分岐条件や石をひっくり返すロジックに修正が生じた場合、互いに影響を与えてしまう恐れがあります。すると、修正範囲が広がったり、バグの原因が特定しにくくなったりします。
（結構面倒そうなので）ここでは省略させて頂きますが、実際にはcheckAbleを二つの関数に分けてみると、各処理毎に影響範囲を限定できます。挑戦してみてください。
例)
checkAble → isReversibleとreverseに分割

投稿2015/10/11 08:45

編集2015/10/11 08:50