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>
asked Aug 17, 2016 at 5:34
\$\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\$ Commented Aug 17, 2016 at 7:51
  • 1
    \$\begingroup\$ @Tushar you should try moving this to an answer rather than a comment \$\endgroup\$ Commented 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\$ Commented 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\$ Commented 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);
 }
}
answered Aug 17, 2016 at 9:01
\$\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\$ Commented Aug 17, 2016 at 9:07
  • \$\begingroup\$ @FelipeFelixdeLuca Sorry, instead of width, css should be used. Please check updated code. \$\endgroup\$ Commented Aug 17, 2016 at 9:12
  • \$\begingroup\$ This looks better Tushar \$\endgroup\$ Commented 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\$ Commented Aug 17, 2016 at 9:24
  • \$\begingroup\$ @FelipeFelixdeLuca Update onclick="openNav()" to onclick="toggleNav(true)" \$\endgroup\$ Commented Aug 17, 2016 at 9:25

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.