見出し画像

リファクタリング的な作業（１）

LiuK

（Python学習初心者の試行錯誤・備忘録です）
半月ほど前に書いた

が一応動いたことで、ちょっとした高揚感(笑)にしばらく浸っていました。少し時間が経って、冷静になってみると、コードの中身がひどいもんなので、初心者なりに「リファクタリング」的な作業をしてみようと思います。
test.py

import csv
import sqlite3
import TkEasyGUI as eg
from myspeech_lib import myspeech
mydb = "mydb.sqlite3"

#メイン画面の作成
def make_main():
    lfont = ('標楷體', 100) #大きい表示用フォント
    sfont = ('Arial', 50) #小さい表示用フォント
    btfont= ('Arial', 20) #ボタン表示用フォント

    #表示サンプル
    myword = '仮置き'; pinyin = 'karioki'
    menu_def = [
        ['ファイル',['読み込み',['CSV::LoadCSV'],  '---', 'Exit']],
    ]

    #メイン画面のレイアウト
    layout_main =[
        [eg.Menu(menu_def)],
        [eg.Combo(values=["zh-TW","ja","en-US"],default_value="zh-TW",key="-LANG-")],
        [eg.Button("発声", font = btfont, key ="btn_speak"),
        eg.Button("連想", font = btfont, key ="btn_show_pinyin"),
        eg.Button("覚えた", font = btfont, key ="btn_OK"),
        eg.Button("まだ", font = btfont, key ="btn_NG")],
        [eg.Text('サマリー仮置き',
        key="summary",font=('Arial', 15))],
        [eg.Text(myword, font = lfont, key="disp_main")],
        [eg.Text(pinyin, font = sfont, key="disp_sub")],
    ]
    return eg.Window("単語暗記 for Windows(Python版)", layout=layout_main, finalize=True)

#優先データを抽出
def flashcard():
    con = sqlite3.connect(mydb)
    cur = con.cursor()

    def getrow(SQLstr:str):
        cur.execute(SQLstr)
        return cur.fetchone()
    
    totalcount = getrow("SELECT COUNT(*) FROM t_shengci")[0]
    level0count = getrow("SELECT COUNT(*) FROM t_shengci WHERE level = 0")[0]
    level1count = getrow("SELECT COUNT(*) FROM t_shengci WHERE level = 1")[0]
    level2count = getrow("SELECT COUNT(*) FROM t_shengci WHERE level = 2")[0]
    level3count = getrow("SELECT COUNT(*) FROM t_shengci WHERE level = 3")[0]
    tempid, temphanzi, temppinyin, templevel, temptimestamp \
        = getrow("SELECT id, hanzi, pinyin, level, timestamp FROM t_shengci \
                 ORDER BY level ASC, timestamp ASC")
    resdict={'id':tempid,'hanzi':temphanzi, 'pinyin':temppinyin, 'level':templevel, \
             'timestamp':temptimestamp, 'totalcount':totalcount, 'level0count':level0count, \
            'level1count':level1count,'level2count':level2count,'level3count':level3count}

    con.close()
    return(resdict)

#コンテンツ更新
def refresh_content(tempdic):
    window["disp_main"].update(tempdic["hanzi"])
    window["disp_sub"].update("")
    window["summary"].update("レベル0:{}個 レベル1:{}個 レベル2:{}個 レベル3:{}個 合計 {}個"\
                            .format(tempdic["level0count"],tempdic["level1count"],tempdic["level2count"],\
                                    tempdic["level3count"],tempdic["totalcount"]))

#学習履歴更新
def inc_level(): #覚えていた場合 levelをプラス1
    con = sqlite3.connect(mydb)
    con.execute("UPDATE t_shengci SET level = {},timestamp = CURRENT_TIMESTAMP WHERE id = {} "\
                .format(tempdic["level"]+1,tempdic["id"]))
    con.commit()
    con.close()

def zero_level(): #忘れていた場合、levelを0
    con = sqlite3.connect(mydb)
    con.execute("UPDATE t_shengci SET level = {},timestamp = CURRENT_TIMESTAMP WHERE id = {} "\
                .format(0,tempdic["id"]))
    con.commit()
    con.close()

#表示作成
window = make_main()
#最初の読み込み・表示
tempdic = flashcard()
refresh_content(tempdic)

while True:
    event, value = window.read()
    
    if event == eg.WIN_CLOSED or event == "Exit": #終わるとき
        break

    elif event == "LoadCSV": #学習教材をCSVで読み込む処理、古いのは削除
        mycsv = eg.popup_get_file("CSVファイルを選択",
                                file_types=(("All Files", "*.*"), ("CSV Files", "*.csv"), ))        
        mydb = "mydb.sqlite3"
        con =sqlite3.connect(mydb)
        with open(mycsv, newline='', encoding='utf-8') as csvfile:
            con = sqlite3.connect(mydb)
            con.execute("DELETE FROM t_shengci")
            csvreader = csv.reader(csvfile, delimiter=',')
            for row in csvreader:
                con.execute("INSERT INTO t_shengci(hanzi, pinyin) VALUES('{}','{}')" 
                            .format(row[0],row[1]))
            con.commit()
        con.close()
        tempdic = flashcard()
        refresh_content(tempdic)

    elif event =="btn_speak":   #読み上げ         
        myspeech(value["-LANG-"],tempdic["hanzi"])

    elif event =="btn_show_pinyin":  #ピンイン表示
        window["disp_sub"].update(tempdic["pinyin"])

    elif event =="btn_OK": #記憶定着OKなら。Levelを一つ上げ、タイムスタンプ更新,表示データ更新
        inc_level()
        tempdic = flashcard()
        refresh_content(tempdic)
    
    elif event =="btn_NG": #記憶定着NGなら。Levelを0に戻し、タイムスタンプ更新,表示データ更新
        zero_level()
        tempdic = flashcard()
        refresh_content(tempdic)

window.close()

このメインのコードと、読み上げ用のmyspeech.pyというコードがある。
実行すると

と単語暗記アプリが起動すると言うものです。

重複削除

・まず、mydb = "mydb.sqlite3" というのが複数回出てきますが後のは重複なので不要⇒削除。（変数のスコープの勘違いから複数書いていました）

with文使う

con = sqlite3.connect(mydb)
con.execute(”SQL”)
con.commit()
con.close()

の記述を、

with sqlite3.connect(mydb) as con
    con.execute("SQL")
    con.commit()

のような書き方に変えてcon.close()を無くす。

プレースホルダにする

ここを読むと、Python の文字列操作を使用してクエリを組み立てるのはSQLインジェクションの脆弱性につながるのでよくないそうです。

    with sqlite3.connect(mydb) as con:
        con.execute("UPDATE t_shengci SET level = {},timestamp = CURRENT_TIMESTAMP WHERE id = {} "\
                    .format(0,tempdic["id"]))
        con.commit()

これを

    with sqlite3.connect(mydb) as con:
        con.execute("UPDATE t_shengci SET level = ?,timestamp = CURRENT_TIMESTAMP WHERE id = ? "\
                    ,(0,tempdic["id"]))
        con.commit()

あと、長くなるSQL文はトリプルクォートを使って、複数行で書いた方が見やすくなりそうです。

    with sqlite3.connect(mydb) as con:
        con.execute("""UPDATE t_shengci 
                    SET level = ?,timestamp = CURRENT_TIMESTAMP 
                    WHERE id = ? """ ,(0,tempdic["id"]))
        con.commit()

もっと大きい問題がある

　・・とこのあたりまで来て、こんな小手先の話ではなく、もっと大きい問題があるなという気がしました。
　DB操作やら、画面制御やら、いろいろな処理を「メイン」のコードの中で書いてしまっているので、見通しが悪いのです。今はたった120行程度とはいえ、もうちょっと機能拡張したら、すぐに手に負えなくなるのは想像できます。どうやって、整理したらいいか

MVCモデルとか？

　ネット上で情報を探していたら、MVCというのが一般的なようです。

（この解釈で良いのかどうかわかりませんが）要はデータベース制御のところと、画面制御のところと、アプリの肝になるロジックのところは切り分けろ、って話なのだろうと解釈しました。
　途中で挫折するかもだけど、試してみます。

この記事が気に入ったらサポートをしてみませんか？