LoginSignup

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?

This article is a Private article. Only a writer and users who know the URL can access it.
Please change open range to public in publish setting if you want to share this article with other users.

More than 5 years have passed since last update.

@svartalfheim(Yusuke Tashiro)

Javascriptコードレビュー20140416

Last updated at Posted at 2014-04-16

Javascriptコードレビュー

ソースコード

今回はAndriod用にビルドされてものを対象にしています。

https://svn.sonicmoov.com:444/svn/chizunavi/branches/1.0/chizunavi/smart/js/jquery.fullPage2.js

コードレビュー基本評価項目（Qiita）

あとなんか社内のSVNルールと違うような気がする感ある。

コードレビュー

コード規約編

1.定数・変数について

定数になりえそうな変数はざっと見た感じでは見当たらなかったので問題無いです。

2.クラス名はupperCamelCase、変数名,メソッド名はlowerCamelCase（Class,variable）

変数・メソッド名はきちんとlowerCamelCaseでほぼ統一されていました。すばらしいです。
クラスは見た限り宣言されてないようです。

※sublime-textのjshintプラグインやgrunt-contrib-jshintで以下の設定で発見できます

options:{
    camelcase:true
}

//731行目
var settime = 100;//惜しい！

3.インデントの調整

ソースコードのインデントは綺麗に統一されていました。すばらしいです。
ただ、インデントはタブではなく 空白スペース の方がよいでしょう。
あと変数のインデントも揃えると見やすいと思います。

//1178行目
//修正前
var destinyPos = destiny.position();
var slidesContainer = slides.find('.slidesContainer').parent();
var slideIndex = destiny.index();
var section = slides.closest('.section');
var sectionIndex = section.index('.section');
var anchorLink = section.data('anchor');
var slidesNav = section.find('.fullPage-slidesNav');
var slideAnchor = destiny.data('anchor');

//修正後
var destinyPos      = destiny.position();
var slidesContainer = slides.find('.slidesContainer').parent();
var slideIndex      = destiny.index();
var section         = slides.closest('.section');
var sectionIndex    = section.index('.section');
var anchorLink      = section.data('anchor');
var slidesNav       = section.find('.fullPage-slidesNav');
var slideAnchor     = destiny.data('anchor');

4.名前空間汚染の回避（Namespaceを作る）

分けたほうが再利用性が高まりますが、具体的なお話はコンポーネント化のところでします。

補足

1.jQueryオブジェクトはより明示的な方が早期バグ発見につながります

//342行目
//こっちよりも
var nav = $('#fullPage-nav');

//こっちの方がわかりやすい気がす…る……
var $nav = $('#fullPage-nav');

2.jQueryオブジェクトを変数に格納する方がパフォーマンスがよいです

//785行目
//修正前
$('body').removeClass('section0').//中略
$('body').addClass(cid);
$('body').removeClass('open');

//修正例
var $body = $('body');
$body.removeClass('section0').//中略
$body.addClass(cid);
$body.removeClass('open');

修正前はbodyタグの検索を$('body')が呼び出されるたび行っているので3回分のコストですが、
修正後は1回の検索コストで済んでいます。

3.冗長なコードはなるべくまとめた方がよいでしょう

以下は修正中に削除し忘れたコードだと思いますが。。

//32行目
//修正前
var displayHeight = window.innerHeight ? window.innerHeight : window.innerHeight;

//修正後
var displayHeight = window.innerHeight;

それとjQueryの初期化関数は一箇所にまとめても良いと思います。

  • 5行目の$(function(){
  • 179行目の(function($) {
  • 1901行目の$(document).ready(function() {

コメント

ほとんど書かれていません。だめです。
あとコメントは日本語で書いたほうがいいと思います。

//226行目
//Defines the delay to take place before being able to scroll to the next section
//BE CAREFUL! Not recommened to change it under 400 for a good behavior in laptops and 
//Apple devices (laptops, mouses...)

意味はわかるけど、日本語じゃだめなの…？

コンポーネント化

1.ソースコードの構造

全容を把握しているわけではありませんが、だいたい以下のような感じのようです。

  • $.fn.fullpageを中心にほとんどすべての処理がそのjQueryプラグインの内部に記述されてます。
  • 内部の主な処理
    • サイドメニューの管理
    • スクロール動作の管理
    • デバイス判別処理
    • URL周りの処理
    • その他いろいろ

2.単一の機能のコンポーネントを抽出する

まずその前にNamespaceを用意します。
Namespaceはグローバル大域を汚染しなくて済むので結構便利です。
Namespaceの宣言の仕方はいくつかありますが、ファイル分割をしないのであればファイルの先頭で宣言すればいいと思います。
ファイルを分割する場合はNamespace.jsを用意するか各ファイルの先頭で
以下のように宣言すればいいと思います。

var chizunavi = this.chizunavi || {};

話を戻しますが、このプログラムでいうデバイス判別処理は独立した機能なので抜き出します。


var chizunavi = this.chizunavi || {};

/**
 * 端末機種判別や回転状態を判別するUtilクラス
 */
chizunavi.DeviceUtil = {
    /**
    * @return {boolean} iOS端末かどうかの真偽値
    */
    isIOS:function(){
        if((navigator.userAgent.indexOf('iPhone') > 0 && navigator.userAgent.indexOf('iPad') > 0)){
            return true;
        }
        return false;
    },
    /**
    * @return {boolean} 端末が縦向きかどうか
    */
    isPortrait:function(){
        if(Math.abs(window.orientation) === 0 || Math.abs(window.orientation) === 180){
            return true;
        }
        return false;
    }
};

if(chizunavi.DeviceUtil.isIOS() && chizunavi.DeviceUtil.isPortrait()){
    //iOSで縦向きの場合
}

※iOSを固有名詞とみるとisiOSかisIOSかは悩みどころ。。

このDeviceUtilはこの案件の実装とは 完全に独立している のでほかの案件でも使えますね！

3.コンポーネントの分割

この案件ではModelに相当するような（ajaxで値を取得して表示させたり）みたいなことはしなくてもよいので、
Viewだけについて考えます。

この案件ではViewに相当するものは３つあります。

  • 全体を管理するView
  • スクロールを管理するView
  • サイドメニューを管理するView

それぞれをもし自分がコンポーネント化するとしたら

//スクロールを管理するView
chizunavi.ScrollView = function(){};

//サイドメニューを管理するView
chizunavi.SideMenuView = function(){};

//全体を管理するView
//ScrollViewもSideMenuViewもこのAppViewの中にあるイメージ
chizunavi.AppView = function(){};

のような感じにすると思います。
そしてそれぞれのViewの中でそれぞれの責任をお互いに侵さないように実装します。

今回使っている$.fn.fullpagerはScrollViewの内部で拡張します。

var p = AppView.prototype;

//初期化関数に初期化処理はまとめる
p.initialize = function(){
    $.fn.fullpage.setAutoScrolling = this.setAutoScrolling;
};

p.setAutoScrolling = function(event){
    //なんか処理
};

こうすることでScrollViewはスクロールに関する責任だけを全うできます。そのうえ、デバッグ的にも問題箇所の特定がしやすくなるのでぜひともやってほしいです。

またもしもスクロール内で表示している各セクションのViewを管理したい場合は以下みたいに基本的なViewをつくると,
同じような処理を共通メソッドで扱えて便利です。

//SectionView.js

var SectionView = function(){};

var p = SectionView.prototype;

p.show = function(){};

/******************************************/

//TopSectionView.js

var TopSectionView = function(){};

var p = TopSectionView.prototype = new SectionView();

//p.show()を宣言しなくてもTopSectionView.show()が実行できる

パフォーマンスチューニング

  • -webkit-transformアニメーション
  • PNG圧縮
  • JSミニファイ
0

Register as a new user and use Qiita more conveniently

  1. You get articles that match your needs
  2. You can efficiently read back useful information
  3. You can use dark theme
What you can do with signing up
Sign upLogin

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?