JavaScriptの綺麗なコードの書き方

Question

JavaScriptの綺麗なコードの書き方がわかりません。
例えば下記のような関数があった場合、皆さんならどのようにまとめますか？

  function userLogin() {
    FB.api('/me', function(response) {
	  document.getElementById('name').value = response.name;
	  document.getElementById('email').value = response.email;
      function calculateAge(birthday) {
        var birth = birthday.split('/'); /
        var _birth = parseInt("" + birth[2] + birth[0] + birth[1]);
        var today = new Date();
        var _today = parseInt("" + today.getFullYear() + affixZero(today.getMonth() + 1) + affixZero(today.getDate()));
        return parseInt((_today - _birth) / 10000);
      }

      function affixZero(int) {
        if (int < 10) int = "0" + int;
          return "" + int;
      }
      var birthday = calculateAge(response.birthday);
			document.getElementById('age').value = birthday;
			
			if(response.gender === "male") {
			  document.getElementsByName("gender")["1"].checked = true;
      }else if(response.gender === "female") {
			  document.getElementsByName("gender")["2"].checked = true;
      }
			document.getElementById('address').value = response.location.name;
			
			for(ed in response.education) {
        var school = response.education[ed].school;
        var schoolName = school.name;
      }

			document.getElementById('education').value = schoolName;
    });
  }

Answer 1

動作が質問文のコードと同じかは検証していませんが、
書き方の方針として、次のように ほとんどのロジックは関数に抜きだし、
関数からの返り値を DOM に設定するようにしてみては如何でしょう。

function userLogin() {
    FB.api('/me', function(response) {

	function calculateAge(birthday) {
	    var birth = birthday.split('/');
	    var _birth = parseInt('' + birth[2] + birth[0] + birth[1]);
	    var today = new Date;
	    var _today = parseInt('' + today.getFullYear() + affixZero(today.getMonth() + 1) + affixZero(today.getDate()));
	    return parseInt((_today - _birth) / 10000);
	};

	function affixZero(int_v) {
	    var prefix = '';
	    if (0 < int_v && int_v < 10) {
		prefix = '0'
	    }
	    return prefix + int_v;
	};

	function getSchoolName(education) {
	    var schoolName = '';
	    for (var ed in response.education) {
		schoolName = ed.school;
	    }
	    return schoolName;
	};

	function getCBIndex(gender) {
	    var idx = '';
	    if (gender === "male") {
		idx = "1"
	    } else if (gender === "female") {
		idx = "2"
	    }
	    return idx;
	}

	document.getElementById('name').value = response.name;
	document.getElementById('email').value = response.email;
	document.getElementById('age').value = calculateAge(response.birthday);
	document.getElementById('education').value = getSchoolName(response.education);
	var cb_index = getCBIndex(response.gender);
	if (cb_index !== '') {
	    document.getElementsByName('gender')[cb_index].checked = true;
	}
    });
};

Answer 2

function calculateAge(birthday) {
	var birth = birthday.split('/');
	var _birth = parseInt("" + birth[2] + birth[0] + birth[1]);
	var today = new Date();
	var _today = parseInt("" + today.getFullYear() + affixZero(today.getMonth() + 1) + affixZero(today.getDate()));
	return parseInt((_today - _birth) / 10000);
}

ここはかなりひどいです…。普通に算術演算すればいいだけです。数値を無理やり文字列として結合して"0"を足したりそれを数値化したりを繰り返すのは計算コストが非常に高いですし、人間が見ても分かりにくいでしょう。以下でこれで十分です。

あと、parseIntは文字列を数値に変換するための関数ですので、「parseInt((_today - _birth) / 10000);」のような使い方をすると、数値を文字列に変換し、文字列を数値に変換という、不要な型変換が2度発生します。数値の端数の切捨てならMath.floor()を使いましょう。

function calculateAge(birthday) {
	var birth = birthday.split('/');
	var _birth = birth[2] * 10000 + birth[0] * 100 + birth[1];

	var today = new Date();
	var _today = today.getFullYear() * 10000 + (today.getMonth() + 1) * 100 + today.getDate();
	return Math.floor(_today - _birth) / 10000);
}

次にこの部分。

if (response.gender === "male") {
	document.getElementsByName("gender")["1"].checked = true;
} else if (response.gender === "female") {
	document.getElementsByName("gender")["2"].checked = true;
}

「maleでもfemaleでもない場合にはチェックしない」という意図なのか(そんなことある？)
「maleならば、1にチェック、そうでなければ2にチェック」でいいのかどちらでしょうか？
後者なら、私ならこう書いてしまいます。

document.getElementsByName("gender")[(response.gender == "female") + 1].checked = true;

これだと「(response.gender == "female") + 1」の部分が嫌だと言う人がいるかもしれません。そういう人はそこの部分を書き換えていいと思います。しかし以下の部分は共通しており「…」の部分のみ異なるのです。重複した記述を避けるのがきれいなプログラミングの基本です。無駄な繰り返しは行わないようにしましょう。

document.getElementsByName("gender")[…].checked = true;

Comments

[sounisi5011]

> (response.gender == "female") + 1

実際に動かしてみたのですが、これでは"female"ならば2、そうでなければ1、となってしまいます。

var response = {gender: ''};

response.gender = 'male';
console.log((response.gender == "female") + 1); // 1

response.gender = 'female';
console.log((response.gender == "female") + 1); // 2

response.gender = 'unknown';
console.log((response.gender == "female") + 1); // 1

> 「maleならば、1にチェック、そうでなければ2にチェック」

とはならないものと考えます。

[miu_ras]

あぁ…そうですね…。私の表現が間違っていました。すみません。

意味合いとしてはmaleとfemaleしか存在しないことを前提として書いています。なので、「femaleではない」==「maleである」という意味だと理解してください。

もしも男でも女でもない状態を許すのであれば「other」などという選択肢があるべきだと思います。しかしそうではないし、そういうシステムは普通はないよねということで、コードが間違っている可能性の方が高いと思って、「maleとfemaleしか存在しない」ことを前提としています。

Answer 3

綺麗なコードにしたいだって？そりゃ、CoffeeScriptにするのに限るよ。プログラマーの友達は紅茶よりも珈琲だしね。ついでに、日付計算は面倒だからMoment.jsに任せることにするよ。Dateは使いにくいからね。

moment = require 'moment'

class Me
  constructor: ({@name, @email, birthday, @gender, location, education}) ->
    @birthday = moment birthday, 'MM/DD/YYYY'
    @age = moment().startOf('day').diff @birthday, 'years'
    @address = location.name
    @schoolList = education.map (ed) -> ed.school.name

userLogin = ->
  FB.api '/me', (response) ->
    me = new Me response
    document.getElementById('name').value = me.name
    document.getElementById('email').value = me.email
    document.getElementById('age').value = me.age
    [['mail', '1'], ['femail', '2']]
      .filter (pair) -> me.gender == paire[0]
      .forEach (pair) ->
        document.getElementsByName('gender')[pair[1]].checked = true
    document.getElementById('address').value = me.address
    if me.schoolList > 0
      document.getElementById('education').value = me.schoolList[0]

え、課題は「JavaScriptの綺麗なコードの書き方」だからCoffeeScriptを使うのはレギュレーション違反だって？仕方が無いなー。JavaScriptでも書いてみたよー。

'use strict';
import moment from 'moment';

class Me {
  constructor(response) {
    this.name = response.name;
    this.email = response.email;
    this.birthday = moment(response.birthday, 'MM/DD/YYYY');
    this.age = moment().startOf('day').diff(this.birthday, 'years');
    this.gender = response.gender;
    this.address = response.location.name;
    this.schoolList = response.education.map((ed) => ed.school.name);
  }
}

function userLogin() {
  FB.api('/me', (response) => {
    let me = new Me(response);
    document.getElementById('name').value = me.name;
    document.getElementById('email').value = me.email;
    document.getElementById('age').value = me.age;
    [['mail', '1'], ['femail', '2']]
      .filter((pair) => me.gender == paire[0])
      .forEach((pair) => {
        document.getElementsByName('gender')[pair[1]].checked = true;
      });
    document.getElementById('address').value = me.address;
    if (me.schoolList.length > 0) {
      document.getElementById('education').value = me.schoolList[0];
    }
  });
}

なに動かないって？ECMAScript 6に対応していない古いブラウザは知らないよ！

ついカッとなってやった、今は反省している。

Answer 4

function userLogin() {
  FB.api('/me', function(response) {
    if(!response){
      return;
    }

    document.getElementById('name').value = response.name;
    document.getElementById('email').value = response.email;
    document.getElementById('address').value = response.location.name;
    if(response.gender==="female" || response.gender==="male"){
      document.getElementsByName("gender")[1+(response.gender==="female")].checked = true;
    }

    //calculateAge
    document.getElementById('age').value = (function (birthday) {
      function affixZero(n) {
        if (n < 10){
          return "0" + n;
        }
        return "" + n;
      }
      
      var DATE_OBJ = new Date();
      var _Date=affixZero(DATE_OBJ.getDate(),_Month=affixZero(DATE_OBJ.getMonth() + 1;

      var birth = birthday.split('/');
      birth = birth[2]+birth[0]+birth[1];
      
      var today = (DATE_OBJ.getFullYear() + _Month) + _date)-0);

      return ((today - birth) / 10000) | 0;

    }(response.birthday));

    //getSchoolName
    document.getElementById('education').value = (function (_schoolObject){
      var _schoolName="";
      for(_key in _schoolObject) {
        _schoolName = (_schoolObject[_key].school).name;
      }
      return _schoolName;
    }(response.education));
  });
}

綺麗かと言われると微妙かもしれませんが元のコードを意識しつつ、改変してみました。
コードの整理と変数の削減(説明用の変数という観点から見るとこれはしない方が良いようにも思えますが……)を意識して記述しました。

Answer 5

私なら以下のようにします。
本当なら、userLogin関数内に別の関数を定義するコードそのものを控えたいのですが…

function userLogin() {
    /**
     * "/me"用の処理
     * @param {Object} res レスポンスオブジェクト…?
     */
    function meCallback(res) {
        var nameE = getId('name');
        nameE.value = res.name;

        var emailE = getId('email');
        emailE.value = res.email;

        var birthday = calculateAge(res.birthday);
        var ageE = getId('age');
        ageE.value = birthday;

        var gender = res.gender;
        var genderEs = getNames('gender');
        var genderE =
              (gender === 'male') ? genderEs[1]
            : (gender === 'female') ? genderEs[2]
            : null;
        if (genderE) {
            genderE.checked = true;
        }

        var addressE = getId('address');
        addressE.value = res.location.name;

        var education = res.education;
        var schoolName = '';
        forObject(education, function (_, education) {
            schoolName = education.school.name;
        });
        var educationE = getId('education');
        educationE.value = schoolName;
    }

    /**
     * 現在の日付と誕生日との差を取得する
     * @param {string} birthday 誕生日を示す日付文字列。形式は"mm/dd/yyyy"
     * @return {number}
     */
    function calculateAge(birthday) {
        var birth = dateToInt(birthday);
        var today = getTodayInt();

        return parseInt((today - birth) / 10000, 10);
    }

    /**
     * 現在の日付の数値を取得する
     * @return {number}　日付を数値に変換した値。"yyyymmdd"形式の文字列を数値に変換したもの。
     */
    function getTodayInt() {
        var today = new Date();

        var year = today.getFullYear() + '';
        var month = affixZero(today.getMonth() + 1);
        var date = affixZero(today.getDate());

        return parseInt(year + month + date, 10);
    }

    /**
     * 日付文字列を数値に変換する
     * @param {string} date_str 日付文字列。形式はmm/dd/yyyy
     * @return {number}　日付を数値に変換した値。"yyyymmdd"形式の文字列を数値に変換したもの。
     */
    function dateToInt(date_str) {
        var date_params = date_str.split('/');
        return parseInt(date_params[2] + date_params[0] + date_params[1], 10);
    }

    /**
     * オブジェクトに対するfor
     * @param {Object} obj 対象のオブジェクト
     * @param {function(string=, ?*=)} callback コールバック関数。第一引数にプロパティ、第二引数に値が代入される。
     */
    function forObject(obj, callback) {
        var supportHasOwnProperty = obj.hasOwnProperty !== void 0;
        var p;
        for (p in obj) {
            if (!supportHasOwnProperty || obj.hasOwnProperty(p)) {
                callback(p, obj[p]);
            }
        }
    }

    /**
     * 数値のゼロ埋めを行う
     * @param {number} num ゼロ埋めを行う数値。1桁の数値の先頭に0を追加し2桁にする。
     * @return {string} ゼロ埋めを行った数値文字列
     */
    function affixZero(num) {
        return ('0' + num).slice(-2);
    }

    /**
     * `document.getElementById`のエイリアス
     * @param {string} id 取得する要素のid属性値
     * @return {?HTMLElement} 取得した要素
     */
    function getId(id) {
        return document.getElementById(id);
    }

    /**
     * `document.getElementsByName`のエイリアス
     * @param {string} name 取得する要素のname属性値
     * @return {NodeList} 取得した要素を含むNodeList
     */
    function getNames(name) {
        return document.getElementsByName(name);
    }

    FB.api('/me', meCallback);
}

元のコードにあった、以下の問題を修正しています。

• parseInt関数の第二引数を指定し、過去のブラウザにおいて問題が発生する可能性を回避しました。
• affixZero関数の引数として定義された変数intは、"int"が予約語のため利用できません。このため、変数名をnumに書き換えました。
• for inは、オブジェクトのループとしてそのまま利用するには不適切です。このため、for inの処理をforObject関数にまとめ、書き換えました。

parseInt関数について:

• parseInt() - JavaScript | MDN
• JavaScriptでparseIntする場合は基数を指定しよう | 株式会社LIG

"int"が予約語であるという出典:

• Reserved keywords in JavaScript · Mathias Bynens

for inについて:

• JavaScript - for in文とfor文 - Qiita

Comments

[nanndemoiikara]

粒度が細かい
宣言位置が多い
なぜ三項演算子にしたのかわからない
> var genderE =
> (gender === 'male') ? genderEs[1]
> : (gender === 'female') ? genderEs[2]
> : null;

上記の理由から可読性が悪く感じます。
getId、getNamesメソッド等はこの規模の物に必要ないかと思うのですが
いかがでしょうか？

[sounisi5011]

> 粒度が細かい

単機能に分割した方が、個別の処理を把握しやすくなるものと考えました。
meCallback関数の中身もより細かい部分に分割したほうが良かったかもしれません。

> 宣言位置が多い

宣言を多くしているのは、可読性を向上させるためです。
各変数に代入する式を簡潔にすれば、それだけ理解しやすくなります。
また値を変更する度に別の変数名を付けたほうが、その値が何を担っているのか読み解きやすくなります。

> なぜ三項演算子にしたのかわからない

条件文を1つの変数に集約させることで、より分かりやすく、尚且つバグを減らせるものと考えます。

> 上記の理由から可読性が悪く感じます。

私にはこちらの方が読みやすいです。

> getId、getNamesメソッド等はこの規模の物に必要ないかと思うのですが

getNames関数に関しては、完全に助長で余分ではあります。
が、getIdに関しては、長い`document.getElementById`という文字列を見なくて済む分、入力の手間も減り、可読性も向上するものと考えます。
最も、関数名に関してはもうすこしどうにかしたほうが良いかもしれません。

[Cf_cwd]

三項演算子について意図(分岐用のコードをまとめることを目的にしていること)は分かりますが私も少し読みにくい印象を受けました。
説明用変数の考え方には同意します。
ですがElementを省略するならElmのような省略の方がわかりやすいかもしれませんね。
emailEやaddressEといった名前が合わせて使用されていると「Eメールアドレス」との誤読を招く、もしくは認識の妨げになるかもしれません。

[sounisi5011]

> ですがElementを省略するならElmのような省略の方がわかりやすいかもしれませんね。
> emailEやaddressEといった名前が合わせて使用されていると「Eメールアドレス」との誤読を招く、もしくは認識の妨げになるかもしれません。

var nameElem = getId('name');

var emailElem = getId('email');

var ageElem = getId('age');

var genderElems = getNames('gender');
var genderElem =
(gender === 'male') ? genderElems[1]
: (gender === 'female') ? genderElems[2]
: null;

var addressElem = getId('address');

var educationElem = getId('education');

一理あると考えます。
こうして書いてみると、こちらの方が若干分かりやすいように思います。

[sounisi5011]

三項演算子の是非について、私は以下の記事の主張に同意します。

http://qiita.com/tanakahisateru/items/4fd82e03c90dc130329a

記述法は以下を参考にしています。

http://qiita.com/chokuryu@github/items/8b9cff5ea92be245b026

[nanndemoiikara]

回答頂きありがとうございます。

> また値を変更する度に別の変数名を付けたほうが、その値が何を担っているのか読み解きやすくなります。

その値が何を担っているかという部分に関してはsounisi5011様のコードは
EやEs等でElementなのかNodeListなのか命名規則が明確で非常にわかりやすく思います。
ただ2度も使用しないgetId('address')等を変数に入れる必要はございますでしょうか？

> 条件文を1つの変数に集約させることで、より分かりやすく、尚且つバグを減らせるものと考えます。

説明不足で申し訳ございません。
sounisi5011様の主張で同意して頂いている
http://qiita.com/tanakahisateru/items/4fd82e03c90dc130329a
のURLから抜粋させて頂きます。
> あらゆる変数から、一時的にですら null である可能性を完全に排除できた。何より素晴らしいのは、変数の値を決定するのが「1行」になったことだ。変数の妥当化までに他の手続きが介入する余地がいっさいない。

ここで「」で表示強調されているように1行になる事が重要なのではないかなと思います。
その為、改行して三項演算子を使うぐらいならifで分けたほうが良いのでは？と思った為です。
そちらについては参考で記述頂いております下記URLコメント欄でも記述されております。
http://qiita.com/chokuryu@github/items/8b9cff5ea92be245b026

> getIdに関しては、長い`document.getElementById`という文字列を見なくて済む分、入力の手間も減り、可読性も向上するものと考えます。

このコードを読む人はJSが少なくとも読める人かと思います。
その為、getIdと書かれるよりはdocument.getElementByIdと書いて頂いた方がわかりやすいのではないかなと思うのですが
いかがでしょうか？

[sounisi5011]

> ただ2度も使用しないgetId('address')等を変数に入れる必要はございますでしょうか？

この点については私自身助長であると理解しています。

が、このような最適化を行い続ければ読みにくくなります。
例えば、この関数が多数の引数を持つ関数の場合、または関数/メソッド名が長い場合に可読性が大幅に減少します。

このコードでは`document.getElementById`を`getId`に短縮しているためその利点を把握しにくいですが、これを元に戻すとその読みにくさが顕在化します。

document.getElementById('address').value = response.location.name;

以上のコードは、要素の取得を意味する`document.getElementById`メソッドに34文字も使用しています。また代入する値も、22文字と長いものです。
これを読むためには、視界を左から右へと、長いこのコードを読み解くために移動させる必要があります。それは可読性の減少に繋がります。

続いて、変数に代入した以下はどうでしょうか。

> var addressE = document.getElementById('address');
> addressE.value = res.location.name;

変数名の長さにも影響されますが、この方が可読性は向上します。
何故なら、要素を意味する`document.getElementById('address')`が34文字なのに対し、変数`addressE`は僅か8文字です。
全体的な文字数は増えるものの、「addressE = document.getElementById('address')」という事を意識してしまえば、肝心の処理部分は「addressE.value = res.location.name」だけとなります。
圧倒的に読みやすいです。少なくとも私はそのように感じています。

---

> ここで「」で表示強調されているように1行になる事が重要なのではないかなと思います。

行数は問題ではありません。
三項演算子が式であるため、結果がどうであれ確実に変数genderEが定義され、以降の処理ではこの変数に対する簡潔な処理のみを記述すれば良い点が大きな利点です。

> こう書くことで、$data は条件付きで変化する状態変数ではなく、必ず再定義されるイミュータブルな変数(とみなして扱うことが可能)であることが、機械的に保証できた。このほうが関数型言語でいう変数(Scalaのval)に近い。

私は、上記の「必ず再定義されるイミュータブルな変数」に条件を集約してしまうこの記述が最も適切であると考えています。

---

> このコードを読む人はJSが少なくとも読める人かと思います。
> その為、getIdと書かれるよりはdocument.getElementByIdと書いて頂いた方がわかりやすいのではないかなと思うのですが

成程…それは確かに一理あるかもしれません。

prototypeの宣言に関する質問でも、そのような観点からわざわざ変数に格納しない、等の意見が見られました。
https://teratail.com/questions/4919

[Cf_cwd]

本題とは異なりますがdocument.getElementByIdを変数に代入することによりキャッシュが効いてほんの少し処理が早くなりそうな気がしないでもないですね。どうなんでしょう……

三項演算子を使用することのメリットであると私が考えるのは代入の式を何度も書く必要がない点でしょうか。if-elseですと代入式を何度か書く必要がありますからね。
ですから複雑でないのであれば三項演算子を使う意味やある種のメリットがあると考えます。
(DRYの原則的にもその方がいいのですかね？)

ですが今回の例ですと三項演算子がネストされていて、一瞬ですが関係を把握するのに思考を必要とするように思います。この点に関してどう解釈すべきかが問題ではないでしょうか。
今回の例を読みやすさという点から見るとif-elseの方が私は読みやすいと思います。
これは処理の流れをそのまま文に起こしているからですね。
ですがある特定の処理とその記述といった点から見ると三項演算子を使い、重複部分を省いたコードは綺麗だと思いました。

つまり処理を追って読む場合はif-elseの方が読みやすく、それらを関数かなにかのように一連の塊としてみる際には三項演算子の方がまとまっている印象を受けました。

[sounisi5011]

> 本題とは異なりますがdocument.getElementByIdを変数に代入することによりキャッシュが効いてほんの少し処理が早くなりそうな気がしないでもないですね。

肝心のdocument.getElementByIdに関しては、関数内でこの記述をそのまま呼んでいるので、むしろ関数処理のオーバーヘッドがかかって遅くなるかもしれません。(未検証)

ただ、別の箇所にて、この考え方は考慮していました。
今までのコメントで触れてはいなかったのですが、一々変数に代入しているのは、これを意識している事も理由として挙げられます。

var education = res.education;

例えば上記の文ですが、こうすればres.educationが変数educationにキャッシュされるので、この後のfor文で一々resオブジェクトを読みにいく必要が無くなります。

確かwindowオブジェクトも、ローカルの変数にキャッシュすればパフォーマンスが向上するらしいです。

> ですが今回の例ですと三項演算子がネストされていて、一瞬ですが関係を把握するのに思考を必要とするように思います。この点に関してどう解釈すべきかが問題ではないでしょうか。

その点について考慮が足りませんでした。

私は三項演算子のこの記述法に慣れきっていて、switch文と同じような感覚で利用しています。
なので、極めて簡潔で見やすいです。

しかしこの記述を知らない人間にとって、三項演算子を解釈する必要性があり、それにより読みにくくなっている点があります。
それについて考慮できていませんでした。

[Cf_cwd]

私も自作のライブラリ内で三項演算子を用いて似たような記述をしているのですが周囲の友人からの反応は微妙ですね。
代入のための文を一つにまとめることができて場合によってはスマートだと思うんですがね……
以前可読性を保つために三項演算子の乱用は避けた方が良いという記述をいくつかのサイトで見かけた記憶があります。(情報元を提示できなくて申し訳ないです)
書いた本人は対応関係をあっさり追えますがそのコードを初めて読む人はその対応を一度整理する必要があり、若干ながら読む際のコストを必要とするからでしょうか

Answer 6

私も綺麗な書き方が気になります。
私の場合この様に記述します。

var doc = document;
 function userLogin(res) {

    this.gender        = res.gender        || 'male';
    this.birth_day     = res.birthday      || '01/01/1970';
    this.name          = res.name          || 'sample';
    this.email         = res.email         || '[e-mail]';
    this.location_name = res.location.name || 'test';
    this.education     = res.education     || {};

    this.Controller();
}

userLogin.prototype = {
    Controller   : function()
    {
        doc.getElementById('name').value  = this.name;
        doc.getElementById('email').value = this.email;
        doc.getElementById('age').value   = this.calculateAge();
        var genderDOMs = doc.getElementsByName('gender');

        switch ( this.gender )
        {
            case 'male':
                    genderDOMs[1].checked = true;
                break;
            case 'female':
                    genderDOMs[2].checked = true;
                break;
        }
        doc.getElementById('address').value = this.location_name;
        //TODO:ちょっとここ元のソースが何がしたいのかわんかんない
        var schoolName = '';
        for ( var ed in this.education ) schoolName = this.education[ed].school.name;

        doc.getElementById('education').value = schoolName;
    },
    calculateAge : function()
    {
        var ymd   = parseInt(this.birth_day.replace(/(\d{2}).(\d{2}).(\d{4})/, '$3$1$2'), 10),
            date  = new Date(),
            today = [date.getFullYear(), 
                ('0' + (date.getMonth() + 1) ).slice(-2), 
                ('0' + date.getDate()).slice(-2)].join('');

        if ( isNaN(ymd) ) throw Error('Exception birthDay Format mm/dd/yyyy');

        return parseInt( ( (today - ymd) / 10000), 10);
    }
};
FB.api('/me', function(res){new userLogin(res);});

Comments

[sounisi5011]

元のコードを良く読むと、`response.birthday`の形式は「月/日/年」のようです。

> var _birth = parseInt("" + birth[2] + birth[0] + birth[1]);

このため、`response.birthday`に対する処理は"/"を削除するだけでは駄目です。

> var ymd = parseInt(this.birth_day.replace(/\u002f/g, '')),

また`gender`に関する処理も、"male"でも"female"でもない場合はそもそもチェックボックスをチェックしないような処理ではないかと思われます。

> if(response.gender === "male") {
> document.getElementsByName("gender")["1"].checked = true;
> }else if(response.gender === "female") {
> document.getElementsByName("gender")["2"].checked = true;
> }

"male"以外の場合に`document.getElementsByName('gender')[2]`をチェックしてしまうような処理は誤りではないでしょうか。

>this.gender = ( gender === 'male' )? genderDOM[1] : genderDOM[2] ;

[sounisi5011]

さらに、parseInt関数は第二引数を指定しないと、問題を起こす可能性があります。
これについては、質問のコードでも同じですが…

http://liginc.co.jp/web/js/31018

[nanndemoiikara]

コードレビュー頂きありがとうございます。

> 元のコードを良く読むと、`response.birthday`の形式は「月/日/年」のようです。
> このため、`response.birthday`に対する処理は"/"を削除するだけでは駄目です。
> また`gender`に関する処理も、"male"でも"female"でもない場合はそもそもチェックボックスをチェックしないような処理ではないかと思われます。
> さらに、parseInt関数は第二引数を指定しないと、問題を起こす可能性があります。

ご指摘頂きありがとうございます。勉強になります。
また、DOMの取得部分もバラバラでしたので可読性が悪いコードでしたので修正いたしました。
問題がございましたらご指摘頂けますと幸いでございます。

[sounisi5011]

`response.birthday`の形式について、
初期値と例外のメッセージの形式が「年/月/日」のままです。

> this.birth_day = res.birthday || '1970/01/01';

> if ( isNaN(ymd) ) throw Error('Exception birthDay Format yyyy/mm/dd');

[nanndemoiikara]

ご指摘頂きありがとうございます。
修正致しました。

[sounisi5011]

例外のメッセージの形式が"yyyy/mm/dd"のまま直っていません。

> throw Error('Exception birthDay Format yyyy/mm/dd');

また、calculateAge関数内の宣言部にて、変数ymdの代入文の後ろが`;`で終わっています。
このため、変数date, todayがvar文に含まれておらず、グローバル変数として宣言されてしまいます。

> var ymd = parseInt(this.birth_day.replace(/(\d{2}).(\d{2}).(\d{4})/, '$3$1$2'), 10);
> date = new Date(),
> today = [date.getFullYear(),
> ('0' + (date.getMonth() + 1) ).slice(-2),
> ('0' + date.getDate()).slice(-2)].join('');

[nanndemoiikara]

ご指摘頂きありがとうございます。
修正いたしました。

Answer 7

さくっとこんな具合ですかね。

var userLogin = function (){
	function userLogin(response){
		var option = $.extentd(response); // jQuery 使っていれば…

		var userLogin = {};
		userLogin.name = option.name || '';
		userLogin.email = option.email || '';
		userLogin.address = option.location.name || '';
		userLogin.age = this.calculationAge(option.barthday || '' );
		userLogin.gender = this.distinctionGender(option.gender || '');
		userLogin.education = this.odistinctionSchool(option.education || []);

	}
};
userLogin.prototype = {
	GENDER_MALE:'male',
	GENDER_FEMALE'female'.

	getName:function(){return this.name;},
	getEmail:function(){return this.email;},
	getAddress:function(){return this.address;},
	getAge:function(){return this.age;},
	getGender:function(){return this.gender;},
	getEducation : function(){return this.education;},

	calculationAge:function(barthday){
		var barth = new Date(barthDay);

		// 日付に変換できなければ 空文字列
		if (isNan(barth)) return '';

		var toDay = new Date().toISOString().substring(0,10).replace(/-/g,'');
		barth = barth.toISOString().substring(0,10).replace(/-/g,'');

		return Math.floor((toDay -barth) / 10000);
	},
	distinctionGender:function(gender){

		switch(gender){
		case GENDER_MALE:
		case GENDER_FEMALE:
			// 正しい値だったからそのまま
			break;
		default:
			// 初期値は男
			gender = GENDER_MALE;
		}
		return gender;
	},
	distinctionSchool:function(education){
		var schoolName = [];
		for (var i = 0,len = education.length ; i < len; len++){
			schoolName.push(education[i].school.name || '');
		}
		return schoolName;
	}

};

FB.api('/me', function(response) {

	var formItem = new userLogin(response);
	var elements = form.elements;

	elements['name'].value = formItem.getName();
	elements['email'].value = formItem.getEmail();
	elements['address'].value = formItem.getAddress();
	elements['age'].value = formItem.getAge();
	getElementByValue(elements['gender'],formItem.getGender()).checked = true;
	elements['#education'].value = formItem.getName().join(); // 引数なしjoinはカンマ区切り
}

// レスポンス サンプル
var response = {
	"id": "123456789012345",
	"name": "Tarou Tanaka",
	"education": [
		{
			"school": {
				"id": "000000000000001",
				"name": "○○高等学校"
			},
			"type": "High School",
			"year": {
				"id": "000000000000002",
				"name": "2012"
			}
		},
		{
			"school": {
				"id": "000000000000003",
				"name": "○○専門学校"
			},
			"type": "College",
			"year": {
				"id": "000000000000004",
				"name": "2014"
			}
		}
	],
	"birthday": "01/21/1987",
	"email":'[e-mail]',
	"address":"Japan Tokyo",
	"barthday":"1987/1/1",
	"gender":"Okama", // 男女以外の想定 実際は、空なのか、keyが無いのかは未検証
};

Comments

[think49]

どうやら私のコードがベースになっているようですが、いくつか ReferenceError が発生します。
ReferenceError: form is not defined
ReferenceError: getElementByValue is not defined

> if (isNan(barth)) return '';
何もしなければ(この一文を削除すれば)、input要素に反映された時点で age === "NaN" になり、エラーである事が明確になります。
age === '' にすると処理が出来なかったのか、処理して空文字なのかが分からなりませので、NaN を返すのがベストだと個人的には思います。

> case this.GENDER_MALE:
> case this.GENDER_FEMALE:
いずれもコンストラクタ内のローカル変数なので undefined になるのでは。

> elements['#education'].value = formItem.getName().join(); // 引数なしjoinはカンマ区切
好みの問題ですが、"SchoolA,SchoolB" はセパレータが分かりづらいと思ったので "SchoolA, SchoolB" にしてみました。

> distinctionSchool:function(education){
FB.api の callback 返り値オブジェクトの仕様が不明だったので質問者さんのコードが for-in であることから Object,keys を使いましたが、配列で良かったのでしょうか。
例えば、education = {a: {school: {name: 'schoolA'}}} である場合に期待通りに動作しません。

[think49]

> var option = response.slice();
response は new Object だと思っていましたが、Array.prototype.slice を持っているのでしょうか。
このコードの意図が分かりません。

[Ryo]

> コンストラクタ内のローカル変数なので undefined
…あ、prototypeへの引っ越しで漏れてますね。お恥ずかしい…
机上だけで、実行もしてなかったです。

> education = {a: {school: {name: 'schoolA'}}}
本物のレスポンス気になったんで後述のページでデータ見ると
education:[{school:{id:"",name:"",},type:"",year:{id:"",name:""}}]
になってたので、問題ないかと。

> var option = response.slice();
slice持って無いですね。期待したのはjQueryの $.extend() の動作です。
この時点で、参照を切っておきたいので素直にそっちを使えばよかったかなと
思っています。

投稿したあとFB.api()のリファレンス覗いたんですが、 response帰ってきた直後に
if (response && !response.error) { /* 正常時の処理群 */ }
で、「エラー出たらerrorって返すからよろしく」だそうで

実際のresponseは
Facebook にログインした状態で https://developers.facebook.com/tools/explorer/
にアクセスし、「Get Token」で取得したい情報を選択
遷移したページ右下の「OK」で取れるみたいです。 レスポンスサンプル更新しときます。

[Ryo]

think49 さんレビューありがとうございました。

Answer 8

個人的には getElementById が並ぶコードは美しくないと思うので、HTML から作り直したくなります。
JavaScript コードの側面では下記変更を加えています。

• form関連要素の参照に getElementById ではなく、HTMLFormElement#elements を使用するようにした
• String.prototype.padLeft を使った(ES7 提案)
• school.name が最後の名前だけ取得する仕様だった為、リストで取得するようにした
• useLogin 内で関数定義しない為にクロージャに閉じ込めた
• parseInt を Number に変更
• 数値切捨てを parseInt から Math.floor に変更(parseInt は内部処理で String 型にキャストするので数値切り捨てに使うべきではありません)

<form id="sample">
  <p><label>name <input type="text" name="name" value=""></label></p>
  <p><label>email <input type="text" name="email" value=""></label></p>
  <p>gender
    <label><input type="radio" name="gender" value="male">male</label>
    <label><input type="radio" name="gender" value="female">female</label>
  </p>
  <p><label>age <input type="text" name="age" value=""></label></p>
  <p><label>address <input type="text" name="address" value=""></label></p>
  <p><label>education <input type="text" name="education" value=""></label></p>
</form>

<script>
'use strict';
var userLogin = (function (){
  function userLogin (form) {
    FB.api('/me', function (response) {
      var elements = form.elements;

      elements['name'].value = response.name;
      elements['email'].value = response.email;
      getElementByValue(elements['gender'], response.gender).checked = true;
      elements['age'].value = calculateAge(response.birthday);
      elements['address'].value = response.location.name;
      elements['education'].value = getSchoolNames(response.education).join(', ');
      form = null;
    });
  }

  function calculateAge (birthday) {
    var today = new Date;

    today = Number([today.getFullYear(), padLeft(today.getMonth() + 1, 2, '0'), padLeft(today.getDate(), 2, '0')].join(''));
    birthday = birthday.split('/');
    birthday = Number([birthday[2], padLeft(birthday[0], 2, '0'), padLeft(birthday[1], 2, '0')].join(''));

    return Math.floor((today - birthday) / 10000);
  }

  function getSchoolNames (education) {
    return Object.keys(education).map(function (key) {
      return education[key].school.name;
    });
  }

  function getElementByValue (elements, value) {
    var i = elements.length,
        element;

    while (i--) {
      element = elements[i];

      if (element.value === value) {
        return element;
      }
    }

    return null;
  }

  /**
   * ES7 String.prototype.padLeft
   * @url https://github.com/ljharb/proposal-string-pad-left-right
  **/
  var padLeft = (function (){
    if (typeof String.prototype.padLeft == 'function') {
      return function padLeft (string, maxLength /*, fillString*/) {
        return arguments.length > 2 ? String.prototype.padLeft.call(string, maxLength, arguments[2]) : String.prototype.padLeft.call(string, maxLength);
      };
    }

    return function padLeft (maxLength /*, fillString*/) {
      var string = String(this),
          fillString = arguments.length < 2 ? ' ' : String(arguments[1]),
          stringLength = string.length,
          diffLength = maxLength - stringLength;

      return diffLength < 1 ? string : Array(ceil(diffLength / fillString.length) + 1).join(fillString).slice(0, diffLength) + string;
    };
  }());

  return userLogin;
}());

userLogin(document.getElementById('sample'));
</script>

Answer 9

時間が経っていますが、面白い課題だと思ったので回答します。

元のコードは、無駄に複雑なロジックが多いという印象です。そのため方針としては、特に構造化や複雑化はせず、読みやすくすっきりさせる方向で考えました。

• 年齢計算: 愚直なロジックの方が、分かりやすく処理も早いと考えました
• 性別処理: もともと .checked=false であったという前提を含んでいます
• 学校名処理: 速度は落ちるかもしれませんが、「最終」学歴ということを見やすくしたつもりです

懸案事項として一点、、学校名処理に関して、そもそもJavaScriptのobjectにorderは存在しないので、前提のロジックを考え直した方が良いように思いました。

function userLogin() {
  FB.api('/me', function(response) {
    document.getElementById('name').value = response.name;
    document.getElementById('email').value = response.email;
    document.getElementById('age').value = (function getAgeOf(birthday_string) {
      var birthday = new Date(birthday_string);
      var today = new Date();
      var differenceOfYear = today.getFullYear() - birthday.getFullYear();
      if (today.getMonth() <= birthday.getMonth()
        && today.getDate() <  birthday.getDate())
        return differenceOfYear - 1;
      else
        return differenceOfYear;
    })(response.birthday);
    document.getElementById('address').value = response.location.name;
    document.getElementsByName("gender")[1].checked = (response.gender === "male");
    document.getElementsByName("gender")[2].checked = (response.gender === "female");
    document.getElementById('education').value = (function getSchoolNameOf(educations) {
      var keys = Object.keys(educations);
      return educations[keys.pop()].school.name;
    })(response.education);
  });
}

Answer 10

初めて投稿させて頂きます。
私自身完全な独学のみなので、キレイなのかどうか判断しかねる部分がありますが
自分ならこう書くかなという感じで投稿させて頂きました。
私も本来であればuserLogin内に沢山巻数を置きたくはないのですが、かと言ってglobal直下にも置きたくはありませんので、他の部分がどうなっているか判断できませんことからuserLogin内に書く方法にしています。
折角なので指摘などして頂けますと幸いです。

function userLogin() {
  // "01", "12" のような 2桁の文字列にして返す
  var affixZero = function(n) {
    return ('0' + n).slice(-2).toString();
  };

  var calculateAge = function(birthday) {
    // birthday は 月/日/年 の文字列形式で渡ってくる
    var birthDayArg = birthday.split('/'),
        _birthDay = (birthDayArg[2] + affixZero(birthDayArg[0]) + birthDayArg[1]) - 0,
        today = new Date(),
        _today = ( today.getFullYear() + affixZero(today.getMonth() + 1) + affixZero(today.getDate()) ) - 0;
    return _birthDay - _today;
  };

  var checkGenderId = function(genderStr) {
    var genderList = ['', 'male', 'female'];
    return genderList.indexOf(genderStr);
    // 別言語でも gender 判定をするなら素直にif文の方がメンテが楽かと思います。
    /*
    var gId = 0;
    if(genderStr === 'male' || genderStr === 'maschio') {
      gId = 1;
    } else if(genderStr === 'female' || genderStr === 'femminile') {
      gId = 2;
    }
    return gId;
    */
  };

  var getSchoolName = function(educations) {
    var schoolName = '',
        keys = Object.keys(educations);
    for( var i=0, l=keys.length; i<l; i+=1) {
      var key = keys[i];
      if(educations[key].school && educations[key].school.name) {
        schoolName = educations[key].school.name;
        break;
      }
    }
    return schoolName;
  };

  FB.api('/me', function(response) {
    var d = document,
        name = response.name,
        email = response.email,
        age = calculateAge(response.birthday),
        gender = checkGenderId(response.gender),
        address = response.location && response.location.name || '',
        schoolName = getSchoolName(response.education);

    /*
      他にセットする値の処理や整形をするのであればココに記述していく。 
     */

    // 値をセットしていく
    d.getElementById('name').value = name;
    d.getElementById('email').value = email;
    d.getElementById('age').value = age;
    if(gender) {
      d.getElementsByName("gender")[gender].checked = true;
    }
    d.getElementById('address').value = address;
    d.getElementById('education').value = schoolName;
  });
}