回答編集履歴

3 箇条書きがそもそもしすぎてたので修正

miyabi-sun

miyabi-sun score 19346

2017/11/10 10:42  投稿

レビューしました。
- そもそもjQueryの`$(fn)`自体が`window.onload = fn`の完全上位互換みたいなものです。
おすすめは普通に変数宣言してそれに突っ込む方法です。
- ところで`fixBox`ってなんですか?未定義でエラーが出そうです。
- `fixBoxL`も未定義です。`fix`で値が変わるのは`BoxL`なのでBoxLを一生懸命更新しても何も変わりませんよ。
- jQueryオブジェクトは慣習的に変数名の頭に$を付ける人が多いです
- jQueryの`$(fn)`自体が`window.onload = fn`の完全上位互換みたいなもの
おすすめは普通に変数宣言してそれに突っ込む方法
内部で宣言されている変数は全てfix外で管理し、fixは値を書き換えるのに集中
- `fixBox`や`fixBoxL`が未定義だね。fix関数の中身だから頭にfix付けてみただけ?
- jQueryオブジェクトは慣習的に変数名の頭に$を付ける人が多い
---
上記を踏まえてコードをリファクタリングしました。
```JavaScript
$(function() {
   var $Box = $('.sidebar'),
       offset, BoxL, BoxT;
   var fix = function() {
       offset = $Box.offset();
       BoxL = offset.left;
       BoxT = offset.top;
   }
   var render = function() {
       if ($(window).scrollTop() < offset.top - 20) {
           $Box.css({
               position: 'absolute',
               top: 'inherit',
               left: 'inherit',
           });
       } else {
           $Box.css({
               position: 'fixed',
               top: '20px',
               left: BoxL,
           });
       }
   }
   var refresh = function() {
     fix();
     render();
   }
   refresh();
   $(window).on('resize', refresh);
   $(window).on('scroll', render);
});
```
2 関数の動作修正

miyabi-sun

miyabi-sun score 19346

2017/11/10 10:39  投稿

レビューしました。
- そもそもjQueryの`$(fn)`自体が`window.onload = fn`の完全上位互換みたいなものです。
`window.onload = fn`は後勝ちで上書きされ、前の関数が動作しなくなる害悪設定です。二度と使う事はないので忘れましょう。
- 単なる関数は[ホイスト(巻き上げ)](https://qiita.com/tomcky/items/988fc5f56d019e9dc097)に気をつけましょう
おすすめは普通に変数宣言してそれに突っ込む方法です。
- fixは外だしします、内部で宣言されている変数は全てfix外で管理し、fixは値を書き換えるのに集中しましょう。
- ところで`fixBox`ってなんですか?未定義でエラーが出そうです。
- `fixBoxL`も未定義です。`fix`で値が変わるのは`BoxL`なのでBoxLを一生懸命更新しても何も変わりませんよ。
- `$(window).on('resize', fn)`の第二引数に無名関数を作って渡していますが、thisを使わない設計なのでそのままfixを渡してしまったほうが簡潔です。
- jQueryオブジェクトは慣習的に変数名の頭に$を付ける人が多いです
- オブジェクトのケツカンマはChromeでは動きますが、IEでは動作しなかったような…まぁ優先度低ですね
- そもそもオフセット値を書き換えたら画面更新しないと駄目でしょ。  
---
上記を踏まえてコードをリファクタリングしました。
※`fixBox`、`fixBoxL`は$BoxとBoxLなんだろうなと解釈したので変数名を変えました。
```JavaScript
$(function() {
   var $Box = $('.sidebar'),
       offset, BoxL, BoxT;
   var fix = function() {
       offset = $Box.offset();
       BoxL = offset.left;
       BoxT = offset.top;
   }
   fix();
   $(window).on('resize', fix);
   $(window).on('scroll', function() {
   var render = function() {
       if ($(window).scrollTop() < offset.top - 20) {
           $Box.css({
               position: 'absolute',
               top: 'inherit',
               left: 'inherit',
           });
       } else {
           $Box.css({
               position: 'fixed',
               top: '20px',
               left: BoxL,
           });
       }
   });
   }
   var refresh = function() {
     fix();
     render();
   }
   refresh();
   $(window).on('resize', refresh);
   $(window).on('scroll', render);
});
```
1 文章校正

miyabi-sun

miyabi-sun score 19346

2017/11/10 10:36  投稿

レビューしました。
- そもそもjQueryの`$(fn)`自体が`window.onload = fn`の完全上位互換みたいなので書き方がおかしいですね。
- そもそもjQueryの`$(fn)`自体が`window.onload = fn`の完全上位互換みたいなものです。
`window.onload = fn`は後勝ちで上書きされ、前の関数が動作しなくなる害悪設定です。二度と使う事はないので忘れましょう。
- 単なる関数は[ホイスト(巻き上げ)](https://qiita.com/tomcky/items/988fc5f56d019e9dc097)に気をつけましょう
おすすめは普通に変数宣言してそれに突っ込む方法です。
- fixは外だしします、内部で宣言されている変数は全てfix外で管理し、fixは値を書き換えるのに集中しましょう。
- ところで`fixBox`ってなんですか?未定義でエラーが出そうです。
- `fixBoxL`も未定義ですが、これは`BoxL`と同一ですか?
(多分コードの外側で定義してあるjQueryオブジェクトなんだろうと思いますが)
- `fixBoxL`も未定義です。`fix`で値が変わるのは`BoxL`なのでBoxLを一生懸命更新しても何も変わりませんよ。
- `$(window).on('resize', fn)`の第二引数に無名関数を作って渡していますが、thisを使わない設計なのでそのままfixを渡してしまったほうが簡潔です。
- jQueryオブジェクトは慣習的に変数名の頭に$を付ける人が多いです
- オブジェクトのケツカンマはChromeでは動きますが、IEでは動作しなかったような…まぁ優先度低ですね
---
上記を踏まえてコードをリファクタリングしました。
※`fixBox`、`fixBoxL`はそのままなのでこのコードはエラーが出て動作しません。
修正して試してみてください。
※`fixBox`、`fixBoxL`は$BoxとBoxLなんだろうなと解釈したので変数名を変えました。
```JavaScript
$(function() {
   var $Box = $('.sidebar'),
       offset, BoxL, BoxT;
   var fix = function() {
       offset = $Box.offset();
       BoxL = offset.left;
       BoxT = offset.top;
   }
   fix();
   $(window).on('resize', fix);
   $(window).on('scroll', function() {
       if ($(window).scrollTop() < offset.top - 20) {
           fixBox.css({
           $Box.css({
               position: 'absolute',
               top: 'inherit',
               left: 'inherit',
           });
       } else {
           fixBox.css({
           $Box.css({
               position: 'fixed',
               top: '20px',
               left: fixBoxL,
               left: BoxL,
           });
       }
   });
});
```

思考するエンジニアのためのQ&Aサイト「teratail」について詳しく知る