-
Notifications
You must be signed in to change notification settings - Fork 1
オプションで壁とアイテムのおおよその数を指定できるようにした #23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
オプションにクソデカ数字を入れられた時の処理を追加する |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
argsで渡される値と実際のマップで得られる状態に乖離があるので
入力された値/4
などでケアしてあげるほうが優しいと思います!
実装上の都合で要件が若干複雑なのでテストコードも書きましょう
値のバリデーションも忘れずに!
src/RandomMapGenerator.py
Outdated
if not os.path.exists(file_path): | ||
os.makedirs(file_path) | ||
with open(f"{file_path}{file_name}.map", "w") as f: | ||
with open(f"{file_path}/{file_name}.map", "w") as f: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
os.path.join()
で統一したい
with open(f"{file_path}/{file_name}.map", "w") as f: | |
with open(os.path.join(file_path,f"{file_name}.map"), "w") as f: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
確かに!
修正します.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
アルゴリズムの関係上, このままの実装だと, argで渡された値と若干の乖離があるのは避けられないことなので諦めます.
ただ, argで渡された値/4
は簡単に実装できると思うのでやってしまいます.
(追加する処理としては, argで渡された値/4
で端数が出た場合は切り捨てして小さいランダムマップの生成処理をする)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
修正しました!
コメントで指摘された点の修正
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
入力値のチェックの部分気になったのでコメントしています
実装面に関しては後は気になるところは無いかも
テストコード書けそうなら書いてほしいです!
block_num = int(args.blockNum) if args.blockNum else 9 | ||
except ValueError: | ||
print( | ||
"error: Expected an integer for option '--blockNum', " | ||
"but received a different type." | ||
) | ||
print(f"blockNum type: {type(args.blockNum)}") | ||
if not (0 <= block_num <= 40): | ||
raise ValueError("Out of range") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
範囲外の値を入力されたときはvalidation checkで例外より先にケアできると良いかなと思いました!
https://qiita.com/LyricalMaestro0/items/80e6ec2d58c828977172
https://qiita.com/TakamiChie/items/95ab9ae401879de90231
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
テストコードの件把握しました.
ただ, まとめて他のissueで解決した方がいい気がするので, 特に何もなければ, 別のissueで対応します.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
特段の事情が無い限りは変更したPRでテストもまとめる方が良いと思います
- 実装が適切か否か,テスト条件という成功/失敗のはっきりした基準でわかること
- テスト条件で考慮漏れがある → 実装でも考慮漏れがある可能性を発見できること
このあたりが挙げられます
SSIA
オプションで指定された数で分割された小さいマップのアイテムと壁の数を指定するため, 出来上がるマップの総数は約4倍の数になる