JavaScriptでユーザーの所在地情報を整形するコードのリファクタリング方法

Question

実現したいこと

以下のようにユーザの所在地（都市、国）を整形した文字列に変換したいです。

• 入力されている location_info オブジェクトから、LocalityName（都市）、SubAdministrativeAreaName（都市圏）、AdministrativeAreaName（州）、CountryName（国）を取得し、整形して出力する。
• もし都市名がない場合は都市圏名、さらにない場合は州名、最終的に国名を取得して表示する。
• それらが全てない場合はデフォルトの文字列（例：「Middle-of-Nowhere, Planet Earth」）を表示する。

前提

リーダブルコードを読んで、どうリファクタリングすべきか理解したいと思っているのですが、現時点で以下のコードがよくわかりません。
ユーザの所在地情報は location_info というオブジェクトに格納されており、以下のプロパティを含んでいます。

• LocalityName: 都市名
• SubAdministrativeAreaName: 都市圏名
• AdministrativeAreaName: 州名
• CountryName: 国名

現在、以下のようなコードでユーザの所在地を文字列にしていますが、リファクタリングしてコードをより読みやすくしたいと考えています。

発生している問題・エラーメッセージ

現在、コードが少し冗長で理解しにくい状態です。
コードをリファクタリングして、より簡潔で可読性の高い形にしたいです。

該当のソースコード

var place = location_info["LocalityName"]; // 例 : "Santa Monica"
if (!place) {
    place = location_info["SubAdministrativeAreaName"]; // 例 : "Los Angeles"
    if (!place) {
        place = location_info["AdministrativeAreaName"]; // 例 : "California"
        if (!place) {
            place = "Middle-of-Nowhere";
            if (location_info["CountryName"]) {
                place += ", " + location_info["CountryName"]; // 例 : "USA"
            } else {
                place += ", Planet Earth";
            }
        }
    }
}
return place;

試したこと

リファクタリングを進めるために、以下のコードで変数名を整理しました：

var town = location_info["LocalityName"]; // 例 : "Santa Monica"
var city = location_info["SubAdministrativeAreaName"]; // 例 : "Los Angeles"
var state = location_info["AdministrativeAreaName"]; // 例 : "CA"
var country = location_info["CountryName"]; // 例 : "USA"

ただし、これをどのようにリファクタリングすべきかまだ決められませんでした。

Answer 1

主だった所としては、if文のネストが読みにくいと感じている、ということですかね？

このロジックの場合ですと関数化する必要はありますが、条件を逆にすることで早期returnが可能になります。

const location_info_to_place = (location_info) => {
    if (location_info["LocalityName"]) {
        return location_info["LocalityName"];
    }
    if (location_info["SubAdministrativeAreaName"]) {
        return location_info["SubAdministrativeAreaName"];
    }
    if (location_info["AdministrativeAreaName"]) {
        return location_info["AdministrativeAreaName"];
    }
    return "Middle-of-Nowhere" + (
        location_info["CountryName"]
            ? ", " + location_info["CountryName"]
            : ", Planet Earth"
    );
}

あるいは、JavaScriptだと || 演算子を使うことで最初のtruthyな値を返すという事が出来るので、下記のような書きかたもできます

const localityName = location_info["LocalityName"];
const subAdministrativeAreaName = location_info["SubAdministrativeAreaName"];
const administrativeAreaName = location_info["AdministrativeAreaName"];
const countryLabel = "Middle-of-Nowhere, " + (location_info["CountryName"] || "Planet Earth");

const place = localityName || subAdministrativeAreaName || administrativeAreaName || countryLabel;

Answer 2

まず、普通に列挙すればいいと思います。
冗長というのはそうかもしれませんが、一度書けば十分ですし、それぞれに具体的に関連があるとか順序があるという話でもありませんので、普通に一つずつ同じ方法で同じ分岐を書けばいいです、
else ifでつなげれば実現できます。

次に、要求している仕様とソースコードがそもそも違うように思えるのですが、その場合リファクタリングというか不具合の修正になると思います。

次に、LocalityNameをtownに変換するということをするのであれば、そもそもとしてそのように命名したほうが良いかと思います。表記揺れをあえて作るのがナンセンスかなという。
もしするとしても対応表をオブジェクトで定義すればいいかと思います。一つ一つを変数で取り出すのは別にjavascriptなら分割代入を使ってもよいでしょう。

次に、単にfalseと判定されればよく厳密比較しないなどでいえば&&でつなげることで最初にtrueになった文字列を取得するなどというテクニックを使うことも検討できますし、
if elseについてもブロック(複文)を使用しないなどのテクニックが使えます。
(個人的にもっともよいのは三項演算子での:後ろ改行)
これら考慮もできますがそこまで行くと無法状態になるかと思います。

次にリーダブルコードですが、あれはおそらくjavaについて書かれたものですし、技術書というよりは思想書だと思っています。
極端な話リーダブルなソースというのは誰が書いても同じソースになれば全員が読めるということになるかと思うのですが、リーダブルコードを読んで全員が同じコードを書くようになるかといわれればそうではないでしょう。

次にjavascriptはその簡易性、柔軟性が特徴の言語であり言語により良い書き方というのは変わってくるでしょう。
javaであれば説明的な変数や関数がとても分かりやすくてもjavascriptだと不自然であったりすれば読みずらいですし、javascriptの良さを捨てる行為とも言えます。

リーダブルコードは「ハンマーを持った人はすべてが釘に見える」というアンチパターンを誘発しがちですし、上記の理由からそこまで実際に生きるものではないのではないかと思います。

特にパターンやアーキテクチャが読みやすさには寄与しているかと思うので、そこら辺を勉強なさるとよいと思います。
例えば、質問の内容ではデータが容易されていませんが、テストデータを用意してどういったパターンを通過すればよいのかというのをあらかじめ決めておく、関数化し最低限の組み合わせで開発できるようにすれば読みやすくはなります。
また、ドメイン、つまりどういった役割かに分割して関数や処理を分割するだとか、
データの変換と、処理(質問の例でいればtownに変換と連続したif)を混在させない、など。
実際に生きているノウハウは本を読まなくても学べます。

基本的に読みやすく使いやすいというのはポリモーフィズムにより実現できるかと思うので、そういったパターンを適用可能な範囲に目を向けるべきで、名前、テクニック、美しさが品質に寄与するだろうというのは思想の一種でしかないのではないでしょうか。

実際のソースコード(雑な例)

let location_info = {
    "LocalityName": "Santa Monica", 
    "SubAdministrativeAreaName": "Los Angeles",
    "AdministrativeAreaName": "California",
    "CountryName": "USA"
};

// 入力されている location_info オブジェクトから、LocalityName（都市）、SubAdministrativeAreaName（都市圏）、AdministrativeAreaName（州）、CountryName（国）を取得し、整形して出力する。
const { LocalityName, SubAdministrativeAreaName, AdministrativeAreaName, CountryName } = location_info

// もし都市名がない場合は都市圏名、さらにない場合は州名、最終的に国名を取得して表示する。
let existsName = ''
if(LocalityName) {
    existsName = LocalityName 
} else if (SubAdministrativeAreaName) {
    existsName = SubAdministrativeAreaName
} else if (AdministrativeAreaName) {
    existsName = AdministrativeAreaName
} else if (CountryName) {
    existsName = CountryName
} else {
    // それらが全てない場合はデフォルトの文字列（例：「Middle-of-Nowhere, Planet Earth」）を表示する。
    const defaultStr = 'Middle-of-Nowhere, Planet Earth'
    existsName = defaultStr
}

alert(existsName)

プライオリティを管理する、キーを列挙した配列を定義するだとか、方法はあるがそれが読みやすい方法であるか？は疑問。
代入が一度だけで充分だとかを明示できる文法があればそれを使うべきだとは思う。
また、デフォルトはデフォルトとして記述し上書きする可能性があることを示す方が若干読みやすくはあるかと思う。

---

追記。元のコードが正しいとして、こういった方法も考えてみました。
処理としては関連がある、例外的な処理もあると考えて書いていますが、見やすさについては色々ありえると思います。
そもそもとして、別に列挙しても問題ないだろうとも思いますし、一連の処理なのでどんなデータが来てもこう動くだろうと推測することでわかりやすいともとらえれます。

// 最初に存在した文字列を取得する
let existsName = [LocalityName, SubAdministrativeAreaName, AdministrativeAreaName].find((name) => name)
// すべて存在しない場合
if (!existsName) {
    const defaultStr = 'Middle-of-Nowhere'
    const defaultStr2 = ', Planet Earth'
    if (CountryName) {
        existsName += CountryName
    } else {
        existsName += defaultStr2
    }
}

Answer 3

リファクタリングするほどでもない気もしますが、例えばこんなんどうでしょ
return "" がまずいなら適当に

if (!location_info["LocalityName"]) return "";               // 例 : "Santa Monica"
if (!location_info["SubAdministrativeAreaName"]) reeturn ""; // 例 : "Los Angeles"
if (!location_info["AdministrativeAreaName"]) return "";     // 例 : "California"
if (!location_info["CountryName"]) return "Middle-of-Nowhere, Planet Earth";
return "Middle-of-Nowhere, " + location_info["CountryName"]; // 例 : "USA"