Fixed bug - passing parameter values #189

Srihari123456 · 2022-06-07T17:21:55Z

Addressed Issue #183

Case 1:
Before Refactoring:

function f1(a) {
    if (a) {
        console.log("in if")
    } else {
        console.log("in else")
    }
}

var a = isFlagTreated('featureFlag')
f1(a)

After Refactoring (Previously):

function f1(true) {
    console.log("in if")
}

f1(true)

After Refactoring (Now):

function f1(true) {
    console.log("in if")
}

f1(true)

Case 2:
Before Refactoring:

function f1(a) {
    if (a) {
        console.log("in if")
    } else {
        console.log("in else")
    }
}

var b = isFlagTreated('featureFlag')
f1(b)

After Refactoring (Previously):

function f1(a) {
    if (a) {
        console.log("in if")
    } else {
        console.log("in else")
    }
}

f1(true)

After Refactoring (Now):

function f1(true) {
    console.log("in if")
}

f1(true)

Case 3:
Before Refactoring:

function f1(a) {
    if (a) {
        console.log("in if")
    } else {
        console.log("in else")
    }
}

f1(isFlagTreated('featureFlag'))

After Refactoring (Previously):

function f1(a) {
    if (a) {
        console.log("in if")
    } else {
        console.log("in else")
    }
}

f1(true)

After Refactoring (Now):

function f1(true) {
    console.log("in if")
}

f1(true)

CLAassistant · 2022-06-07T17:21:59Z

All committers have signed the CLA.

ketkarameya · 2022-06-08T13:17:37Z

Hey Thanks for the PR. I am looking into it. I am very unfamiliar with this code, so I may take some time. :|

Srihari123456 · 2022-06-11T18:23:00Z

Sure @ketkarameya !

ketkarameya · 2022-06-14T18:04:48Z

Hey @Srihari123456 ,

I was looking at the goal of the PR. I have one concern here - isn't the literal true redundant? Couldn't that be eliminated too?

For Case 1 - should'nt the refactoring be

function f1() {
    console.log("in if")
}

f1()

Note I have removed the literal true from argument. Unrelated, I am a Javascript noob, but I was surprised to find that JS allows one to pass true as a parameter for f1 .
Similarly for other cases.

ketkarameya

I have done my first pass over the PR.

ketkarameya · 2022-06-14T20:11:19Z

javascript/src/refactor.js

                                nodeArgumentIsFlag = nodeArgument.value === engine.flagname;
                                break;
                        }
                        if (nodeArgumentIsFlag) {
-                            const flagType = methodHashMap.get(node.callee.name).flagType;
+                            const flagType = methodHashMap.get(node.callee.name).flagType; // control || treated


Dangling comments?

I'll remove this in the updated commit

ketkarameya · 2022-06-14T20:11:30Z

javascript/src/refactor.js

                            engine.changed = true;

                            if (
-                                (flagType === 'treated' && engine.behaviour) ||
+                                (flagType === 'treated' && engine.behaviour) || // engine.behaviour = true => treated


Dangling comments?

I'll remove this in the updated commit

ketkarameya · 2022-06-14T20:11:42Z

javascript/src/refactor.js

@@ -462,7 +460,7 @@ class RefactorEngine {
                            engine.isPiranhaLiteral(declaration.init) &&
                            typeof declaration.init.value === 'boolean'
                        ) {
-                            assignments[declaration.id.name] = declaration.init.value;
+                            assignments[declaration.id.name] = declaration.init.value; // Assigning a -> true


Dangling comments?

I'll remove this in the updated commit

ketkarameya · 2022-06-14T20:13:05Z

javascript/src/refactor.js

+                            engine.changed = true;
+                            return engine.trueLiteral();
+                        } else if (assignments[node.name] === false) {
+                            engine.changed = true;


should engine.changed be false here? if not, can we pull out the two assignments outside the if statement

engine.changed must be true here. I'll refine the code such that it isnt redundant

ketkarameya · 2022-06-14T20:15:26Z

javascript/src/refactor.js

@@ -498,6 +524,64 @@ class RefactorEngine {
                        return this.remove();
                    }
                } else if (node.type === 'Identifier') {
+                    // this is the part where the function arguments are substituted values if they are a part of the assignments
+                    if (node.name in assignments) {
+                        if (assignments[node.name] === true) {


Simplify if-else-if to if-else or ternary.
return assignments[node.name] === true ? engine.trueLiteral() : engine.falseLiteral() ?

@ketkarameya the value of assignments[node.name] can be true or false or undefined. Thats why I didnt code it in if-else. Anyways I'll incorporate this suggestion and refine the code

ketkarameya · 2022-06-14T20:17:27Z

javascript/src/refactor.js

@@ -478,11 +476,39 @@ class RefactorEngine {
        return assignments;
    }

+    adjustFunctionParams(parameterList) {


Can we have some comments here - what is adjustFunctionParams doing?

Yes added necessary comments

ketkarameya · 2022-06-14T20:21:51Z

javascript/src/refactor.js

+        return parameterList;
+    }
+
+    pruneVarReferencesinFunc(nn, assignments) {


Can we have some comments on what this function is doing?

Added necessary comments

ketkarameya · 2022-06-14T20:27:50Z

javascript/src/refactor.js

+                        if (node.name === parent.params[i].name) {
+                            for (let j = 0; j < parameterList.length; j++) {
+                                if (
+                                    parameterList[j].functionName === parent.id.name &&


Can we move this check outside of this for loop ? From what i understand, you are checking if the parent function declaration name is same as the function name of parameter_list right? Maybe this could be like the first line inside enter ?

Also do we also need to check that parameterList.length == parent.params.length ?

@ketkarameya parameterList contains the details of the parameters to be removed across all the functions in the file. On the other hand, parent.params denotes only the arguments of a specific function declaration. Hence parameterList.length need not be same as parent.params.length

Then how do we know that parent is the correct method declaration? Will this handle overriden methods?

Can we move this check outside of this for loop ? From what i understand, you are checking if the parent function declaration name is same as the function name of parameter_list right? Maybe this could be like the first line inside enter ?

parameterList is a list of objects, each of which has functionName, parameterIndex, value. So even if its brought under enter, we need to iterate over the list of objects available in parameterList

ketkarameya · 2022-06-14T20:29:27Z

javascript/src/refactor.js

@@ -536,7 +620,7 @@ class RefactorEngine {
        estraverse.traverse(this.ast, {
            enter: function (node, parent) {
                if (node.type === 'FunctionDeclaration') {
-                    current = node.id.name;
+                    current = node.id.name; // name of the function


dangling comment?

I'll remove this in the updated commit

ketkarameya · 2022-06-14T20:29:48Z

javascript/src/refactor.js

            var functionsWithSingleReturn = this.getFunctionsWithSingleReturn();
            var redundantFunctions = this.getRedundantFunctions(functionsWithSingleReturn);
            this.pruneFuncReferences(redundantFunctions);

            iterations++;
        }
-
+        console.log(this.changed);


stale code? could be removed!

I'll remove this in the updated commit

ketkarameya · 2022-06-15T02:57:26Z

Hey @Srihari123456 ,

I was looking at the goal of the PR. I have one concern here - isn't the literal true redundant? Couldn't that be eliminated too?

For Case 1 - should'nt the refactoring be
function f1() {
    console.log("in if")
}

f1()
Note I have removed the literal true from argument. Unrelated, I am a Javascript noob, but I was surprised to find that JS allows one to pass true as a parameter for f1 . Similarly for other cases.

@Srihari123456 I just wanted to make sure that you didn't miss this comment.

Srihari123456 · 2022-06-15T03:04:47Z

Hey @Srihari123456 ,
I was looking at the goal of the PR. I have one concern here - isn't the literal true redundant? Couldn't that be eliminated too?
For Case 1 - should'nt the refactoring be
function f1() {
    console.log("in if")
}

f1()
Note I have removed the literal true from argument. Unrelated, I am a Javascript noob, but I was surprised to find that JS allows one to pass true as a parameter for f1 . Similarly for other cases.
@Srihari123456 I just wanted to make sure that you didn't miss this comment.

@ketkarameya Yes I have figured the way to eliminate redundant literals and actually wanted to implement it this way. So I thought of raising a separate ticket for the same. Anyways I'll include this in my updated commit as well

Srihari123456 · 2022-06-15T05:34:58Z

I have done my first pass over the PR.

@ketkarameya I've commited the necessary changes. Please check that when you can

ketkarameya · 2022-06-16T03:05:55Z

Hey @Srihari123456 i am out on vacation until end of the week. I ll take a look at it as soon as I am back.

ketkarameya · 2022-06-21T03:55:29Z

@Srihari123456 can you please look into why these test cases are failing? Also I was wondering if you could add end to end test cases for this new feature you have implemented (The scenarios similar to the ones you have added in the PR description)

Srihari123456 · 2022-06-27T04:18:39Z

Will check this

Fixed bug - passing parameter values

97af467

mkr-plse requested review from anag004 and ketkarameya June 7, 2022 18:24

ketkarameya reviewed Jun 14, 2022

View reviewed changes

Addressed Comments in PR

3157e49

ketkarameya added the legacy-js Issues/PR related to older PiranhaJS implementation label Sep 7, 2022

ketkarameya force-pushed the master branch from d802f3e to 3955145 Compare September 7, 2023 02:55

Fixed bug - passing parameter values #189

Are you sure you want to change the base?

Fixed bug - passing parameter values #189

Conversation

Srihari123456 commented Jun 7, 2022

CLAassistant commented Jun 7, 2022 • edited

ketkarameya commented Jun 8, 2022

Srihari123456 commented Jun 11, 2022

ketkarameya commented Jun 14, 2022

ketkarameya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Srihari123456 Jun 15, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ketkarameya commented Jun 15, 2022

Srihari123456 commented Jun 15, 2022

Srihari123456 commented Jun 15, 2022

ketkarameya commented Jun 16, 2022

ketkarameya commented Jun 21, 2022 • edited

Srihari123456 commented Jun 27, 2022

CLAassistant commented Jun 7, 2022 •

edited

Srihari123456 Jun 15, 2022 •

edited

ketkarameya commented Jun 21, 2022 •

edited