Iyarr
@Iyarr

Are you sure you want to delete the question?

Leaving a resolved question undeleted may help others!

可読性を評価してほしいです

プログラム作ったけど、、、

 普通の式から逆ポーランド記法に変換するプログラムを組んだのですが、今後のために可読性を上げるためのアドバイスが欲しいです。プログラムの内容は難しいのですが、率直な意見をいただきたいです。もうすぐ複数人との共同開発を始めてみたいと思っていて可読性について少しでも知っておきたいです。
ソースコードは下のURLです。
https://github.com/ERIN-IARU/Qiita/blob/793b0e80753fdb27173dcb387e45b161eacfdf24/porandmake.py

ソースコードの解説については長くなるので記事の方で投稿しようと思うのですが、可読性を考えて作るうえで種類に関係なく使わない方がいい関数や、わかりにくい関数などを聞きたいです。いただいた意見を考慮して修正していきたいと思っています。
部分的なものでも構いませんのでよろしくお願いします。

0

具体的な内容については置いておいて、ぱっと
拝見した状態で、以下3点が気になりました。

(1) 9行目について、他のコメント行は「#+全角スペース1個」であるのに対し、
 「#+半角空スペース3個」であること
(2) 14行目の空コメント
(3) 18、36、51行目の「処理〇」はコメントからではどんな処理なのか読み取れないこと

また、コードとは別の補足ですが、共同開発で
GitHubを利用するのであれば、コミットメッセージのルールも
厳密に決めておいた方が良いかもしれません。

2Like

@syutorum001 指摘ありがとうございます。自分のプログラムの方でコメントを
# 不要な括弧の除去
# 演算子の検出 +、ー
# 演算子の検出 *
に変更し、いらないコメントは消しました。
コミットメッセージのルールは全く意識したことなかったので参考にしていこうと思います。

0Like

メソッド名について

porandmakeからは「ポーランド作成」としか読み取れないので、「逆ポーランド記法への変換」を表す名称が良いと思います。
そうすればコメントでの説明も不要です。
「何から変換するのか」という点も分かると良いですね。

内部の処理ブロックについて

いくつかの工程に分かれるなら、それをメソッドに分割するのも良いと思います。
そうすることで全体の見通しが良くなります。

また「演算子の検出」とするのであれば、それ以外の処理(変換処理など)を行うのは混乱を招きます。
説明と事実が不一致だと読み手が困ることになります。

コミットメッセージについて

このような規約もあります。

どのようなルールを採用するかは状況によりますが、長く開発する場合は履歴も重要な情報です。

3Like

syntaxgetレベルの関数を許容しているので処理Noの区切りで関数化しても良いと思います。
カウンタの再利用しなければならないほどメモリリソースに困っているわけではないと思うので。

0Like

関数名やメソッド名は動詞で始めるといいですよ。処理を行わせるので命令形(動詞の原形)で。
あるいは、取得情報の名詞や命令の過去分詞でもいいです。len関数やsorted関数のように。

len(formula) ⇒ length of formula: 数式の長さ
sorted(data) ⇒ ソートされたデータ

convert_to_rpn(formula) ⇒ 数式を逆ポーランド記法に変換せよ
rpn(formula) ⇒ 数式の逆ポーランド記法、逆ポーランド記法された数式
逆ポーランド記法の英語は Reverse Polish Notation, 略称 RPN

    syntax = syntax.replace(' ','')
    syntax = syntax.replace(' ','*')

最初の replace で空白が無くなるので、2つ目の replaceは意味がないですね。
syntax は RPN構文 とか RPN文法 のようなときに使うので、数式なら formula の方がいいかと思います。

0Like

変数名・関数名についていうと、入力の読み取りから変換まで行う関数が syntaxget() なのは役割を想像しづらいです。読み取りと下処理だけ行うようにして以下のように分けるとよさそうです。

# キーボードからの入力
def input_polish_notation():
    expr = input("式を入力してください\n")
    expr = expr.replace(' ','')
    expr = expr.replace(' ','*')
    return expr

#   逆ポーランド記法への変換
def to_reverse_polish_notation(expr):
    ...

expr = input_polish_notation()
expr = to_reverse_polish_notation(expr)
print(expr)

変数名は syntax (構文)より expr(式; expression の略)が適切だと思いました。ポーランドは porand ではなく poland で、ポーランド記法は polish notation といいます。


    syntax = syntax.replace(' ','')
    syntax = syntax.replace(' ','*')

は1行目ですべての半角スペースを削除しているので2行目の変換が無意味になっています。

1Like
  • ひとつの関数・メソッドは1画面で処理全体が見渡せるように25行以内に機能分割
  • リーダブルコードにならって、変数名・関数名に情報を持たせる

ついでに、*, / にも対応してみました。

def expr():
    """キーボードから数式入力、空白削除"""
    return input("数式を入力してください: ").replace(' ', '')


def rpn(expr):  # Reverse Polish Notation
    """数式exprを逆ポーランド記法に変換"""
    while expr.startswith('(') and expr.endswith(')'):
        expr = expr[1:-1]
    return add_sub(expr) or mul_div(expr) or expr


def add_sub(expr):
    nest = 0
    for i, c in enumerate(expr):
        if c == '(':
            nest += 1
        elif c == ')':
            nest -= 1
        elif nest == 0 and c in '+-':
            return [rpn(expr[:i]), rpn(expr[i+1:]), c]
    return None


def mul_div(expr):
    nest = 0
    operands = 0
    for i, c in enumerate(expr):
        if c == '(':
            nest += 1
        elif c == ')':
            nest -= 1
        elif nest == 0 and c in '*/':
            return [rpn(expr[:i]), rpn(expr[i+1:]), c]
        if nest == 0 and end_of_operand(expr, i):
            operands += 1
            if operands == 1:
                separation = i + 1
            elif operands == 2:
                return [rpn(expr[:separation]), rpn(expr[separation:]), '*']
    return None


def end_of_operand(expr, i):
    return not (len(number := expr[i:i+2]) == 2 and number.isdigit())


print(rpn(expr()))
1Like
This answer has been deleted for violation of our Terms of Service.

Your answer might help someone💌