Return to Answer

• 1

• 1.1k
• 6
• 13

• 1.1k
• 6
• 13

I agree with all of your points you made above about what you need to do to this code. I noticed a few other things I think would help improve your code. Most of these are pretty straightforward.

First, would be to encapsulate your code in an IIFE. This will create a closure and ensure you are working in your own private scope. If you aren't sure what a closure is, you can read more about them. Since you are using jQuery, you can pass that into your closure as $ which will allow you to reference it as $ safely throughout your code.

Also, you should move the 'use strict'; declaration out of the global scope because this can cause unexpected side effects with other code (you can read all about strict mode from MDN. You should include it in your private scope. Here is how I would recommend you start your code:

(function( ,ドル undefined ){
 'use strict';
 // your code will go here
})( jQuery );

You could also pass in the window and document objects as well for a minor performance increase, but it isn't as necessary.

The next recommendation is to cache all of your selectors. Accessing the DOM is one of jQuerys slowest methods so you want to do this as little as possible. I typically create a single function to do this. I also recommend as little code in document.ready as possible. Again, I create a single function call for that. As an example:

(function( ,ドル undefined ){
 'use strict';
 var $reorderingToggle, $reorderingCancelBtn , $reorderingSaveBtn;
 function getSelections() {
 $reorderingToggle = $('#reordering-toggle');
 $reorderingCancelBtn = $('#reordering-cancel-button');
 $reorderingSaveBtn = $('#reordering-save-button');
 } 
 function init() {
 getSelections();
 }
 $(function() {
 init();
 });
})( jQuery );

Note: You obviously don't have to use a single function in document.ready, it's just my preferred pattern.

After the getSelections function runs, you can reference these objects by the variable name everywhere else in your code.

Next, I would use as few anonymous functions as you can. This will help you with debugging your code later on. So continuing with the same code from above I would add a defineEvents function similar to below:

(function( ,ドル undefined ){
 'use strict';
 var $reorderingToggle, $reorderingCancelBtn , $reorderingSaveBtn;
 var reorderingToggleOptions ={
 on: "Cancel Reorder",
 off: "Reorder Limits",
 size: "small"
 };
 function init() {
 getSelections();
 defineEvents();
 }
 function defineEvents() {
 $reorderingToggle
 .bootstrapToggle( reorderingToggleOptions )
 .on('change', changeReorderToggle);
 }
 function changeReorderToggle() {
 //your code here
 }

I also created a variable to hold the options for the bootstrap toggle and added this to the top of the code. That way, you can easily find and change these properties without having to search through the code. Also, in the above code, I use jQuerys ability to chain functions together. I don't mind using the chaining as long as the code stays readable. If it starts to get too long or unmanageable, you can always start again with your variable (or if you just prefer this style):

$reorderingToggle.bootstrapToggle( reorderingToggleOptions );
$reorderingToggle.on( 'change', changeReorderToggle);

Another thing to do would be to namespace your events. That way your change code wouldn't interfere with any other change code. You namespace your events just by adding the namespace to the event name. So the above change code could be this instead:

// in this example, hwt would be your event namespace
$reorderingToggle.on( 'change.hwt', changeReorderToggle);

That way you can remove just your change event code with:

$reorderingToggle.off( 'change.hwt');

The next thing I would do is set up your own namespace for your code. Since this for testing hardware, use something hwt (like above) or something more specific like hardwareTests. You can define this object and add any functions that you would want exposed as public methods. Any functions you don't add to this object would then be private methods. For example:

(function( ,ドル undefined ) {
 'use strict';
 // the rest of the code
 window.hardwareTests = {
 init : init
 };
 $(function(){
 hardwareTests.init();
 });
})( jQuery );

Here we added our namespace to the global window object and added the init function to it. Again, you should any functions you want the user to be able to execute. A good example might also be a destroy method that can be called to remove your code. Any function not added to the object can only be called by your code inside of the IIFE. So, in this example, the defineEvents function would not be available to be called.

One of the other things I see in the code is manipulating an element using the css method. If possible, I would recommend adding/removing CSS classes instead of this whenever possible. Obviously some of these you are doing calculations in JS that you can't do in CSS but, for everything else, use a class instead. This helps keep your separation of concerns (CSS for styling and JavaScript for interaction).

Also, I see a lot of parent() calls and even parent().parent(). This is a very brittle approach. What happens if your DOM changes? Your code breaks. If you need to reference something, add a class or an id to it and use that to do your selection. It will save you headaches later on.

The last thing I will mention is the need to DRY your code. There are numerous places where you have the same (or very similar) pieces of code. As a general rule of thumb, if you are typing in the same line of code more than once, you should consider making it a stand alone function. For example, pleaseSave and revert are basically the same function. Why not create a single function that handles both instances?

function notifyUser( title, message, type, placement ) {
 $.notify({
 title: title,
 message: message
 }, {
 type: 'type,
 placement: {
 from: placement
 }
 }); 
}

If you look through your code I'm sure can find other examples. I hope that helps. Feel free to leave a comment and best of luck!