Skip to content
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
236 changes: 236 additions & 0 deletions solutions/akash-asthana/Assignment-1/assignment1.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,236 @@
//Declaration of variables

var commands = process.argv.slice(2);
var argumentNameArray = [];
var argumentValueArray = [];
var errorStatement = "";
var errorPresent = false;


class Parser {

addOption(name, isReq, type, cantBeUsedWith) {
this.options.push({ name: name, isReq: isReq, type: type, cantBeUsedWith: cantBeUsedWith });
}
}
Parser.prototype.options = [];
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you set the array directly on the prototype, it becomes a static variable, which is incorrect.

p = new Parser();
p.options.length; // 0
p.addOption('test')
p.options.length; // 1
q = new Parser();
q.options.length // 1, which is wrong

Lets not deal with prototypes in this problem, that concept isn't needed here.




var parser = new Parser();

parser.addOption("key", true, "number");
parser.addOption("name", false, "string");
parser.addOption("local", false, 'boolean', "remote");
parser.addOption("remote", false, "boolean", "local");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Boolean arguments are bad in general, since it is not possible for me to figure out what the true/false means by looking at these calls alone. You're forcing me to look at your implementation to figure it out, which hurts readability.

Instead, you can do something like this:

parser.addOption({name: 'key', isRequired: true, type: "number"});

That way, it is clear.




/*
* Function for extracting out
* argument's name from
* the commands array
*/
function extractArgumentName(argument) {
var argumentName = "";
for (var i = 2; i < argument.length; i++) {
if (argument[i] === '=') {
return argumentName;
}
argumentName += argument[i];
}
return argumentName;
}



/*
* Function for extracting out
* argument's value from
* the commands array
*/
function extractArgumentValue(argument) {
if (!argument.includes("=")) {
return true;
}
var argumentValue = "";
var index = argument.indexOf('=');
for (var i = index + 1; i < argument.length; i++) {
argumentValue += argument[i];
}
return argumentValue;
}



/*
* Function to check if arguments
* given by the user are defined
*/
function checkIfArgumentDefined(argumentName) {
for (var i = 0; i < parser.options.length; i++) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid looping over the list, you can store the options in a map<name, option> instead.

if (argumentName === parser.options[i].name) {
return true;
}
}
return false;
}



/*
* Function to validate the
* type of argument's value
*/
function typeCheck(argumentNameToBeChecked, argumentValueToBeChecked) {

var i;

for (i = 0; i < parser.options.length; i++) {
if (argumentNameToBeChecked === parser.options[i].name) {
argumentNameToBeChecked = parser.options[i].name;
break;
}
}

if (parser.options[i].type === "number") {
return !isNaN(argumentValueToBeChecked);
}
if (parser.options[i].type === "string") {
return /^[a-zA-Z]+$/.test(argumentValueToBeChecked);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does a string have restrictions like this?
All characters can be valid inside a string.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I intended a "string" to consist of only letters so that the option "--name" can have a proper name like "akash" instead of something like "akash123". Anyway i'll change it.

}
if (parser.options[i].type === "boolean") {
return true;
}
}



/*
* Function to check if
* the mandatory arguments are
* present in the commands
* given by the user
*/
function areRequiredArgumentsPresent(argumentNameArray) {
for (var i = 0; i < parser.options.length; i++) {
if (parser.options[i].isReq === true) {
if (!argumentNameArray.includes(parser.options[i].name)) {
errorStatement += `Error :The argument "--${parser.options[i].name}" is required\n`;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://www.facebook.com/groups/856311934764144/permalink/861189610943043/

no global variables, no standalone functions, etc.

errorStatement & parser are global variables inside this library file.

I'd recommend raising exceptions here.

return false;
}
}
}
return true;
}



/*
* Function to check if arguments
* given by the user can be used together
*/
function ifArgumentsCanBeUsedTogether(argumentNameArray) {
for (var i = 0; i < argumentNameArray.length; i++) {
if (argumentNameArray.includes(parser.options[i].cantBeUsedWith)) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're assuming that only 2 arguments can be mutually exclusive.
What if we wanted to allow exactly one of 5 options?

errorStatement += `Error : The argument "--${parser.options[i].name}" can't be used with` +
` the argument "--${parser.options[i].cantBeUsedWith}"\n`
return false;
}
}
return true;
}



//MAIN FUNCTION
function parseArgumentsIntoJSON(commands) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a method in your class (along with the previous ones). That way,

const p = new Parser();
p.addOption(...)
result = p. parseArgumentsIntoJSON(commands);

Btw, you're not allowed to create the Parser object in this file, you can only define the class.
You should only create class instances outside the library, in your test (this is how your integration tests will look like).


for (var i = 0; i < commands.length; i++) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


var argumentName = extractArgumentName(commands[i]);
var argumentValue = extractArgumentValue(commands[i]);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you really need separate functions for this? How about:

var [name, value] = commands[ii].split("=")


//Checking if the argument provided is defined
if (!checkIfArgumentDefined(argumentName)) {
errorPresent = true;
errorStatement += `Error : The argument "--${argumentName}" is undefined`;

}

if (!typeCheck(argumentName, argumentValue)) {
errorStatement += `Error : Type of the argument "--${argumentName}" is invalid\n`;
errorPresent = true;
}



argumentNameArray.push(argumentName);
argumentValueArray.push(argumentValue);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use a map here? Instead of these 2 arrays which represent keys and values of that map.

}



/*
* Checking if the mandatory arguments
* are present
*/
if (!areRequiredArgumentsPresent(argumentNameArray)) {
errorPresent = true;
}


/*
* Checking if the arguments
* can be used together
*/
if (!ifArgumentsCanBeUsedTogether(argumentNameArray)) {
errorPresent = true;
}



/*
* Printing out the JSON output
* if no error is found
*/
if (!errorPresent) {
var finalResult = {};
for (var i = 0; i < argumentNameArray.length; i++) {
finalResult[argumentNameArray[i]] = argumentValueArray[i];
}

console.log(JSON.stringify(finalResult));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to make this library actually useful, you should return the JSON object, and then let the application print it (or compare against expected value in test, or whatever else it wants to do).

}

/*
* Printing out the error statement
* if error is found
*/
else {
console.log(errorStatement);
}


}



/*
* Calling the main function and
* passing the command given by the user
*/
parseArgumentsIntoJSON(commands);



//Exporting functions to test
module.exports = {
extractArgumentName,
extractArgumentValue,
typeCheck,
checkIfArgumentDefined,
areRequiredArgumentsPresent,
ifArgumentsCanBeUsedTogether,
parseArgumentsIntoJSON
};
40 changes: 40 additions & 0 deletions solutions/akash-asthana/Assignment-1/assignment1.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
//USED JEST FOR TESTING THE PROJECT
const { extractArgumentName,
extractArgumentValue,
typeCheck,
checkIfArgumentDefined,
areRequiredArgumentsPresent,
ifArgumentsCanBeUsedTogether,
parseArgumentsIntoJSON } = require("./assignment1");

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you have here are some "unit tests", which is a good start.
Can you also write some "integration tests"?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'll do that!

test("Should return the name of the argument passed", () => {
expect(extractArgumentName("--key=212")).toEqual("key");
});

test('Should return value of the argument passed', () => {
expect(extractArgumentValue("--name=akash")).toEqual("akash");
})

test('Should check valid type of the argument passed', () => {
expect(typeCheck("key", "213jkbkj12")).toEqual(false)
})

test("Should check if given argument is defined", () => {
expect(checkIfArgumentDefined("name")).toEqual(true)
});

test("Should check if given argument is defined", () => {
expect(checkIfArgumentDefined("lastName")).toEqual(false);
});

test("Should check if given argument is defined", () => {
expect(areRequiredArgumentsPresent(["key", "name"])).toEqual(true);
});

test("Should check if given argument is defined", () => {
expect(ifArgumentsCanBeUsedTogether(["key", "name", "local", "remote"])).toEqual(false);
});