このコードは良いコードですか？

python
1import numpy as np
2import pygame
3import os
4
5BLACK = (0,0,0)
6WHITE = (255,255,255)
7GLAY = (100,100,100)
8SKYBLUE = (100,200,300)
9BROWN = (100,500,0)
10GREEN = (0,255,0)
11red = (255,0,0)
12width = 1000
13height = 1000
14
15pygame.init()
16
17screen = pygame.display.set_mode((width,height))
18myclock = pygame.time.Clock()
19myfont_small_small=pygame.font.Font(None,16)
20myfont_small=pygame.font.Font(None,32)
21myfont=pygame.font.Font(None,64)
22myfont_big=pygame.font.Font(None,100)
23myfont_big_big=pygame.font.Font(None,200)
24
25scare = 1/20000
26time_per_flame = 1
27
28display_position_x = 0
29display_position_y = 0
30
31
32gravitational_constant = 6.667e-11
33
34def absolute_path_maker(relative_path):
35    """making absolute path　images\????.png"""
36    return os.path.join(os.getcwd(), relative_path)
37
38def to_vector(before):
39    aspect = before[0]
40    power = before[1]
41    value = ((power**2)/(aspect[0]**2 + aspect[1]**2))**(1/2)
42    return_val = np.array([aspect[0] * value,aspect[1] * value])
43    del value
44    del power
45    del aspect
46    return return_val
47
48class heavenly_body:
49    def __init__(self,position_x,position_y,mass,initial_acceleration):
50        """the position unit must be "m".the mass unit must be "kg"."""
51        self.position_x = position_x
52        self.position_y = position_y
53        self.speed = np.array([0,0])
54        self.acceleration = np.array([0,0])
55        self.acceleration_list = {(0,0):0}#the unit is "m/s"
56        self.acceleration_list = {}
57        self.power = {(0,0):0}#the unit is "N"
58        self.power = {}
59        self.mass = mass
60        for direction,power in initial_acceleration.items():
61            self.acceleration_list[direction] = power
62    def update(self,group):
63        global time_per_flame
64        global gravitational_constant
65        for ins in group:
66            if ins.cla != self:
67                distance_x = ins.cla.position_x - self.position_x
68                distance_y = ins.cla.position_y - self.position_y
69                self.distance = ((distance_x ** 2) + (distance_y ** 2)) ** (1/2)
70                power = ((self.mass * ins.cla.mass)/(self.distance ** 2))*gravitational_constant
71                ins.cla.power[(-distance_x,-distance_y)] = (time_per_flame**2) * power
72        for directions,power in self.power.items():
73            self.acceleration_list[directions] = power/self.mass
74        self.power = {}
75        self.acceleration = np.array([0,0])
76        for aspect,power in self.acceleration_list.items():
77            self.acceleration = self.acceleration + to_vector([aspect,power])
78        self.speed = self.speed + time_per_flame * self.acceleration
79        self.acceleration_list = {}
80        self.position_x += time_per_flame * self.speed[0]
81        self.position_y += time_per_flame * self.speed[1]
82
83class heavenly_body_pygame(pygame.sprite.Sprite):
84    def __init__(self,position_x,position_y,mass,initial_acceleration = {(1,1):0},planet_image_path = "images\main.png"):
85        global scare
86        pygame.sprite.Sprite.__init__(self)
87        self.image = pygame.image.load(absolute_path_maker(planet_image_path))
88        self.rect = self.image.get_rect()
89        self.rect.centerx = int(position_x * scare)
90        self.rect.centery = int(position_y * scare)
91        global heavenly_body
92        self.cla = heavenly_body(position_x,position_y,mass,initial_acceleration)
93    def update(self,group):
94        global scare
95        global display_position_x
96        global display_position_y
97        self.cla.update(group.sprites())
98        next_position_x = self.cla.position_x
99        next_position_y = self.cla.position_y
100        next_position_x = next_position_x * scare
101        next_position_y = next_position_y * scare
102        next_position_x += display_position_x
103        next_position_y += display_position_y
104        next_position_x = int(next_position_x)
105        next_position_y = int(next_position_y)
106        self.rect.centerx = next_position_x
107        self.rect.centery = next_position_y
108
109star = heavenly_body_pygame(10000000,10000000,5.972e+20,{(0,-1):0},"images\sun.png")
110planet = heavenly_body_pygame(10000000,13758000,7.347e+18,{(-1,0):102},"images\e.png")
111planet2 = heavenly_body_pygame(10000000,3000000,7.347e+18,{(-1,0):30},"images\c.png")
112planet3 = heavenly_body_pygame(10000000, 9000000, 1e+18, {(1,0):130},"images\m.png")
113planet4 = heavenly_body_pygame(10000000, 20000000, 1e+19, {(-1,0):50},"images\j.png")
114planet5 = heavenly_body_pygame(10000000, 8000000, 1e+10, {(-1,0):150},"images\s.png")
115planets = pygame.sprite.Group()
116planets.add(planet)
117planets.add(star)
118planets.add(planet2)
119planets.add(planet3)
120planets.add(planet4)
121planets.add(planet5)
122state = "title"
123
124key_pressing = []
125
126while state == "title":
127    for event in pygame.event.get():
128        if event.type == pygame.QUIT:
129            state = "end"
130        if event.type == pygame.KEYUP:
131            if event.key == pygame.K_SPACE:
132                state = "normal"
133while state != "end":
134    screen.fill((0,0,0))
135    for i in range(60):
136        planets.update(planets)
137    planets.draw(screen)
138    pygame.display.flip()
139    for event in pygame.event.get():
140        if event.type == pygame.QUIT:
141            state = "end"
142        if event.type == pygame.KEYDOWN:
143            if event.key == pygame.K_LEFT:
144                key_pressing.append("left")
145            if event.key == pygame.K_RIGHT:
146                key_pressing.append("right")
147            if event.key == pygame.K_UP:
148                key_pressing.append("up")
149            if event.key == pygame.K_DOWN:
150                key_pressing.append("down")
151            if event.key == pygame.K_LCTRL:
152                key_pressing.append("lctrl")
153            if event.key == pygame.K_RCTRL:
154                key_pressing.append("rctrl")
155        if event.type == pygame.KEYUP:
156            if event.key == pygame.K_LEFT:
157                key_pressing.remove("left")
158            if event.key == pygame.K_RIGHT:
159                key_pressing.remove("right")
160            if event.key == pygame.K_UP:
161                key_pressing.remove("up")
162            if event.key == pygame.K_DOWN:
163                key_pressing.remove("down")
164            if event.key == pygame.K_LCTRL:
165                key_pressing.remove("lctrl")
166            if event.key == pygame.K_RCTRL:
167                key_pressing.remove("rctrl")
168            if event.key == pygame.K_SPACE:
169                if ("lctrl" in key_pressing) or ("rctrl" in key_pressing):
170                    scare = scare / 2
171                    display_position_x = display_position_x / 2
172                    display_position_y = display_position_y / 2
173                    display_position_x = display_position_x + width / 4
174                    display_position_y = display_position_y + height / 4
175                else:
176                    scare = scare * 2
177                    display_position_x = display_position_x * 2 
178                    display_position_y = display_position_y * 2
179                    display_position_x = display_position_x - width / 2
180                    display_position_y = display_position_y - height / 2
181    for i in key_pressing:
182        if i == "left":
183            display_position_x += 10
184        if i == "right":
185            display_position_x -= 10
186        if i == "up":
187            display_position_y += 10
188        if i == "down":
189            display_position_y -= 10
190    myclock.tick(60)
191pygame.quit()
192

pythonで、天体について書いてみたのですが、
このコードは良いコードですか？
そもそも物理法則的に間違っていたらそれについても教えていただけないでしょうか？

maisumakun

2020/10/09 13:56

何をもって「よい・悪い」と評価すればいいのでしょうか？

kazuoiwata

2020/10/09 13:58

誰が見てもわかるコードであるか、改善点についてもお願いします

gentaro

2020/10/09 15:22

そういうのはお金払って先生でも雇って聞いてください https://menta.work/

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

回答4件

ベストアンサー

気になる点をつらつら書きます。

コメントがほぼない
グローバル領域へのアクセスが多い
マジックナンバーが多い（せめてなぜその数値かは書くべき）
変数名、特にループ変数名が分かり辛い
命名に一貫性が無い（GREEN/red, planet/planet2）
スタイルが統一されていない（=の両側にスペースを入れるか、など）
ネストが深い

特に最初の事項は致命的です。

細かいこと

Python

import numpy as np
import pygame
import os

標準モジュールを先に読み込んだ方がトラブルが少ないです。

Python

BLACK = (0,0,0)
...
red = (255,0,0)

Python

myfont_small_small=pygame.font.Font(None,16)
...
myfont_big_big=pygame.font.Font(None,200)

辞書、あるいは列挙型にしましょう。

Python

def to_vector(before):
aspect = before[0]
power = before[1]
value = ((power**2)/(aspect[0]**2 + aspect[1]2))(1/2)
return_val = np.array([aspect[0] * value,aspect[1] * value])
del value
del power
del aspect
return return_val

逐一delする必要はありません。GCに任せましょう。

Python

self.acceleration_list = {(0,0):0}#the unit is "m/s"
self.acceleration_list = {}
self.power = {(0,0):0}#the unit is "N"
self.power = {}

無駄です。

Python

self.distance = ((distance_x ** 2) + (distance_y ** 2)) ** (1/2)

ユークリッド距離は関数に切り出しても良いかと。
どこかのモジュールにありそうな気がします。

Python

star = heavenly_body_pygame(10000000,10000000,5.972e+20,{(0,-1):0},"images\sun.png")
planet = heavenly_body_pygame(10000000,13758000,7.347e+18,{(-1,0):102},"images\e.png")
planet2 = heavenly_body_pygame(10000000,3000000,7.347e+18,{(-1,0):30},"images\c.png")
planet3 = heavenly_body_pygame(10000000, 9000000, 1e+18, {(1,0):130},"images\m.png")
planet4 = heavenly_body_pygame(10000000, 20000000, 1e+19, {(-1,0):50},"images\j.png")
planet5 = heavenly_body_pygame(10000000, 8000000, 1e+10, {(-1,0):150},"images\s.png")
planets = pygame.sprite.Group()
planets.add(planet)
planets.add(star)
planets.add(planet2)
planets.add(planet3)
planets.add(planet4)
planets.add(planet5)

最初からリストに直接放り込めば良いのでは？

読み易いコードの書き方

コメントについてまた質問ですが、どのようなことを書いたらいいんでしょうか？

WhatとWhyを書くようにします。
ただしWhatは命名やモジュールの切り分けで表現できる場合があります。

###Whatの例

例えば、これ、第三者がいきなり見て意味が分かるでしょうか？

Python
1planet = heavenly_body_pygame(10000000,13758000,7.347e+18,{(-1,0):102},"images\e.png")

とりあえず惑星なんだなということしか分からないです。
こんなふうに命名すると情報が少し増えます。（火星にしたのは仮です）

Python
1mars = heavenly_body_pygame(10000000,13758000,7.347e+18,{(-1,0):102},"images\e.png")

それぞれの引数についても、一度見ただけではわからないです。
幸いPythonにはキーワード引数の機能がありますので、次のように書けます。

Python
1mars = heavenly_body_pygame(
2    position_x=10000000, position_y=13758000, mass=7.347e+18,
3    initial_acceleration={(-1,0):102},
4    planet_image_path="images\e.png"
5)

キーワード引数が使えない場合はコメントで代替します。
引数名だけでは説明が不足する部分をコメントで補足するのも良いアイデアです。

Python
1mars = heavenly_body_pygame(
2    10000000, 13758000,    # x座標, y座標
3    7.347e+18,             # 質量(単位はoooo)
4    {(-1,0): 102},         # 加速度初期値
5    "images\e.png"        # 惑星画像のパス
6)

ちょっと話は逸れますが、単位は変数名に入れた方が無難です。
mass_kgとかmass_tonnとか。いわゆるアプリケーションハンガリアンですね。

###Whyの例

例えば、次のコード。この変数がなぜ必要なのか読み取れないです。

Python

key_pressing = []

次のように理由が書いてあるだけでだいぶ印象が変わります。

Python
1# 同時押しを判定するために、押されているキーを格納する
2key_pressing = []

投稿2020/10/09 14:04

編集2020/10/10 01:16

LouiS0616

総合スコア35668

kazuoiwata

2020/10/10 00:17

細かいところまでご指摘ありがとうございます。コメントについてまた質問ですが、どのようなことを書いたらいいんでしょうか？まだ始めたばかりでよくわかってなくて申し訳ないです。

LouiS0616

2020/10/10 01:17

回答に追記しました。

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

コードそのものには、他の皆さんが良いコメントをされていますので、別の観点でコメントします。

このくらいの規模のコードが書けるようになってきたら、著名なライブラリパッケージを使いましょう。コードの見通しがよくなり、再利用性が高まります。

numpyをインポートして使われていますが、せっかくのnumpyが生かされていません。速度や加速度に使われるのみで、位置はhoge_x、hoge_yといった形で扱われています。また速度や加速度も実際の計算はnumpyを使わずにx座標とy座標をバラバラで計算しているようです。位置も数値の組み合わせとしてnumpyの配列として扱うことで、位置・速度・加速度の変換計算もnumpy配列のままで簡単にできます。また、numpy配列を直接pygameに送り込むことはできませんが、エレガントに送り込む方法はググるといろいろ出てきます。

さらに、numpyとセットで数値演算を強化したscipyというライブラリがあります。これを使うと速度や加速度といった物理演算がもっと簡単にできます。物理シミュレーションゲームも作れると思います。

Pythonの魅力の1つは豊富なライブラリです。やりたいことをググっていろいろ調べると、その領域でよく使われているライブラリがわかりますので、ぜひそれを使うことをチャレンジしてみてください。

参考: Python 数値解析

投稿2020/10/09 23:12

toast-uz

総合スコア3266

最低限、PEP8に従っていると良いですね。

PEP8

あと、ifが連続していますが、elifではダメなんですかね？

投稿2020/10/09 14:33

takutakuya

総合スコア979

ぱっと見でいうなら、もっと関数化できると思います。
この辺とか規則性ありますよね。
if event.type == pygame.KEYDOWN:
if event.key == pygame.K_LEFT:
key_pressing.append("left")
if event.key == pygame.K_RIGHT:
key_pressing.append("right")
if event.key == pygame.K_UP:
key_pressing.append("up")
if event.key == pygame.K_DOWN:
key_pressing.append("down")
if event.key == pygame.K_LCTRL:
key_pressing.append("lctrl")
if event.key == pygame.K_RCTRL:
key_pressing.append("rctrl")