snippetjavascriptMinor
Create new elements, simple notification constructor, send data when a notification is dismissed, etc
Viewed 0 times
simplenewelementscreatedismisseddataetcconstructorwhensend
Problem
This is not ordinary question but it's also not opinion-based, it's more about the standards and the right way of writing JS code. I admit, I'm a total noob in JavaScript, I do not have experience with this language but since it is necessary for the project I'm working on - I had no other choice but to learn a bit about it :P The most basic of things, took me quite some time so I just want to know if the code is written the right way or not. That way I could understand what I've done wrong and learn from it. The reason I'm asking whether it's right or not, instead of checking it myself is because everything work as it supposed to so I've no idea whether I've done it the right way or at least in a valid way.
My JavaScript code;
`if(isMobile) {
var currentProtocol = window.location.protocol + '\/\/';
var currentHostname = siteHostname;
var currentPath = window.location.pathname;
window.location.href = currentProtocol+mobileHostname+currentPath;
}
function goBack() {
window.history.back()
}
$('a[aria-controls="*"]').click(function (e) {
e.preventDefault();
})
$('.close').click(function (e) {
e.preventDefault();
})
$(function (e) {
$('[data-toggle="tooltip"]').tooltip();
$('[data-toggle="popover"]').popover();
$('[aria-controls="*"]').tab('show');
})
function hideCookieNotice() {
$('#cookie-notice').addClass('hidden');
}
$('#cookie-notice-close').click(function (e) {
document.cookie='acgcookie=0;path=.'+siteHostname+';expires='+(new Date(+new Date()+157785e6)).toGMTString();
hideCookieNotice();
})
if (document.cookie.search('acgcookie=0') !== -1) {
hideCookieNotice();
}
var elementHelper = {
/* ==========================================================================
Block Types and Elements
========================================================================== */
notificationDiv: '',
/* ==========================================================================
Buttons
===================================================
My JavaScript code;
`if(isMobile) {
var currentProtocol = window.location.protocol + '\/\/';
var currentHostname = siteHostname;
var currentPath = window.location.pathname;
window.location.href = currentProtocol+mobileHostname+currentPath;
}
function goBack() {
window.history.back()
}
$('a[aria-controls="*"]').click(function (e) {
e.preventDefault();
})
$('.close').click(function (e) {
e.preventDefault();
})
$(function (e) {
$('[data-toggle="tooltip"]').tooltip();
$('[data-toggle="popover"]').popover();
$('[aria-controls="*"]').tab('show');
})
function hideCookieNotice() {
$('#cookie-notice').addClass('hidden');
}
$('#cookie-notice-close').click(function (e) {
document.cookie='acgcookie=0;path=.'+siteHostname+';expires='+(new Date(+new Date()+157785e6)).toGMTString();
hideCookieNotice();
})
if (document.cookie.search('acgcookie=0') !== -1) {
hideCookieNotice();
}
var elementHelper = {
/* ==========================================================================
Block Types and Elements
========================================================================== */
notificationDiv: '',
/* ==========================================================================
Buttons
===================================================
Solution
Because the definition of the variables defined with
If your target environment supports
In most cases you want to use the
For
Writing
Does not make much sense. There it would be better to either not assign anything
You should limit the variable definition to the scope where you use them.
As of that you should move the
Make sure that you always define the variables that you use in your functions using
Try to avoid to use the same variable names
Keep a consistent way of defining your functions. Within your
In
The final rework of the code could then look like this. It for sure can still be made better, but now you have more structure in it:
```
var elementHelper = {
// Block Types and Elements
notificationDiv: '',
// Buttons
closeButton: '',
// Closing Tags
closeDiv: '',
// Icon Types
icon: function(type) {
function newIcon(type) {
switch( type ) {
case 'info':
return 'info-circle';
case 'info2':
return 'info-circle2';
default:
return 'Error';//return type; depends on what you original expected
}
}
function iconConstruct(icon) {
return '';
}
var theIcon = newIcon(type);
return iconConstruct(theIcon);
}
};
// ======= code to be executeded
if (isMobile) {
redirectMobile( window.location.protocol + '\/\/',
siteHostname,
window.location.pathname );
}
$('a[aria-controls="*"]').click(function(e) {
e.preventDefault();
});
$('.close').click(function(e) {
e.preventDefault();
});
$(function() {
$('[data-toggle="tooltip"]').tooltip();
$('[data-toggle="popover"]').popover();
$('[aria-controls="*"]').tab('show');
});
$('#cookie-notice-close').click(function(e) {
document.cookie = 'acgcookie=0' +
';path=.' + siteHostname +
';expires=' + (new Date(+new Date() + 157785e6)).toGMTString();
hideCookieNotice();
});
if (document.cookie.search('acgcookie=0') !== -1) {
hideCoo
var are always hoisted to the top of the function block, it is better to list them in the beginning of the function and not somewhere in between, otherwise it is not easy to see which variable names are bound to the current scope mdn: var hoisting).If your target environment supports
let you might use that one instead of var (mdn: let).In most cases you want to use the
=== and !== operator and not the == or !=, there are for sure situations where == will be ok but most of the time you should use the strict comparison (Does it matter which equals operator (== vs ===) I use in JavaScript comparisons?).For
$.ajax I would use the promise capabilities .then() and .fail(). The reason I would do that is that you separate the parameter that are used to create the request from the actions that should be don on failure and success.$.ajax({
type: 'POST',
url: 'test.php',
data: form_data
})
.then(function() {
outputLog = 'Successfully changed site language to ' + siteLang;
printLog(outputLog);
})
.fail(function() {
outputMessage = 'Error, failed to change site language to ' + siteLang + '.Please contact Support and report this error.';
outputLog = 'Error';
newNotification(outputMessage);
printLog(outputLog);
});Writing
var form_data = ''; and then later directly changing it to:form_data = {
username: userName,
csrfToken: csrf,
type: e[0],
id: e[1],
is_ajax: '1'
};Does not make much sense. There it would be better to either not assign anything
var form_data; or maybe directly write it that way:var form_data = {
username: userName,
csrfToken: csrf,
type: e[0],
id: e[1],
is_ajax: '1'
},
outputMessage = '',
outputLog = '';You should limit the variable definition to the scope where you use them.
As of that you should move the
outputMessage and outputLog to the then and fail callback functions, because you only use them inside of them nowhere else.Make sure that you always define the variables that you use in your functions using
var, otherwise you will pollute the global/outer scope, which might result in unexpected behaviour: function newNotification(outputMessage) {
var html = elementHelper.notificationDiv + elementHelper.closeButton + elementHelper.icon('info') + ' ' + outputMessage + '.' + elementHelper.closeDiv;
$(html).hide().appendTo('#side-block').fadeIn('slow');
}Try to avoid to use the same variable names
e, here you could just remove the e because you don't use it, otherwise it might be mixed with the var e you defined inside. A different naming would be even more important if you would use the parameter e somewhere.$(function(e) {
$(".close.send-data").click(function(n) {
var e = new Array($(this).data("type"), $(this).data("id"));
sendDismiss(e);
});
});Keep a consistent way of defining your functions. Within your
icon: function (type) { you define newIcon at the beginning and iconConstruct at the end. In the given code it is is obvious, but this might result problems in larger code.newIcon has a return statement return type; at the end that is never called, because of the return 'Error';.In
sendDismiss it is not known what e is because the name is not self-explaining, especially the e[0], e[1] in combination with the type, id it can be guessed, but it is still a bad design.The final rework of the code could then look like this. It for sure can still be made better, but now you have more structure in it:
```
var elementHelper = {
// Block Types and Elements
notificationDiv: '',
// Buttons
closeButton: '',
// Closing Tags
closeDiv: '',
// Icon Types
icon: function(type) {
function newIcon(type) {
switch( type ) {
case 'info':
return 'info-circle';
case 'info2':
return 'info-circle2';
default:
return 'Error';//return type; depends on what you original expected
}
}
function iconConstruct(icon) {
return '';
}
var theIcon = newIcon(type);
return iconConstruct(theIcon);
}
};
// ======= code to be executeded
if (isMobile) {
redirectMobile( window.location.protocol + '\/\/',
siteHostname,
window.location.pathname );
}
$('a[aria-controls="*"]').click(function(e) {
e.preventDefault();
});
$('.close').click(function(e) {
e.preventDefault();
});
$(function() {
$('[data-toggle="tooltip"]').tooltip();
$('[data-toggle="popover"]').popover();
$('[aria-controls="*"]').tab('show');
});
$('#cookie-notice-close').click(function(e) {
document.cookie = 'acgcookie=0' +
';path=.' + siteHostname +
';expires=' + (new Date(+new Date() + 157785e6)).toGMTString();
hideCookieNotice();
});
if (document.cookie.search('acgcookie=0') !== -1) {
hideCoo
Code Snippets
$.ajax({
type: 'POST',
url: 'test.php',
data: form_data
})
.then(function() {
outputLog = 'Successfully changed site language to ' + siteLang;
printLog(outputLog);
})
.fail(function() {
outputMessage = 'Error, failed to change site language to ' + siteLang + '.<br />Please contact Support and report this error.';
outputLog = 'Error';
newNotification(outputMessage);
printLog(outputLog);
});form_data = {
username: userName,
csrfToken: csrf,
type: e[0],
id: e[1],
is_ajax: '1'
};var form_data = {
username: userName,
csrfToken: csrf,
type: e[0],
id: e[1],
is_ajax: '1'
},
outputMessage = '',
outputLog = '';function newNotification(outputMessage) {
var html = elementHelper.notificationDiv + elementHelper.closeButton + elementHelper.icon('info') + ' ' + outputMessage + '.' + elementHelper.closeDiv;
$(html).hide().appendTo('#side-block').fadeIn('slow');
}$(function(e) {
$(".close.send-data").click(function(n) {
var e = new Array($(this).data("type"), $(this).data("id"));
sendDismiss(e);
});
});Context
StackExchange Code Review Q#110836, answer score: 2
Revisions (0)
No revisions yet.