はじめに
Qiita初投稿です。
数か月前に自分が書いたJavaScript(厳密にはGasで作成したWebアプリ)を修正をすることになり、久々にコードを見てみたら、何を意図してるのか全然分からない・・・という状態に陥りました。
自分で書いたにも関わらず・・・
読んで理解するだけで凄く時間がかかってしまい、可読性って大事だな、ってことを身をもって実感したため、己への戒めと備忘録のため記載します。
読めなくて困った所はたくさんあったのですが、今回は、タイトルにある「奇数位置・偶数位置の要素を抽出」の部分と、あと、その後、奇数位置の合計、偶数位置の合計を算出する必要があったため、その部分を。
文章もコードも拙い点があるかと思いますが、誤りやもっと良い方法等、お気づきの点がありましたら、ご指摘頂ければ幸いです。
抽出元のデータ（例）
values = [
[0, 1, 2, 3, 4, 5],
[2, 4, 2, 4, 2, 4],
[8, 9, 8, 9, 8, 9],
]
＜補足＞
上記はスプレッドシートからセルの範囲を指定してgetValues()で取得したデータで、下記を意味しています。
・奇数列（配列の位置としては0,2,4）：販売数の見積
・偶数列（配列の位置としては1,3,5）：実際の販売数
何でこんな扱いづらい形でスプレッドシートに登録してるんだ、というツッコミや、あと、そもそもスプレッドシートで別セルに抽出できるでしょ、という話は本当にごもっともなのですが（そして僕もそう思うのですが・・・）、それはそれとして、ここから、行別に、見積のみ、実際の販売数のみ、を分別する必要がありました。
そして、分別抽出したデータを使って諸々処理をした後に、最後に、行別に、
「見積の合計」と「実際の販売数の合計」とを算出する必要がありました。
なお、実際のデータは、約1000行・200列程度になります。
最初の状態
for(let i=0, len1=values.length; i<len1; i++){
let arr1 = [];
let arr2 = [];
for(let j=0, len2=values[i].length; j<len2; j++){
let n = values[i][j];
switch(true){
case j%2 === 1 :
arr1.push(n);
break;
default :
arr2.push(n);
break;
}
}
sum1 = sumArr(arr1);
sum2 = sumArr(arr2);
//もろもろの処理が続く
}
//もろもろ他の色んな関数があって
function sumArr(arr){
let result = 0;
for(i=0, len=arr.length; i<len; i++){
result += arr[i];
}
return result;
}
試しにコンソールに出力して確認してみると
console.log(arr1);
console.log(arr2);
console.log(sum1);
console.log(sum2);
/****************************
以下、コンソールに出力された内容
****************************/
[1, 3, 5]
[0, 2, 4]
9
6
[4, 4, 4]
[2, 2, 2]
12
6
[9, 9, 9]
[8, 8, 8]
27
24
意図した通りに出力してくれました。
それはそうなんですよね。
実際、アプリも意図した通りに動いてるので。
・・・が、コードを読んでても全く意味が分からない・・・。
数か月前の自分に言ってあげたい。
後で見る人のことを考えようよ、と。
最初の状態で困ったこと
1.変数が何を意味してるのか全く分からない
ここだけを取り出して読む分には構わないのですが、実際にはもっと長いコード（確か当初は500行くらいでした）の一部になります。
arr1、arr2、sum1、sum2らが何を意味してるのかが分からずに大変困りました。
2.無駄に冗長
GASとかJavaScriptを触り始めた当初で、愚直にforをネストして一つずつ処理したり、たった2パターンの条件分岐にswichを使ってたりと、微笑ましい。ifでしょ。if。2パターンなんだから。三項演算子でこうでしょ。
(j % 2 === 1) ? arr1.push(values[i][j]) : arr2.push(values[i][j]);
jsでswitch(true) という書き方が望ましくないという意見があることも当時は知りませんでした。（個人的には、大変便利だと今も思っていますが）
mapとかfilterとかも知らなかったんだと思います、きっと。
「500行くらい」と上述しましたが、そりゃあ、長くもなるよと。
どこに何が書いてあるかを探すのにやたらと時間がかかりました。
方針と修正内容
ということで、以下の方針でリファクタリングを。
1.読んで意味の分かる変数名、関数名に！（←多少長くても意味が分かる名称で）
2.短く書ける部分は短く（←ただし可読性を損なわない範囲で）
ひとまず、以下の形に。
values.forEach(predictionsAndSales => {
let predictions = getPredictions(predictionsAndSales);
let sales = getSales(predictionsAndSales);
let [predictionsTotal, salesTotal] = [getTotalValue(predictions), getTotalValue(sales)];
//もろもろの処理が続く
});
//もろもろ他の色んな関数があって
/**********************************
渡された配列の偶数位置の値を返す
**********************************/
function getPredictions(arr){
return arr.filter((value, i) => i % 2 === 0);
}
/**********************************
渡された配列の奇数位置の値を返す
**********************************/
function getSales(arr){
return arr.filter((value, i) => i % 2 === 1);
}
/**********************************
渡された配列の要素の合計値を返す
**********************************/
function getTotalValue(arr){
return arr.reduce((pre, cur) => pre + cur);
}
だいぶスッキリしましたかね。
多重代入、アロー関数と配列の反復メソッド、とっても便利。
「predictions」が本当に「見積」を意味するかどうかはちょっと怪しいとは思ったのですが、google翻訳を信じる方向で。
合計値の変数名に関しては、おそらく英語として正しいのは「TotalePredictions」「TotaleSales」だと思うのですが、今回のように見直しが入った時の確認では「『Predictions』なのか『Sales』なのか、どっちのTotaleなんだ！」ということの方が大事かな、と思ったので、Totalを後ろにする形で記述。
どうせ拘っても正しい英語なんて書けないですし・・・
あと、関数名では、「見積を取得！」「売上を取得！」という目的は分かるのですが、意図？処理してる内容？（奇数位置、偶数位置の値を返す）ということは分からないので、そちらはコメントに記載。
こういうコメントが本当に必要かどうかは、ちょっと自信がありませんが・・・
今後見直しが入った際、不要と判断したらその時に消す方向で、ひとまず、記載。
でも、まだ気になる点が
具体的には、下記です。
1.関数getPredictions()とgetSales()、殆ど同じ内容なので、まとめたい
2.遠い場所に記載した関数を確認するのが面倒（実際にはもっと長いコードなので）
ということで、上記の問題解決のために修正した結果が、下記です。
values.forEach(predictionsAndSales => {
let selectValuesByPosition = (rem) => predictionsAndSales.filter((n, i) => i%2 === rem);
let [predictions, sales] = [selectValuesByPosition(0), selectValuesByPosition(1)];
let sumElemesOfArr = (arr) => arr.reduce((pre, cur) => pre + cur);
let [predictionsTotal, salesTotal] = [sumElemesOfArr(predictions), sumElemesOfArr(sales)];
//もろもろの処理が続く
});
関数は、関数リテラルにして実際に使う場所の直前に置きました。
（関数のコメントが、とか上で書いてましたが、近くに置いたので結局消しました）
また、奇数位置／偶数位置の要素を取得する関数を一つにまとめ、最初から2で割った余りである「0」or「1」渡して、渡した数字でフィルターをかけるようにしました。
ん～。
確かに、目的は達成したとは思うのですが、ただ、「selectValuesByPosition()」という名称の関数、つまり、「位置で値を選ぶ！」という名称の関数に、「0」とか「1」を渡して奇数位置、偶数位置の値を取得するのはどうなんでしょうか・・・？
直ぐ上にあるので確かめれば分かるのですが、普通に考えたら、0番目、1番目、の要素を取得してきそうですよね。あと、selectValuesByPosition(n)と好きに数字を入れて、n番目の要素を返してくれそうな感じがします。
変数名を工夫して、
「2で割った時の余りが0だったら偶数位置、1だったら奇数位置の要素を返すから、偶数位置を取得したかったら0、奇数位置を取得したかったら1を入れてね！」
ということが分かるようにできたらいいのですが、さすがに全部盛り込んだら変数名が長すぎて可読性が落ちそうですよね・・・
あと、別件で、今更ですが、そもそも先頭の「values」って何なのよ？
要はGasを通してスプレッドシートから取得したデータを振り分けてるんですよね？
ということが全く分からない・・・
結果、再度リファクタリングを
と大きく書きましたが、そんなに大きくは変えてません。
predictionsAndSalesDataFromSs.forEach(predictionsAndSales => {
let selectValuesByPosition = (rem) => predictionsAndSales.filter((n, i) => i%2 === rem);
let at = {odd: 0, even: 1};
let [predictions, sales] = [selectValuesByPosition(at.odd) , selectValuesByPosition(at.even)];
let sumElemesOfArr = (arr) => arr.reduce((pre, cur) => pre + cur);
let [predictionsTotal, salesTotal] = [sumElemesOfArr(predictions), sumElemesOfArr(sales)];
});
まず、先頭の「values」という変数名を「predictionsAndSalesDataFromSs」に変更しました。少し長い変数名ですが、意味が分かるので、まぁ、大丈夫かな、と。
そして問題の「奇数位置／偶数位置」の件は「let at = {odd: 0, even: 1};」 という一行を追加し、selectValuesByPosition()を実行する際に「at.odd」／「at.even」で奇数偶数を指定して、結果、at.oddであれば1を、at.evenであれば0を渡すようにしました。
こうしても、結局、selectValuesByPosition()に直接「0」や「1」を渡すことはできてしまうので、意味があるかどうかは正直分からないのですが、後で見た時に「at.odd」と書いてあれば奇数位置というのが分かりやすいかな、と思いまして。
最初は、下記のように、
let selectValuesByPositionAt = (oddOrEven) => predictionsAndSales.filter((n, i) => {
rem = {'odd': 1, 'even': 0};
if(i%2 === rem[oddOrEven]) {return n ;};
});
という形にして、selectValuesByPositionAt('odd')／selectValuesByPositionAt('even')という風にするのも考えたのですが、関数リテラルが長くなるのも読みにくいし、関数文にして別位置に記載するのも読みにくくなるので、今回の形にしました。
雑感
今は、コードを修正した直後なので、自分的には分かりやすくなったと感じているのですが、他の方が読んでどうなのか、また、今回のように、数か月後の自分が読んでどうなのかは正直分からない、というのが本音です。
また修正を依頼された際は、その時の自分がどう思うかをしっかり振り返りたいと思います。
しかし、名前の付け方って本当に難しい・・・
Google翻訳とたくさん格闘しました。
英語に強い方だと、バチっと短くて分かりやすい感じでキマるのかな・・・？
読みかけで途中で放り出したリーダブルコードを、ちゃんと読もうと肝に銘じました。