HiveBrain v1.2.0
Get Started
← Back to all entries
snippetjavascriptMinor

Create new elements, simple notification constructor, send data when a notification is dismissed, etc

Submitted by: @import:stackexchange-codereview··
0
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
===================================================

Solution

Because the definition of the variables defined with 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.