3
\$\begingroup\$

So far, I've been using codes based on solutions other people wrote, and I'm trying to learn by modifying those codes. My problem now is that my code ended up too messy (a lot of script tags, jquery mixed with javascript, etc). I would like to know what should I do to clean this mess. Since I'm a beginner, I'm not looking for something radical, but something that would make my code understandable and organized.

This is the page I'm building: website

     <script>
        (function() {
            "use strict";
            var toggles = document.querySelectorAll(".c-hamburger");
            for (var i = toggles.length - 1; i >= 0; i--) {
                var toggle = toggles[i];
                toggleHandler(toggle);
            };
            function toggleHandler(toggle) {
                toggle.addEventListener( "click", function(e) {
                    e.preventDefault();
                (this.classList.contains("is-active") === true) ? this.classList.remove("is-active") : this.classList.add("is-active");
                });
            }
        })();
    </script>
    <script>
        $(document).ready(function(){
            $( "#btnMobileMenu" ).click(function() {
                $( "#mbMenu" ).toggle( "slow", function() {

                });
            });
            var heig = $(document).height();
            var wid = $(document).width();
            $( "#mbMenu" ).css({
                height: heig,
                width: wid
            }); 
        });
    </script>
    <script>
        //IIF to avoid polluting global namespace
        (function() {

            $(function() {
            //S1 - add click handler to each menu item
                $(".anchorLink").each(function(k, v) {
                    $(v).click(function(e) {
                        //S2 - build target div id using hash from clicked menu item
                        var targetId = 'target-' + e.originalEvent.currentTarget.hash.slice(1);
                        //S3 - scroll document to top offset of target div
                        $('body').scrollTop($('#' + targetId).offset().top-200);
                        //S4 - apply menu rendering effects _without_ taking deltaY into account
                        myEffectsClick(e);
                    });
                });

                $(window).on('wheel', function(e) {
                    myEffectsScroll(e);
                });
            });

            //no deltaY since we're not scrolling
            function myEffectsClick(e) {
                var windowScrollTop = $(this).scrollTop();
                //'reset' menu as if we had scrolled up
                scrollUp(windowScrollTop);
                //add any applicable effects based on current position
                scrollDown(windowScrollTop);
            }
            //apply effects when scrolling

            function myEffectsScroll(e) {
                var delta = e.originalEvent.deltaY;
                var windowScrollTop = $(this).scrollTop();

                if (delta > 0) {
                    //scroll-down
                    scrollDown(windowScrollTop);
                } else {
                    //scroll-up
                    scrollUp(windowScrollTop);
                }
            }

            function scrollUp(windowScrollTop) {
                if (windowScrollTop < 350) {
                    $(".two").css("border-top-color", "#999999").animate({
                        width: '25px'
                    }, 100);
                }
                if (windowScrollTop < 750) {
                    $(".three").css("border-top-color", "#999999").animate({
                        width: '25px'
                    }, 100);
                }
                if (windowScrollTop < 1150) {
                    $(".four").css("border-top-color", "#999999").animate({
                        width: '25px'
                    }, 100);
                }
                if (windowScrollTop < 1500) {
                    $(".one, .two, .three, .four, .five").css("border-top-color", "#fff");
                    $(".navbar-nav li a, #navRight a, #footer, #footer a").css("color","#fff");
                    $(".five").animate({
                        width: '25px'
                    }, 100);
                    jQuery("body").animate({
                        backgroundColor: "#003333"
                    }, 100 );
                }
            }

            function scrollDown(windowScrollTop) {
                if (windowScrollTop > 0) {
                    $(".one").css("border-top-color", "#fff").animate({
                        width: '50px'
                    }, 100);
                }
                if (windowScrollTop > 350) {
                    $(".two").css("border-top-color", "#fff").animate({
                        width: '50px'
                    }, 100);
                }
                if (windowScrollTop > 750) {
                    $(".three").css("border-top-color", "#fff").animate({
                        width: '50px'
                    }, 100);
                }
                if (windowScrollTop > 1150) {
                    $(".four").css("border-top-color", "#fff").animate({
                        width: '50px'
                    }, 100);
                }
                if (windowScrollTop > 1500) {
                    $(".one, .two, .three, .four, .five").css("border-top-color", "#999999");
                    $(".navbar-nav li a, #navRight a, #footer, #footer a").css("color","#000");
                    $(".five").animate({
                        width: '50px'
                    }, 100);
                    jQuery("body").animate({
                        backgroundColor: "#CCCCCC"
                    }, 100 );
                }
            }
        }());
    </script>
    <script>
        $(document).ready(function () {
            $(window).scroll(function () {
                $('#navRight .rotate').toggleClass("up", ($(window).scrollTop() > 100));
            });
        });
    </script>
    <script>
        $(function() {
            $('a[href*="#"]:not([href="#"])').click(function() {
            if (location.pathname.replace(/^\//,'') == this.pathname.replace(/^\//,'') && location.hostname == this.hostname) {
              var target = $(this.hash);
              target = target.length ? target : $('[id=' + this.hash.slice(1) +']');
              if (target.length) {
                $('html, body').animate({scrollTop: target.offset().top-200}, 1000);
                return false;
              }
            }
          });
        });
    </script>
    <script>
        function openNav() {
            document.getElementById("questionNav").style.width = "100%";
            document.getElementById("questionNav").style.opacity = "1";
        }
        function closeNav() {
            document.getElementById("questionNav").style.width = "0%";
            document.getElementById("questionNav").style.opacity = "0";
        }
    </script>
    <script>
        $('#result').on('click', function() {
            var checkedOptions = $(":checkbox:checked").length*10+"%";
            $('#resultPercent').text(checkedOptions);
        });
    </script>
\$\endgroup\$
4
  • 1
    \$\begingroup\$ Instead of var heig = $(document).height(); var wid = $(document).width(); $("#mbMenu").css({ height: heig, width: wid });, Use CSS: #mbMenu { width: 100vw; height: 100vh; } \$\endgroup\$ – Tushar Aug 17 '16 at 7:51
  • 1
    \$\begingroup\$ @Tushar you should try moving this to an answer rather than a comment \$\endgroup\$ – Siobhan Aug 17 '16 at 8:53
  • 1
    \$\begingroup\$ Please could you provide us a bit of html code that corresponds to classes and ids you used in your javascript \$\endgroup\$ – Siobhan Aug 17 '16 at 9:00
  • \$\begingroup\$ Hello @TolaniJaiye-Tikolo! The link I'm pointing in my question leads to the page I'm building. You can get the html there. \$\endgroup\$ – Felipe Felix de Luca Aug 17 '16 at 9:01
2
\$\begingroup\$
  1. Setting the dimenstions - Use CSS

    var heig = $(document).height();
    var wid = $(document).width();
    $("#mbMenu").css({
        height: heig,
        width: wid
    });
    

    Instead of this, use viewport units

    #mbMenu {
        width: 100vw;
        height: 100vh;
    }
    
  2. Instead of Core Javascript, use jQuery

    var toggles = document.querySelectorAll(".c-hamburger");
    for (var i = toggles.length - 1; i >= 0; i--) {
        var toggle = toggles[i];
        toggleHandler(toggle);
    };
    
    function toggleHandler(toggle) {
        toggle.addEventListener("click", function (e) {
            e.preventDefault();
            (this.classList.contains("is-active") === true) ? this.classList.remove("is-active"): this.classList.add("is-active");
        });
    }
    

    can be written as

    $('.c-hamburger').click(function(e) {
        e.preventDefault();
        $(this).toggleClass('is-active');
    });
    
  3. openNav and closeNav can be combined into

    function toggleNav(open) {
        $('#questionNav').css('width', open ? '100%' : '0');
    }
    

    where open - boolean can be passed to the function to open or close. There is no need to change the opacity as zero-width elements are not visible.

  4. There is no need to iterate over multiple elements and bind the event individually. jQuery does that when working on selectors that selects multiple elements.

    $('.anchorLink').click(function(e) {
        var targetId = 'target-' + this.hash.slice(1);
        $('body').scrollTop($('#' + targetId).offset().top - 200);
        myEffectsClick(e);
    });
    
  5. The events bound on window are not needed to be wrapped inside ready(). Also, multiple statements can be chained and function reference can be used directly as handler.

    $(window).scroll(function() {
        $('#navRight .rotate').toggleClass("up", $(window).scrollTop() > 100);
    }).on('wheel', myEffectsScroll);
    
  6. As the animate callback is not needed, that can be removed.

    $("#mbMenu").toggle("slow", function() {
    
    });
    

    should be

    $("#mbMenu").toggle("slow");
    
  7. scrollUp and similarly scrollDown can be written as following. Note that similar code is reused and else if is used.

    function scrollUp(windowScrollTop) {
        if (windowScrollTop < 1150) {
            var $el;
            if (windowScrollTop < 350) {
                $el = $('.two');
            } else if (windowScrollTop < 750) {
                $el = $('.three');
            } else if (windowScrollTop < 1150) {
                $el = $('.four');
            }
    
            $el.css('border-top-color', '#999').animate({
                width: '25px'
            }, 100);
        } else if (windowScrollTop < 1500) {
            $('.one, .two, .three, .four, .five').css('border-top-color', '#fff');
            $('.navbar-nav li a, #navRight a, #footer').css('color', '#fff');
    
            $('.five').animate({
                width: '25px'
            }, 100);
    
            $('body').animate({
                background: '#033'
            }, 100);
        }
    }
    
\$\endgroup\$
9
  • \$\begingroup\$ thank you very much for the help! I still didn't tried all you wrote, but the numer 3 is not working here \$\endgroup\$ – Felipe Felix de Luca Aug 17 '16 at 9:07
  • \$\begingroup\$ @FelipeFelixdeLuca Sorry, instead of width, css should be used. Please check updated code. \$\endgroup\$ – Tushar Aug 17 '16 at 9:12
  • \$\begingroup\$ This looks better Tushar \$\endgroup\$ – Siobhan Aug 17 '16 at 9:12
  • \$\begingroup\$ I don't know why, but I can't make number 3 work. I just deleted openNav and closeNav and inserted your code. I have a button doing the action, do I have to change something in it too? <a id="testBtn" class="text-center" onclick="openNav()">CHECK</a> \$\endgroup\$ – Felipe Felix de Luca Aug 17 '16 at 9:24
  • \$\begingroup\$ @FelipeFelixdeLuca Update onclick="openNav()" to onclick="toggleNav(true)" \$\endgroup\$ – Tushar Aug 17 '16 at 9:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.