loop over dynamic array length will show warning #1314
Conversation
| node.children[1].children[1].attributes.member_name === 'length' && | ||
| node.children[1].children[1].children[0].attributes.type.indexOf('[]') !== -1) { | ||
| if (common.isForLoop(node) && (common.isDynamicArrayLengthAccess(node.children[1].children[1]) || | ||
| (node.children[1].children[1].children && common.isDynamicArrayLengthAccess(node.children[1].children[1].children[0])))) { |
There was a problem hiding this comment.
It's really difficult to understand what this if statement is doing. Could you break it into several steps with explcit naming :
const XXX = node.children[1].children[1].children
const XXXchild = node.children[1].children[1].children[0]
const isXXX = !!XXX && common.isDynamicArrayLengthAccess(XXXchild)
const isYYY = isXXX || common.isDynamicArrayLengthAccess(node.children[1].children[1])
const isZZZ = common.isForLoop(node) && isYYY
if (common.isForLoop(node) && isZZZ) ...There was a problem hiding this comment.
Also for better readibility you may want to destructure the "common" object :
const { isForLoop, isDynamicArrayLengthAccess } = require('./staticAnalysisCommon')There was a problem hiding this comment.
I tried above suggestion of dividing checks but that was requiring hierarchical checks for existence otherwise tests were not working like
const XXX = node && node.children && node.children[1] && node.children[1].children[1].children
I used a way which solves our purpose of code explanation using comments
| * @return {bool} | ||
| */ | ||
| function isDynamicArrayLengthAccess (node) { | ||
| return node && nodeType(node, exactMatch(nodeTypes.MEMBERACCESS)) && (node.attributes.member_name === 'length') && node.children[0].attributes.type.indexOf('[]') !== -1 |
There was a problem hiding this comment.
Same here, create intermediary variables. Else it's going to be super difficult for the next person to take over your code.
There was a problem hiding this comment.
sounds reasonable.
fixes #994
Now
array.length - 1will show warning