PYTHONソースファイルのリファクタリング

Question

仕事でPYTHON-FLASKを使ったアプリケーションを作っています。

ほとんどの処理をapp.pyというファイルに入れています。
理由としては、PYTHON自体がIMPORT等を使っても、動作としては
全部読み込んで動くスクリプトなので、特に分割するメリットを感じなかったためです。

ところが、直近になって私本人の移動の話があり、
この理解しにくい、ソースコードを別担当者に引き継ぐ必要が出てきました。

まずはコード自体が長すぎるために、これを短く書く検討を始めています。

そこで添付のようなコードの場合は、どこを、どのように切れば
このようなコード縮小が可能でしょうか？

いまだ未熟者のため、リファクタリングで動かなくなるリスクを懸念して
手が付けられていない状況です。

参考のソースファイルを下記に示します。

実際には、これらの5倍ぐらいのコードがあります。
忌憚のないご意見を頂けましたら幸いです。

#app.py

import os
import sys
import io
import time
import numpy as np
import pandas as pd
import cv2
from flask import Flask, render_template, request, redirect, url_for, send_from_directory, session, flash, make_response
import pdb
import PyPDF2
import random
import json
from flask_sqlalchemy import SQLAlchemy
from sqlalchemy import create_engine
from sqlalchemy.orm import sessionmaker
import sqlite3
from datetime import datetime
from werkzeug.utils import secure_filename
import Fxx_COUNT as fsc
import secrets
import logging
import string


LOGFILE = "LOGFILE.log"

app = Flask(__name__)

app.secret_key = 'any random string'
app.logger.setLevel(logging.DEBUG)
fh = logging.FileHandler(LOGFILE)
fh.setLevel(logging.DEBUG)
app.logger.addHandler(fh)

UPLOAD_FOLDER = './uploads/'
STRAGE_FOLDER = './strages/'
TEMP_FOLDER = './temp/'
STATIC_FOLDER = './static/'

ALLOWED_EXTENSIONS = set(['png', 'jpg', 'PNG', 'JPG', 'pdf'])
IMAGE_WIDTH = 640
app.config['SQLALCHEMY_DATABASE_URI'] = 'sqlite:///members.db'
app.config['UPLOAD_FOLDER'] = UPLOAD_FOLDER  # MCOのオリジナルをアップロードするフォルダ
app.config['STRAGE_FOLDER'] = STRAGE_FOLDER  # 処理済ファイルをアップロードするフォルダ
app.config['TEMP_FOLDER'] = TEMP_FOLDER  # 処理済ファイルをアップロードするフォルダ
app.config['STATIC_FOLDER'] = STATIC_FOLDER  # 処理済ファイルをアップロードするフォルダ
app.config['SECRET_KEY'] = os.urandom(24)
PATTERN_FILE = "Fxx.png"

#db = SQLAlchemy()
# db.init_app(app)


def allowed_file(filename):
    return '.' in filename and \
        filename.rsplit('.', 1)[1] in ALLOWED_EXTENSIONS

# members.dbから、CUSTOMERSを読み込んで、VARへ挿入する
@app.route('/customer')
def customer():
    conn = sqlite3.connect(app.config['DATABASE'])
    cur = conn.cursor()
    cur.execute("SELECT * FROM customers")
    var = cur.fetchall()
    conn.close()
    return render_template('customer.html', var=var)

# items sign up
@app.route('/customer_signed_up', methods=['GET', 'POST'])
def customer_signed_up():
    # pdb.set_trace()
    session['username'] = request.form['username']
    password = request.form['password']
    conn = sqlite3.connect(app.config['DATABASE'])
    cur = conn.cursor()
    cur.execute("INSERT INTO customers(username,password) VALUES (?,?)",
                (session['username'], password))
    conn.commit()
    conn.close()
    return redirect(url_for('logout'))

# items delete
@app.route('/customer_delete', methods=['GET', 'POST'])
def customer_delete():
    session['username'] = request.form['username']
    password = request.form['password']
    conn = sqlite3.connect(app.config['DATABASE'])
    cur = conn.cursor()
    cur.execute("delete from customers where username=?", (session['username'],))
    cur.execute("delete from response where username=?", (session['username'],))
    cur.execute("delete from response2 where username=?", (session['username'],))
    conn.commit()
    conn.close()
    return redirect(url_for('logout'))

# ログイン処理　←　SESSION変数にログインしたユーザーの名前が入る
@app.route('/customer_logged_in', methods=['GET', 'POST'])
def customer_logged_in():
    session['username'] = request.form['username']
    dir1=os.path.join(app.config['UPLOAD_FOLDER'],session['username'])
    if not os.path.exists(dir1):
        os.mkdir(dir1)

    app.logger.info('%s logged in successfully', session['username'])
    return redirect(url_for('homepage_customer'))

# メインページへの誘導のため、データベースのテーブルを呼び出す
@app.route('/')
@app.route('/homepage_customer')
def homepage_customer():
    if 'username' in session:
        conn = sqlite3.connect(app.config['DATABASE'])
        cur = conn.cursor()
        cur.execute("SELECT * FROM response WHERE username=? order by url asc" ,
                    (session['username'],))
        response = cur.fetchall()
        cur.execute("SELECT * FROM response2 WHERE username=?",
                    (session['username'],))
        response2 = cur.fetchall()
        conn.close()

        image = {}
        my_images = []  # イメージ画像の辞書

        for id, name, file1, number, sender in response:
            if file1.endswith('.png'):
                image['url'] = file1
                image['id']=id
                image['message']=number
                if sender == 0:
                    sender = 0  
                elif not sender:
                    sender = -1
                image['sender']=sender
                my_images.append(image.copy())

        pimage = {}
        p_images = []

        for id,name, file2, sel ,dum,dum in response2:
            if file2.endswith('.png'):
                pimage['id'] = id
                pimage['url'] = file2
                p_images.append(pimage.copy())

        return render_template('index.html', items=my_images, p_items=p_images, USER=session['username'])
    return redirect('customer')

@app.route('/logout')
def logout():
    # remove the username from the session if its there
    session.pop('username', None)
    return redirect('/')

# サーバーへPDFをアップロードする
@app.route('/send', methods=['GET', 'POST'])
def send():
    if request.method == 'POST':
        img_file = request.files['img_file']
        if img_file and img_file.filename.endswith('.pdf'):
            #pdb.set_trace()
            saveFileName = path_( app.config['UPLOAD_FOLDER'],session['username'], secure_filename(img_file.filename))     
            if os.path.isfile(saveFileName):
                flash('ファイル名が重複しています。重複しないページのみを更新します。')
            else:
                img_file.save(saveFileName)
        else:
            flash('pdfファイルではありません')
            saveFileName = ""
        return render_template('index.html', upload_img_url=saveFileName)
    else:
        return redirect('/')

def path_(a,b,c):
    d= os.path.join(a,b)
    return os.path.join(d,c)
    
# サーバーへPattern をアップロードする
@app.route('/pattern', methods=['GET', 'POST'])
def pattern():
    if request.method == 'POST':
        img_file = request.files['img_file']
        if img_file and img_file.filename.endswith('.png'):
            saveFileName = os.path.join(
                app.config['UPLOAD_FOLDER'], datetime.now().strftime("%Y%m%d_%H%M%S_")+secure_filename(img_file.filename))
            if os.path.isfile(saveFileName):
                flash('ファイル名が重複しています')
                saveFileName = ""
            else:
                img_file.save(saveFileName)
                conn = sqlite3.connect(app.config['DATABASE'])
                cur = conn.cursor()
                cur.execute("INSERT INTO response2(username,url,choose,circle,line) VALUES (?,?,?,?,?)",
                            (session['username'], saveFileName, 1 , 10 , 6 ))
                conn.commit()
                conn.close()
        else:
            flash('pngファイルではありません')
            saveFileName = ""
        return redirect('/')
    else:
        return redirect('/')



中略

##python app nonx      --->  Format for Non-X without Fxx/Sxx
##python app           --->  Format for X with Fxx/Sxx

if __name__ == '__main__':
    app.config['USE_FxxSxx_NOTATION'] =  True
    app.config['PORT']='8080'
    app.config['DATABASE']='members.db'
    app.config['ADMIN_PASS']="xxxx"  ## X-USER

    if len(sys.argv)>=2:
        if sys.argv[1]=="nonx":
            app.config['USE_FxxSxx_NOTATION'] = False
            app.config['PORT'] = '8888'
            app.config['DATABASE']='members2.db'
            app.config['ADMIN_PASS']="xxxx"  ## NONX-USER
    app.run('localhost', port=app.config['PORT'] ,debug=True)

Comments

[dodox86]

「引き継ぐ」と言うのは、質問文中の（yuujiMotokiさんが書かれたと思しき）ご提示のコードを、別の人に渡す、と言うことでしょうか。

[siruku6]

リファクタリングで動作が変わる恐れがあるということであれば、自分でリファクタリングするよりは、unittestだけ実装してあげてリファクタリングは引き継いだ相手にしてもらった方が安全でかつ、将来にわたってプラスになるような気がしました。

実際にリファクタリングで動かなくなるリスクも大幅に減らせます。

ただし、周囲の理解が得られるかはわかりません、、、

スマホしか現在使えず、ソース全体を見渡すのが難しいのでひとまずこれだけお伝えしてみます。

Answer 1

ご提示のコードは225行ありました。
この5倍だとしても1125行でそれほど多いとは思えません。
リファクタリングして余計わからなくなる可能性の方が高いです。(分割すると増えますし)

それよりも、移動して引き継ぐなら引き継ぎ資料を作った方が良いです。
もしも設計書がないならそれも作りましょう。
私が引き継ぐ場合、設計書の名前はなんでもいいですが、以下を記載してほしいです。
設計書は自分のためにもなりますのであって損はありません。

・そのアプリが何をするのかの概要
・そのアプリにある機能一覧
・ファイル一覧
何というファイルがあるというもの
例えば、LOGFILE.logはどこにできるとか。
・機能毎の関連ファイル(関数)一覧
機能が使っているファイルの一覧
ファイル一覧とかぶるが、あると便利
・機能毎のフロー
何にアクセスするとどの関数が呼ばれるなど
・機能毎のInput/Output
何を受け取って、何を出力するかなど
受け取る方法(GetとかPOSTとか、どこかのフォルダの●○.txtを読むとか)
出力する方法(●にoutput.txtができますとか、DBのxxtableに1行追加(更新)されますとか)
・使っているpythonバージョン
・使っているflaskバージョン
・使っているmodule一覧、およびそれを使っている機能
・使っているDBの名前
・DBのCreate文
・最初の環境を作るのに必要なInsert文
できれば、本番環境の最初とテスト環境の最初にできるinsertがあるとうれしい。
・logファイルのローテートの設定など
・sqliteっぽいですが、ファイルが無い場合どうするのか
・このアプリを動かすOS
・OSの設定
・使っているWebサーバの名前(Apacheとかnginxとか)と設定(configの中身と意味)
・wsgiではなくgunicornとか使っているならその設定と、接続方法
Apacheを起動するとアプリも起動しますとか、アプリは別に動いているので起動が必要ですとか
簡単に言うとWebアプリを動かすためのサーバ構築手順書ですね
メモリ量とか、必要なアクセス権(/var/www/hogehoge/に777が必要ですみたいな)
・E-R図

他にも
・(会社の他の人からいままで受けた)要望
・要望の対応状況と未対応ならその理由
・(会社の別の人が使っているなら)更新するための手順
全社に連絡して、18時以降じゃないと更新できませんとかあるかも？
・サーバの管理者とかサーバ名とかもあれば
必要ならIDとパスワードも

Comments

[dodox86]

保守する側の立場からも、「リファクタリング」の観点からも、高評価させていただきました。

> 添付のようなコードの場合は、どこを、どのように切ればこのようなコード縮小が可能でしょうか？
と言うのがご質問の本筋なので回答はしませんでしたが、本回答のFiroProchainezoさんと、コメントでのsiruku6さんに同意します。
引継ぎを受け、保守を任される立場からすると現行の動いているコードと、（リファクタリングしたと称する）新しいコードがあり、動きが違った場合、どちらが正しいのか分かりません。それを担保するのがunittest(単体テスト)コードのはずですが、それができない場合はドキュメントで「何をしているのか（求めているのか）」を明示しておかないと、保守ができません。「リファクタリング」とは一般的に外側の挙動はまったく同じに中身を改良することを指しますが、単体テストが無い場合は保証できないと思います。

Answer 2

FiroProchainezoさんの回答ともかぶりますが、1000行程度であれば、ファイル分割はしない方がたぶん親切です。分割しない設計で作ってしまったものをばらして訳わからなくなるよりは良いでしょう。必要なら引き継いだ人がやります。

たぶん引き継ぎ資料を頑張って作ったほうが正しいのでコードに関しては最小限のことだけをする方針にして、PEP8（公式コーディング規約）に準拠させて、あまり使ってない便利関数とかは消して（path_とか要らないと思うんですが）可能ならロジックを整理して（本当にすぐできることだけです。if文が多いので何箇所かは整理できるでしょう。たとえばhomepage_customerはnot inで判定した方がきれいじゃん、とか）、あとはdocstringとかコメントとか型ヒントとかを適宜入れておくという感じでいいのでは。一日かからないと思います。

Answer 3

まだ引継ぎができていませんが、ソースファイルのまま
引き渡すことにいたしました。

逆に、へたにオブジェクト思考かしていないので、
可読性はこのままにしようと思いました。

Comments

[Zuishin]

最低の自己解決。

長文のコードを読ませておいて長期にわたって放置した挙げ句、ついた回答に何も触れず、質問自体をひっくり返している。引き継ぎ資料や公式ドキュメントをどのようにしたかの報告もない。

つまり、報告もなく、連絡もせず、相談はひっくり返している。

Answer 4

公式ドキュメントを参考にしてみては？
より大きなアプリケーション — Flask v0.5.1 documentation
https://a2c.bitbucket.io/flask/patterns/packages.html

他には「flask ファイル構成」とかでググるといろいろ見つかります。