Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed bug - passing parameter values #189

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Srihari123456
Copy link

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
Copy link

CLAassistant commented Jun 7, 2022

CLA assistant check
All committers have signed the CLA.

@ketkarameya
Copy link
Collaborator

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

@Srihari123456
Copy link
Author

Sure @ketkarameya !

@ketkarameya
Copy link
Collaborator

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.

Copy link
Collaborator

@ketkarameya ketkarameya left a comment

Choose a reason for hiding this comment

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

I have done my first pass over the PR.

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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Dangling comments?

Copy link
Author

Choose a reason for hiding this comment

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

I'll remove this in the updated commit

engine.changed = true;

if (
(flagType === 'treated' && engine.behaviour) ||
(flagType === 'treated' && engine.behaviour) || // engine.behaviour = true => treated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Dangling comments?

Copy link
Author

Choose a reason for hiding this comment

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

I'll remove this in the updated commit

@@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Dangling comments?

Copy link
Author

Choose a reason for hiding this comment

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

I'll remove this in the updated commit

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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

@@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

@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

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

adjustFunctionParams(parameterList) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Yes added necessary comments

return parameterList;
}

pruneVarReferencesinFunc(nn, assignments) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Added necessary comments

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

Choose a reason for hiding this comment

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

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 ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

@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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Author

@Srihari123456 Srihari123456 Jun 15, 2022

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

got it.

@@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

dangling comment?

Copy link
Author

Choose a reason for hiding this comment

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

I'll remove this in the updated commit

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

iterations++;
}

console.log(this.changed);
Copy link
Collaborator

Choose a reason for hiding this comment

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

stale code? could be removed!

Copy link
Author

Choose a reason for hiding this comment

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

I'll remove this in the updated commit

@ketkarameya
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Author

I have done my first pass over the PR.

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

@ketkarameya
Copy link
Collaborator

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
Copy link
Collaborator

ketkarameya commented Jun 21, 2022

@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
Copy link
Author

Will check this

@ketkarameya ketkarameya added the legacy-js Issues/PR related to older PiranhaJS implementation label Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
legacy-js Issues/PR related to older PiranhaJS implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants