refactor/DYN-6124/code-smells#189
Conversation
|
Kudos, SonarCloud Quality Gate passed!
|
RobertGlobant20
left a comment
RobertGlobant20
left a comment
There was a problem hiding this comment.
LGTM with two comments.
Also did you test this changes in DynamoSandbox? Please include a GIF of your testing.
| } | ||
|
|
||
| //call update category groups. | ||
| updateCategoryGroups(sectionElements); |
There was a problem hiding this comment.
is this call not necesary? or is done in other place?
There was a problem hiding this comment.
@RobertGlobant20 Please, let me know if I'm missing something but If you look at the function declaration, it doesn't return anything that might affect the context where it is executed. Also, all the operations there are related to sectionElements which is defined as a let.
There was a problem hiding this comment.
yeah looks like the function was disabled so no longer valid
|
|
||
| if (pathToItem.length == 1) { | ||
| return item ? true : false; | ||
| return !!item; |
There was a problem hiding this comment.
could you explain whyitem ? true : false;is the same than !!item;?
I know that "!" is the logical not operator but not sure why is used two times?
There was a problem hiding this comment.
@RobertGlobant20 !! operator returns the boolean value. Double NOT (!!)
| } | ||
|
|
||
| // Get keywords from typeListNode and push them into itemData | ||
| let keywords = typeListNode.keywords.split(","); |
There was a problem hiding this comment.
why do we remove this line?
There was a problem hiding this comment.
@QilongTang This variable is not used. The operations below this statement refers to keywords property from the ItemData class (this.keywords)
| default: this.others.push(element); break; | ||
| } | ||
|
|
||
| var finalData = { |
There was a problem hiding this comment.
I guess this is also not used?
There was a problem hiding this comment.
Right, it is not used.
QilongTang
left a comment
QilongTang
left a comment
There was a problem hiding this comment.
LGTM with one comment
|
Please make the change to update librarie.js on Dynamo |
3 participants
This PR includes refactoring due to code smells reports.
FYI
@QilongTang
@RobertGlobant20