patternjavascriptMinor
Validating input variables
Viewed 0 times
inputvariablesvalidating
Problem
I have written the following JavaScript code and want to ask if it is written as per best practices. Please recommend any changes to be made in the code. The code is taking a few variables as input and then validating if all of them have some value and not blank/null.
var var1 = false;
var missingVAL = false;
var validationMsg = "";
host = scriptletContext.get("host");
user = scriptletContext.get("user");
serverID = scriptletContext.get("serverId");
runID = scriptletContext.get("runId");
if (host == false)
{
validationMsg = "serverHostName (serverHostName) is missing.";
var1 = true;
}
if (user == false)
{
if (var1 == true)
{
validationMsg = validationMsg + " Requestor user name (requestorUserName) is missing.";
}
else
{
validationMsg = "Requestor user name (requestorUserName) is missing.";
}
var1 = true;
}
if (serverID == false)
{
if (var1 == true)
{
validationMsg = validationMsg + " (vascoServerId) is missing.";
}
else
{
validationMsg = "(vascoServerId) is missing.";
}
var1 = true;
}
if (runID == false)
{
if (var1 == true)
{
validationMsg = validationMsg + " OAC run type (runTypeId) is missing.";
}
else
{
validationMsg = "OAC run type (runTypeId) is missing.";
}
var1 = true;
}
if (var1 == true)
{
scriptletContext.putGlobal("validationMsg", validationMsg);
}
scriptletContext.putGlobal("missingVAL", var1);Solution
A quick note on style, it's much more common in JavaScript code to have the opening curly bracket on the same line as the expression that precedes it, rather than on a new line.
Along the same lines,
It's a good practice to use strict equality/inequality (
For instance:
Also, when comparing values to
You are using the same validation routine 4 separate times; instead of copying the code, it's simpler to make one function that does the work then just call it 4 times.
Your validations do this:
-
Check that the input value is not false (empty, undefined or null)
-
If it fails, add a message into a string noting the validation failed
-
Make the validation flag variable true to indicate there is a validation problem.
Step (3) could reasonably be removed by using the
We could write a function like this: (code comments added for explanation)
This function introduces a side effect which is to modify the globally-scoped variable
Then at the very end of the script, we can check whether
There is one potential issue with this logic, in that if a
This is what it looks like with all the above applied.
var1, based on the code, indicates whether there has been a validation problem, and to keep the same logical flow, we could name it instead isInvalid for more clarity.Along the same lines,
missingVAL is never used; perhaps you meant to use that one instead of var1?It's a good practice to use strict equality/inequality (
=== and !==) instead of == and != in many cases, to avoid some unpredictable behavior that is otherwise caused by the JavaScript engine.For instance:
1 == "1" //true
1 === "1" //false
null == undefined //true
null === undefined //false
"" == 0 // true
"" === 0 //falseAlso, when comparing values to
true or false, it's simpler to just let the truthiness or falseness of the value be the condition, such as:// instead of this:
if (someValue == true && someOtherValue == false) { ... }
// simply do this: (note the use of not "!" operator)
if (someValue && !someOtherValue) { ... }You are using the same validation routine 4 separate times; instead of copying the code, it's simpler to make one function that does the work then just call it 4 times.
Your validations do this:
-
Check that the input value is not false (empty, undefined or null)
-
If it fails, add a message into a string noting the validation failed
-
Make the validation flag variable true to indicate there is a validation problem.
Step (3) could reasonably be removed by using the
validationMsg, since if there have been no validation failures by the end, it will still be an empty string.We could write a function like this: (code comments added for explanation)
function validate(value, messageIfInvalid) {
// null, undefined, 0 and "" evaluate to false
if (!value) {
// empty string evaluates to false and non-empty to true
if (validationMsg) {
// if not empty, add a space at the end
validationMsg += " ";
}
// add the messageIfInvalid
validationMsg += messageIfInvalid;
}
}This function introduces a side effect which is to modify the globally-scoped variable
validationMsg, which makes it less predictable. It would be generally better to instead pass it the validationMsg as argument and then return the validationMsg with any applicable modifications made by the function, like so:// also pass validationMsg as argument
function validate(value, validationMsg, messageIfInvalid) {
if (!value) {
if (validationMsg) {
validationMsg += " ";
}
validationMsg += messageIfInvalid;
}
// return the validationMsg, whether or not it was modified
return validationMsg;
}Then at the very end of the script, we can check whether
validationMsg is an empty string or not to find out if there have been any invalid inputs.// this condition will be true unless validationMsg is an empty string
if (validationMsg) {
scriptletContext.putGlobal("validationMsg", validationMsg);
scriptletContext.putGlobal("missingVAL", true);
} else {
scriptletContext.putGlobal("missingVAL", false);
}There is one potential issue with this logic, in that if a
value checked as invalid/false, but the messageIfInvalid argument was passed an empty string, then no message would be added to validationMsg. We can add another condition to address this special case and just add a generic validation message, like so:if (!value) {
if (!messageIfInvalid) {
messageIfInvalid = "a value is missing";
}
if (validationMsg) {
validationMsg += " ";
}
validationMsg += messageIfInvalid;
}This is what it looks like with all the above applied.
host = scriptletContext.get("host");
user = scriptletContext.get("user");
serverId = scriptletContext.get("serverId");
runId = scriptletContext.get("runId");
function validate(value, validationMsg, messageIfInvalid) {
if (!value) {
if (!messageIfInvalid) {
messageIfInvalid = "a value is missing";
}
if (validationMsg) {
validationMsg += " ";
}
validationMsg += messageIfInvalid;
}
return validationMsg;
}
var validationMsg = "";
validationMessage = validate(host, validationMessage, "serverHostName (serverHostName) is missing.");
validationMessage = validate(user, validationMessage, "Requestor user name (requestorUserName) is missing.");
validationMessage = validate(serverId, validationMessage, "(vascoServerId) is missing.");
validationMessage = validate(runId, validationMessage, "OAC run type (runTypeId) is missing.");
if (validationMsg) {
scriptletContext.putGlobal("validationMsg", validationMsg);
scriptletContext.putGlobal("missingVAL", true);
} else {
scriptletContext.putGlobal("missingVAL", false);
}Code Snippets
1 == "1" //true
1 === "1" //false
null == undefined //true
null === undefined //false
"" == 0 // true
"" === 0 //false// instead of this:
if (someValue == true && someOtherValue == false) { ... }
// simply do this: (note the use of not "!" operator)
if (someValue && !someOtherValue) { ... }function validate(value, messageIfInvalid) {
// null, undefined, 0 and "" evaluate to false
if (!value) {
// empty string evaluates to false and non-empty to true
if (validationMsg) {
// if not empty, add a space at the end
validationMsg += " ";
}
// add the messageIfInvalid
validationMsg += messageIfInvalid;
}
}// also pass validationMsg as argument
function validate(value, validationMsg, messageIfInvalid) {
if (!value) {
if (validationMsg) {
validationMsg += " ";
}
validationMsg += messageIfInvalid;
}
// return the validationMsg, whether or not it was modified
return validationMsg;
}// this condition will be true unless validationMsg is an empty string
if (validationMsg) {
scriptletContext.putGlobal("validationMsg", validationMsg);
scriptletContext.putGlobal("missingVAL", true);
} else {
scriptletContext.putGlobal("missingVAL", false);
}Context
StackExchange Code Review Q#157781, answer score: 6
Revisions (0)
No revisions yet.