Solution for assignment 1 #52

AkashAsthana · 2019-08-24T13:30:50Z

No description provided.

…ana/Assignment-1/assignment1.js

…-asthana/Assignment-1/assignment1.test.js

kaustubh-karkare

Excellent start!

kaustubh-karkare · 2019-08-24T15:28:54Z

solutions/akash-asthana/Assignment-1/assignment1.js

+var key = new Arguments(true, "number");
+var name = new Arguments(false, "string");
+var local = new Arguments(false, "boolean", "remote");
+var remote = new Arguments(false, "boolean", "local");


In order to be truly reusable (coding instruction 4.2), you can create a class like this (where we can add different options for each test):

parser = Parser() parser.add_option('key', type=int, required=True) parser.add_option('name', type=str) self.assertEqual(parser.parse(['--key=123', '--name=abc']), {'key': 123, 'name': 'abc'}) self.assertEqual(parser.parse(['--key=456']), {'key': 456})

kaustubh-karkare · 2019-08-24T15:31:13Z

solutions/akash-asthana/Assignment-1/assignment1.js

+* argument's name from 
+* the commands array
+*/
+function extractArgumentName(argumentName) {


If the method takes argumentName as input, why does it have to do any process to extract stuff.
Ie - the variable name is not accurate (coding instruction 3), you can just use argument or option if you wish.

kaustubh-karkare · 2019-08-24T15:34:49Z

solutions/akash-asthana/Assignment-1/assignment1.test.js

@@ -0,0 +1,28 @@
+//USED JEST FOR TESTING THE PROJECT
+const { extractArgumentName, extractArgumentValue, typeCheck } = require("./assignment1");
+


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

Yes, I'll do that!

kaustubh-karkare · 2019-09-03T15:23:43Z

solutions/akash-asthana/Assignment-1/assignment1.js

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


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.

kaustubh-karkare · 2019-09-03T15:32:07Z

solutions/akash-asthana/Assignment-1/assignment1.js

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


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.

kaustubh-karkare · 2019-09-03T15:35:38Z

solutions/akash-asthana/Assignment-1/assignment1.js

+
+
+//MAIN FUNCTION
+function parseArgumentsIntoJSON(commands) {


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).

kaustubh-karkare · 2019-09-03T15:37:29Z

solutions/akash-asthana/Assignment-1/assignment1.js

+    for (var i = 0; i < commands.length; i++) {
+
+        var argumentName = extractArgumentName(commands[i]);
+        var argumentValue = extractArgumentValue(commands[i]);


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

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

kaustubh-karkare · 2019-09-03T15:38:31Z

solutions/akash-asthana/Assignment-1/assignment1.js

+//MAIN FUNCTION
+function parseArgumentsIntoJSON(commands) {
+
+    for (var i = 0; i < commands.length; i++) {


Use ii instead of i.

kaustubh-karkare · 2019-09-03T15:41:04Z

solutions/akash-asthana/Assignment-1/assignment1.js

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


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

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.

kaustubh-karkare · 2019-09-03T15:42:10Z

solutions/akash-asthana/Assignment-1/assignment1.js

+    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`;


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.

kaustubh-karkare · 2019-09-03T15:45:19Z

solutions/akash-asthana/Assignment-1/assignment1.js

+*/
+function ifArgumentsCanBeUsedTogether(argumentNameArray) {
+    for (var i = 0; i < argumentNameArray.length; i++) {
+        if (argumentNameArray.includes(parser.options[i].cantBeUsedWith)) {


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

kaustubh-karkare · 2019-09-03T15:48:01Z

solutions/akash-asthana/Assignment-1/assignment1.js

+            finalResult[argumentNameArray[i]] = argumentValueArray[i];
+        }
+
+        console.log(JSON.stringify(finalResult));


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).

kaustubh-karkare · 2019-09-03T15:48:57Z

solutions/akash-asthana/Assignment-1/assignment1.js

+
+
+        argumentNameArray.push(argumentName);
+        argumentValueArray.push(argumentValue);


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

Added property names while adding options in the parser object

Changed the validation criteria of the options of string type.

AkashAsthana added 4 commits August 24, 2019 18:52

Create assignment1.js

965b593

Create assignment1.test.js

9501e87

Rename solutions/akash-asthana/assignment1.js to solutions/akash-asth…

cb29122

…ana/Assignment-1/assignment1.js

Rename solutions/akash-asthana/assignment1.test.js to solutions/akash…

be9d4b6

…-asthana/Assignment-1/assignment1.test.js

kaustubh-karkare requested changes Aug 24, 2019

View reviewed changes

kaustubh-karkare force-pushed the master branch from f9e6d92 to 4bfac84 Compare August 28, 2019 00:04

AkashAsthana added 2 commits September 3, 2019 00:52

Update assignment1.js

9f2d066

Update assignment1.test.js

a841a1b

kaustubh-karkare requested changes Sep 3, 2019

View reviewed changes

AkashAsthana added 3 commits September 4, 2019 02:21

Update assignment1.js

d9e2313

Added property names while adding options in the parser object

Changed i to ii in for loops

68fc69a

Update assignment1.js

84d4f6f

Changed the validation criteria of the options of string type.

		@@ -0,0 +1,28 @@
		//USED JEST FOR TESTING THE PROJECT
		const { extractArgumentName, extractArgumentValue, typeCheck } = require("./assignment1");



		argumentNameArray.push(argumentName);
		argumentValueArray.push(argumentValue);



		//MAIN FUNCTION
		function parseArgumentsIntoJSON(commands) {

Solution for assignment 1 #52

Are you sure you want to change the base?

Solution for assignment 1 #52

Uh oh!

Conversation

AkashAsthana commented Aug 24, 2019

Uh oh!

kaustubh-karkare left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants