質問をすることでしか得られない、回答やアドバイスがある。

15分調べてもわからないことは、質問しよう!

ただいまの
回答率

88.59%

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

受付中

回答 10

投稿

  • 評価
  • クリップ 14
  • VIEW 7,032

the

score 108

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;
    });
  }
  • 気になる質問をクリップする

    クリップした質問は、後からいつでもマイページで確認できます。

    またクリップした質問に回答があった際、通知やメールを受け取ることができます。

    クリップを取り消します

  • 良い質問の評価を上げる

    以下のような質問は評価を上げましょう

    • 質問内容が明確
    • 自分も答えを知りたい
    • 質問者以外のユーザにも役立つ

    評価が高い質問は、TOPページの「注目」タブのフィードに表示されやすくなります。

    質問の評価を上げたことを取り消します

  • 評価を下げられる数の上限に達しました

    評価を下げることができません

    • 1日5回まで評価を下げられます
    • 1日に1ユーザに対して2回まで評価を下げられます

    質問の評価を下げる

    teratailでは下記のような質問を「具体的に困っていることがない質問」、「サイトポリシーに違反する質問」と定義し、推奨していません。

    • プログラミングに関係のない質問
    • やってほしいことだけを記載した丸投げの質問
    • 問題・課題が含まれていない質問
    • 意図的に内容が抹消された質問
    • 過去に投稿した質問と同じ内容の質問
    • 広告と受け取られるような投稿

    評価が下がると、TOPページの「アクティブ」「注目」タブのフィードに表示されにくくなります。

    質問の評価を下げたことを取り消します

    この機能は開放されていません

    評価を下げる条件を満たしてません

    評価を下げる理由を選択してください

    詳細な説明はこちら

    上記に当てはまらず、質問内容が明確になっていない質問には「情報の追加・修正依頼」機能からコメントをしてください。

    質問の評価を下げる機能の利用条件

    この機能を利用するためには、以下の事項を行う必要があります。

回答 10

+6

動作が質問文のコードと同じかは検証していませんが、
書き方の方針として、次のように ほとんどのロジックは関数に抜きだし、
関数からの返り値を 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;
    }
    });
};


投稿

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

+4

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;



投稿

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

  • 2015/06/22 23:17

    > (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にチェック」

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

    キャンセル

  • 2015/06/22 23:54

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

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

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

    キャンセル

+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に対応していない古いブラウザは知らないよ!

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

投稿

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

+1

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

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         || 'test@test.com';
    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);});

投稿

編集

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

  • 2015/06/22 20:56

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

    キャンセル

  • 2015/06/22 23:31

    例外のメッセージの形式が"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('');

    キャンセル

  • 2015/06/22 23:55

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

    キャンセル

+1

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));
  });
}

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

投稿

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

0

私なら以下のようにします。
本当なら、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

投稿

編集

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

  • 2015/06/24 02:11

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

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

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

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

    キャンセル

  • 2015/06/24 12:07

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

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

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

    var education = res.education;

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

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

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

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

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

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

    キャンセル

  • 2015/06/24 16:41

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

    キャンセル

0

初めて投稿させて頂きます。
私自身完全な独学のみなので、キレイなのかどうか判断しかねる部分がありますが
自分ならこう書くかなという感じで投稿させて頂きました。
私も本来であれば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;
  });
}

投稿

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

0

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

元のコードは、無駄に複雑なロジックが多いという印象です。そのため方針としては、特に構造化や複雑化はせず、読みやすくすっきりさせる方向で考えました。
  • 年齢計算: 愚直なロジックの方が、分かりやすく処理も早いと考えました
  • 性別処理: もともと .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);
  });
}

投稿

編集

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

0

個人的には 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>

投稿

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

0

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

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":'samplemail@samplemail.co.jp',
    "address":"Japan Tokyo",
    "barthday":"1987/1/1",
    "gender":"Okama", // 男女以外の想定 実際は、空なのか、keyが無いのかは未検証
};

投稿

編集

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

  • 2015/10/18 11:17

    どうやら私のコードがベースになっているようですが、いくつか 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'}}} である場合に期待通りに動作しません。

    キャンセル

  • 2015/10/18 11:25

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

    キャンセル

  • 2015/10/19 04:10 編集

    > コンストラクタ内のローカル変数なので 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」で取れるみたいです。 レスポンスサンプル更新しときます。

    キャンセル

  • 2015/10/19 04:45

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

    キャンセル

15分調べてもわからないことは、teratailで質問しよう!

  • ただいまの回答率 88.59%
  • 質問をまとめることで、思考を整理して素早く解決
  • テンプレート機能で、簡単に質問をまとめられる

関連した質問

同じタグがついた質問を見る