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, 2016 at 7:51
  • 1
    \$\begingroup\$ @Tushar you should try moving this to an answer rather than a comment \$\endgroup\$
    – Tolani
    Aug 17, 2016 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\$
    – Tolani
    Aug 17, 2016 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\$ Aug 17, 2016 at 9:01

1 Answer 1

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\$ Aug 17, 2016 at 9:07
  • \$\begingroup\$ @FelipeFelixdeLuca Sorry, instead of width, css should be used. Please check updated code. \$\endgroup\$
    – Tushar
    Aug 17, 2016 at 9:12
  • \$\begingroup\$ This looks better Tushar \$\endgroup\$
    – Tolani
    Aug 17, 2016 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\$ Aug 17, 2016 at 9:24
  • \$\begingroup\$ @FelipeFelixdeLuca Update onclick="openNav()" to onclick="toggleNav(true)" \$\endgroup\$
    – Tushar
    Aug 17, 2016 at 9:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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