Python3,pygame オブジェクト指向、自分のコードについてのアドバイス聞きたいです(詳しくは本文で)

python3でpygameを使っています。

質問が漠然としていて申し訳ございませんが、
君のコードは汚い、オブジェクト指向がわかってないとよく言われます。

具体的にオブジェクト指向が何なのか
そしてこの関数の少なさ、ループの多さをどう解決したらいいのでしょうか。

アドバイス頂きたいと思います。

python
1import pygame
2from pygame.locals import *
3import sys
4import PygameSupportLibrary
5from PygameSupportLibrary import Image
6import math
7
8def color(Num):
9    if Num == 1:
10        return "white"
11    elif Num == 2:
12        return "black"
13
14def load_image(target):
15    return pygame.image.load(target).convert_alpha()
16
17pygame.init()
18pygame.display.set_caption(u"Othelo")
19
20board = [[0 for i in range(8)] for i in range(8)]
21board[3][3] = board[4][4] = 1
22board[3][4] = board[4][3] = 2
23
24turn = 1
25
26screen_SIZE = (500,500)
27screen = pygame.display.set_mode(screen_SIZE)
28
29boardImg = load_image('pic/board.png')
30whiteImg = load_image('pic/white.png')
31blackImg = load_image('pic/black.png')
32# 白石 = 1 , 黒石 = 2 として扱う
33
34myfont = pygame.font.SysFont(None, 60)
35
36def put_stone(ImgNum,PosNum1,PosNum2):
37    global Img
38    if ImgNum == 1:
39        Img = whiteImg
40    elif ImgNum == 2:
41        Img = blackImg
42    Pos1 = PosNum1 * 50 + 20 + (PosNum1 + 1)
43    Pos2 = PosNum2 * 50 + 20 + (PosNum2 + 1)
44
45    return screen.blit(Img,(Pos1,Pos2))
46
47def switch_turn(p_turn):
48    global turn
49    if p_turn == 1:
50        return 2
51    elif p_turn == 2:
52        return 1
53
54def ret_stone(sta1,sta2,end1,end2,direction):
55    while True:
56        p_sta1 = sta1 + direction[0]
57        p_sta2 = sta2 + direction[1]
58        board[p_sta1][p_sta2] = turn
59        if p_sta1 == end1 and p_sta2 == end2:
60            print(p_sta1,p_sta2,end1,end2,direction)
61            break
62        return
63
64def set_stone(x, y):
65    global turn
66
67    direction = []
68
69# 座標からクリックされたマスを割り出す
70    Rx = x - 20
71    Ry = y - 20
72    squ1 = math.floor(Rx / (50 + 1))
73    squ2 = math.floor(Ry / (50 + 1))
74
75    Con1 = bool     # 石があらかじめそこにないこと
76    Con2 = bool     # 石に隣接すること
77    Con3 = bool     # 盤面の中にあること
78    Con4 = bool     # 自分の石で相手の石を挟んでいること
79
80# すでに石が置いてないか → 正常作動
81    if board[squ1][squ2] == 0:
82        Con1 = True
83    else:
84        Con1 = False
85
86# 石に隣接するか → listの外の数が定義されてしまう
87    for i in range(-1,2):
88        for j in range(-1,2):
89            if board[squ1 + i][squ2 + j] == 0 or board[squ1 + i][squ2 + j] == turn:
90                pass
91            else:
92                Con2 = True
93                direction.append([i,j])
94
95    if Con2 != True:
96        Con2 = False
97
98# ボード上にますがにますが存在するか → 正常作動
99    if 0 < Rx < 409 and 0 < Ry < 409:
100        Con3 = True
101    else:
102        Con3 = False
103
104# 相手の石を挟んでいるか → 製作中
105    for direct in direction:
106        p_squ1 = squ1
107        p_squ2 = squ2
108        while True:
109            p_squ1 = p_squ1 + direct[0]
110            p_squ2 = p_squ2 + direct[1]
111            if p_squ1 == 8 or p_squ1 == -1 or p_squ2 == 8 or p_squ2 == -1:
112                break
113            if board[p_squ1][p_squ2] == turn:
114                Con4 = True
115                ret_stone(squ1,squ2,p_squ1,p_squ2,direct)
116                break
117            else:
118                pass
119
120        if Con4 != True:
121            Con4 = False
122
123# 上記4条件を満たした時実行
124    if Con1 and Con2 and Con3 and Con4 == True:
125        board[squ1][squ2] = turn
126        turn = switch_turn(turn)
127    else:
128        pass
129
130while True:
131    screen.fill((0,0,0))
132    screen.blit(boardImg,(20,20))
133
134    str_turn = myfont.render("turn=" + str(color(turn)), True, (225,225,225))
135    screen.blit(str_turn, (20,434))
136
137    for i in range(0,8):
138        for j in range(0,8):
139            if board[i][j] == 0:
140                pass
141            elif board[i][j] == 1 or 2:
142                put_stone(board[i][j],i,j)
143
144    pygame.display.update()
145
146    for event in pygame.event.get():
147        if event.type == QUIT:
148            sys.exit()
149
150        if event.type == MOUSEBUTTONDOWN and event.button == 1:
151            x, y = event.pos
152            set_stone(x, y)
153
154
155
156

補足(2015/8/13):オセロの盤面を作っています。

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

回答4件

オブジェクト指向が分かるとコードがキレイになるかというと･･･そこは関係ないと思います。言った本人はコードの質がオブジェクト指向の欠如に由来している、とまでは意味して言ってないと思いますよ。
オブジェクト指向でもなんでも、汚いコードは汚いです。

仰るとおり、関数が長すぎ(＝少なすぎ）です。関数ではなく、コメントで機能を分けてしまってるのではないでしょうか。例えばこれ

# ボード上にますがにますが存在するか → 正常作動
    if 0 < Rx < 409 and 0 < Ry < 409:
        Con3 = True
    else:
        Con3 = False

こーゆーのを関数にしてしまえばいいのではないでしょうか。
→

def valid_position(x, y):
     return 0 < x < 409 and 0 < y < 409

以下も同様です。

# 上記4条件を満たした時実行
    if Con1 and Con2 and Con3 and Con4 == True:
        board[squ1][squ2] = turn
        turn = switch_turn(turn)
    else:
        pass

→

def can_put(board, x, y):
    return valid_position(x, y) and is_neighbor_stone_exists(board, x, y) and ....

コメントするのではなく、名前をつけましょう。

それとグローバル変数を使いすぎです。絶対量のことではなく、この程度でグローバル変数をつかっていたら、数千行になったときに苦労しますよ。特にturnのようなゲームの進行によって値が変わるものはグローバルに置かないほうがよいです。最後まで変わらないもの、例えば画面サイズとか石の画像ならOKです。

投稿2015/08/12 14:49

sharow

総合スコア1149

TENTO_fumirou05

2015/08/13 04:52

global変数に関してはその方にも言われてしまいました... やはり関数で引数(メッセージ)として送って扱うという方法がいいんですかね。そしてそれがオブジェクト指向というものにつながっていくんでしょうか? 参考になりました。ありがとうございます!

sharow

2015/08/13 05:29

> そしてそれがオブジェクト指向というものにつながっていくんでしょうか? そういう面もちょっとだけあるかもしれません。オブジェクト指向というより、classの利便性ですけども、例えば関数に小分けして全部引数にしたときに put_to_board(board, x, y, stone) get_from_board(board, x, y) swap_stone_on_board(board, x, y) みたいになったときに、すべての関数が盤面への副作用や問い合わせだと気付きます。主体(モデル)は駒でも座標でもなく、盤面ですよね。そして毎回boardを引数に渡すのが面倒です。 classにしてしまえば board.put(x, y, stone) board.get(x, y) board.swap(x, y) みたいに引数にboardを渡さなくて済むわけです。最初はこんな感じでいいんじゃないでしょうか。

TENTO_fumirou05

2015/08/14 15:57 編集

なるほど!boardの部分がclass側でselfで補完されるということですね。使ってみます。ありがとうございました。

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

ベストアンサー

オブジェクト指向とコードの汚さは別問題なので、
まずコードをきれいにするところから始めましょう。

Con1 = bool     # 石があらかじめそこにないこと
Con2 = bool     # 石に隣接すること
Con3 = bool     # 盤面の中にあること
Con4 = bool     # 自分の石で相手の石を挟んでいること

きれいにするには何をするかといえば、初歩的でも重要なこととして、
たとえば上の「Con1、Con2……」のような連番は止めましょう。

そうではなく、コメントで説明していることを変数名（関数名）にしましょう。
たとえば「石があらかじめそこにないこと」なら「is_empty_cell」とか。

もしかして、これへの疑問として「変数名が長くなるではないか」
「変数の命名に時間がかかるではないか」と思うかもしれません。

しかしコードが汚いと結局、デバッグにかかる時間が増える、
という形で負債を払わされるので、そこでケチらなくていいです。

すでに石が置いてないか → 正常作動
if board[squ1][squ2] == 0:
    Con1 = True
else:
    Con1 = False

「正常作動」とありますが、テストの結果はコメントで済まさないで、
なるべくドキュメントやバージョン管理システムなどで管理しましょう。

テストのしやすさと関連して、コメントで分けるのではなく、
関数（やクラス）にして切り分けましょう。
関数やメソッドへの切り出しは非常に重要です。

もしかしてまた、これへの疑問として、
「短い関数をわざわざ作る必要はないのでは」
「関数を増やすとかえって回りくどいのでは」
と思うかもしれません。

しかし、関数へ切り分けることは、
たとえばif文やfor文のネストを浅くして
複雑性を減らすことにもつながります。

オブジェクト指向については、考え方だけ述べておきます。

サンプルコードにも表れていますが、
処理の流れをif文とfor文の組み合わせで表現する、
というスタイルで思考していると思います。

しかし、オブジェクト指向ではまったく考え方が異なります。

オブジェクトの組み合わせ（メッセージング）で表現します。
このオブジェクトとは対象とか目的という意味です。
処理ではなく対象や目的を中心に考えます。

それはどういうことでしょうか？
リバーシの例で考えてみましょう。

リバーシは盤上で駒をひっくり返していくゲームですよね。
そこで、「盤」「駒」「ひっくり返す」といった、
人間にとっての対象や目的をオブジェクトにモデリングします。

「盤」はさらに「マス目」に分割できますし、
「ひっくり返す」ことも「置く」「挟む」などに分解できるでしょう。

このモデリングの基本として、名前や命名（の背後の分析）が重要になってきます。
名前は単なるラベルでただの飾り（だから連番でもいい）、と思うかもしれませんが、
じつはオブジェクト指向の本質的な要素になっています。

最初に戻って、コードのきれいさとオブジェクト指向はイコールではありませんが、
たとえば命名を大事にすることは両者で共通しています。
ですからまず最初の一歩として、連番を止めるところから始めましょう。

投稿2015/08/12 17:29

LLman

総合スコア5592

TENTO_fumirou05

2015/08/13 04:33

一つ一つ丁寧にアドバイス頂きありがとうございましたやはり関数化やテストの結果を残すことはデバックの上でも大事そうですね... gitなどバージョン管理システムについても勉強を進めたいと思います。

TENTO_fumirou05

2015/08/13 05:03 編集

他の方々のご回答もとてもわかりやすかったのですが、 LLmanさんの回答をベストアンサーに選ばせていただき、解決済みとさせていただきましたご回答ありがとうございました!

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

LLmanさんがサラッと書かれてる「メッセージ」って概念は、意外とポイントになる気がしています…。

メッセージ脳の作り方
 メッセージ指向のイロハ

オブジェクト指向に興味があったら責務やメッセージの概念はしっかり押さえていないと、変な方向に考えが向いてしまうかもしれません。

オブジェクトを使うには、メッセージを送る。
オブジェクトには、担うべき責務がある。

当たり前だ。と思われそうですが、その意味を忘れずにいればオブジェクト指向って…？とか惑わずに済みます。

そのあとは…
機能の切り分け(責務の分担)の問題について、大いに悩みましょうw

投稿2015/08/12 19:53

退会済みユーザー

総合スコア0

TENTO_fumirou05

2015/08/13 04:28

>責務やメッセージの概念オブジェクト指向に関する理解が変わり、深まりました。おかげで一段階ステップアップできたと思います。回答ありがとうございました。

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

オブジェクト指向的な考え方を身に付けたいということであれば、
まずはオセロというゲームを「モデル化」することから始めてみてはいかがでしょうか？

以下、例ですが、、、

まず、オセロをやるには何が必要でしょうか？
とりあえず、盤(Board)と石(Stone)が必要ですよね？

では、Boardというクラスと、Stoneというクラスを書いてみましょう。

python
1class Board :
2
3class Stone :
4

では、オセロにおけるStoneとはなんでしょう？
仮に、
・黒もしくは白である
・ひっくり返えすことができる
としましょうか

Python
1class Stone :
2  def __init__(self, initial_color):
3     self.__colors = ["white","black"]
4     if initial_color in self.__colors :
5         self.__current_color = initial_color
6     else :
7         self.__current_color = self.__colors[0]
8  
9  def get_current_color(self):
10      return self.__current_color
11
12  def reverse(self):
13      if self.__current_color == self.__colors[0] :
14         self.__current_color = self.__colors[1]
15      else :
16         self.__current_color = self.__colors[0]

これでStoneをひっくり返すということが出来そうですね。

python
1stone = Stone("white")
2
3print stone.get_current_color() #->white
4
5stone.reverse()
6print stone.get_current_color() #->black
7
8stone.reverse()
9print stone.get_current_color() #->white

では、Boardとはなんでしょう？
おそらく、8かける8のマス目（Cell）を持っていて、、、
ではCellとはなんでしょう？
おそらく、石が置かれていない状態、白石が置かれた状態、黒石が置かれた状態があって、
「石を置く」という行為ができそうだから、
def set_stone(self,stone) のようなメソッドがありそうで、、、

というようにドンドンとモデル化していくのがオブジェクト指向的な考え方です。

投稿2015/08/12 14:06

編集2015/08/12 14:15