1
\$\begingroup\$

I have the following piece of code which hides an element when a specified date was reached. I would like to get some tips about do's and don'ts.

Specifically, I'm interested in:

  • improvements brought to this code
  • avoid bad practices

And whatever you guys consider I should be careful about.

var Timer = (function(){
    var $el = $('#element');

    function count() {
        setInterval(function() {
            check();
        }, 1000);
    }

    function hide() {
        $el.hide();
    }

    function check() {
        var currentDate = new Date();
        var endingDate = new Date("July 25, 2016 11:06:00");

        if (currentDate.getTime() >= endingDate.getTime()) { 
            hide();
        }
    }


    return {
        count: count  
    };
})();

Timer.count();
\$\endgroup\$
  • \$\begingroup\$ Welcome to codereview! What should we do with this code ? Please edit your question and add some information about what the code does and what would you like to improve about it. You can also look in our help section at How do I ask a good question? Thanks \$\endgroup\$ – Grajdeanu Alex. Jul 18 '16 at 14:20
  • \$\begingroup\$ @Dex'ter I would like suggestions on how I can make it better. \$\endgroup\$ – Christopher Hill Jul 18 '16 at 14:22
  • \$\begingroup\$ You should include more information in the queston's body \$\endgroup\$ – Siobhan Jul 18 '16 at 14:39
  • 1
    \$\begingroup\$ There is no way this code works. CountDown is an intervalId so it won't have a run function. You've either copied your code incorrectly or haven't tested it (come back when it's working though!) \$\endgroup\$ – RobH Jul 18 '16 at 14:47
  • 1
    \$\begingroup\$ @RobH Ok, well... I updated it. \$\endgroup\$ – Christopher Hill Jul 18 '16 at 15:09
1
\$\begingroup\$

My notes:

  • There is no need to create a function that calls check, you can just pass check
  • There is no need for function hide, you only call it once, and it only has 1 line of code, I would inline that function
  • Very minor, but there is no need to declare endingDate inside check, I would declare it once outside of the function
  • You are using a revealing pattern, I would use a simple self executing pattern. On the whole if you expose 1 function that you will run only once, use a self executing function.
  • Once the date is passed you will be hiding that element non-stop every second, that sounds like overkill
  • I abhor anonymous functions, choose a meaningful name, and you can spare a line of comment ;)

My approach with these notes would be

    (function HideElementWhenTimeComes (){
      var $el = $('#element'),
          endingDate = new Date("July 25, 2016 11:06:00"),
          intervalID;

      function check() {
        var currentDate = new Date();
            if (currentDate.getTime() >= endingDate.getTime()) { 
              $el.hide();
              clearInterval(intervalID);
            }
      }

      intervalID = setInterval(check , 1000 );

    })();
\$\endgroup\$

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.