Conversation
RichDom2185
left a comment
RichDom2185
left a comment
There was a problem hiding this comment.
Please fix formatting issues and clean up unused code/log statements
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| next=next[1]; | ||
| } | ||
| if (count==3){ | ||
| ans=true | ||
| } | ||
| return ans&&this.isBinaryTree(structures[0][1]); | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| } | ||
| DataVisualizer.isBinTree=this.isBinaryTree(structures); | ||
| DataVisualizer.isGenTree=this.isGeneralTree(structures); | ||
| this.get_depth(structures[0],0,0); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| let ans = false; | ||
| let count = 0; | ||
| while (next instanceof Array) { | ||
| count++; | ||
| next = next[1]; | ||
| } | ||
| if (count == 3) { | ||
| ans = true; | ||
| } | ||
| return ans && this.isBinaryTree(structures[0][1]); | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| DataVisualizer.toggleTreeMode(); | ||
| } | ||
| DataVisualizer.toggleNormalMode(); | ||
| DataVisualizer.redraw(); | ||
| }} | ||
| > | ||
| <div style={{ display: 'flex', alignItems: 'center', gap: 8 }}> | ||
| <Icon icon="grid-view" /> | ||
| <Checkbox checked={DataVisualizer.getNormalMode()} style={{ marginTop: 7 }} /> | ||
| </div> | ||
| </AnchorButton> |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| } | ||
|
|
||
| if (node.children![1] instanceof ArrayTreeNode) { | ||
| if (node.children![1].children![0] instanceof ArrayTreeNode) { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
RichDom2185
left a comment
RichDom2185
left a comment
There was a problem hiding this comment.
I personally think this is in violation of OOP principles. Data Visualizer should ideally be a facade that handles the state like currently selected mode, etc. and forwards the calls accordingly.
The actual implementations will belong in two separate classes instead, one for the binary tree stuff and one for the existing implementation. That way we don't have to wrap so much code inside the if-else, and no need for commenting //OriginalView or //TreeNode as everything becomes obvious
Common logic shared across both implementation can live in an abstract class that is subclassed by both.
Thoughts? CC @sayomaki
| } | ||
| DataVisualizer.isBinTree = this.isBinaryTree(structures); | ||
| DataVisualizer.isGenTree = this.isGeneralTree(structures); | ||
| this.get_depth(structures[0], 0, 0); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| if (DataVisualizer.getBinTreeMode()) { | ||
| DataVisualizer.toggleNormalMode(); | ||
| } | ||
| DataVisualizer.toggleBinTreeMode(); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| public static isBinaryTree(structures: Data[]): boolean { | ||
| if (structures[0] === null) { | ||
| return true; | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
I agree. The team is working on an OOP refactor of this PR. |
| let ans = false; | ||
| let count = 0; | ||
| while (next instanceof Array) { | ||
| count++; | ||
| next = next[1]; | ||
| } | ||
| if (count == 3) { | ||
| ans = true; | ||
| } | ||
| return ans && this.isBinaryTree(structures[0][1]); | ||
| } |
There was a problem hiding this comment.
Bug: The isBinaryTree function throws a TypeError when given a non-binary-tree structure because it recursively calls itself on undefined without proper validation.
Severity: HIGH
Suggested Fix
Before making a recursive call to isBinaryTree on a child node, add a check to ensure the child is an array. For example, use Array.isArray(structure[1]) and Array.isArray(structure[2]) before proceeding with the recursion.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/features/dataVisualizer/dataVisualizer.tsx#L61-L76
Potential issue: The `isBinaryTree` function recursively calls itself on child nodes
without first verifying that the child structure is a valid array. When provided with a
data structure that is not a binary tree (e.g., a node has a non-array child), the
function will attempt to call `isBinaryTree(undefined)`. This results in a `TypeError:
Cannot read properties of undefined`, causing a crash.
| structures.push(this.nodeCount[depth]); | ||
| if (this.nodeCount[depth] > this.longestNodePos) { | ||
| this.longestNodePos = this.nodeCount[depth]; | ||
| } | ||
| this.nodeCount[depth]++; | ||
| } | ||
|
|
||
| this.TreeDepth = Math.max(this.TreeDepth, depth); | ||
| this.get_depth(structures[0], depth + 1, 0); | ||
| this.get_depth(structures[1], depth, nodePos + 1); | ||
| return depth; |
There was a problem hiding this comment.
Bug: The get_depth function mutates the input dataRecords array. On redraw(), this leads to compounded data corruption and incorrect tree rendering in GeneralTree mode.
Severity: MEDIUM
Suggested Fix
Refactor get_depth to be a pure function. Instead of mutating the input array, it should create a copy of the array before performing any operations, for instance by using the spread syntax [...tree].
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/features/dataVisualizer/dataVisualizer.tsx#L39-L58
Potential issue: In GeneralTree mode, the `get_depth` function mutates the original
user-provided arrays in `dataRecords` by pushing elements onto them. When `redraw()` is
called, this mutation is not correctly reversed, as `tree.pop()` only removes the last
added element. Subsequent calls to `redraw()` will operate on corrupted data, leading to
incorrect tree visualizations as the depth calculation compounds with each redraw.
| static redraw() { | ||
| this.isRedraw = true; | ||
| this.clear(); | ||
| DataVisualizer.counter = -DataVisualizer.counter; | ||
| return DataVisualizer.dataRecords.map(structures => this.drawData(structures)); |
There was a problem hiding this comment.
Bug: The redraw() method repeatedly appends nodePos values to the data structure in GeneralTree mode, corrupting the data with each redraw.
Severity: MEDIUM
Suggested Fix
Ensure that the redraw() function operates on a clean or deep-copied version of the data for each render, rather than mutating the original data structure passed to it. This will prevent state corruption between redraws.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/features/dataVisualizer/dataVisualizer.tsx#L255-L259
Potential issue: The `redraw()` method in GeneralTree mode corrupts the original data
structures by repeatedly appending `nodePos` values to nodes during each redraw
operation. This mutation persists across redraws, causing the data structure to grow
incorrectly with each call, leading to rendering errors and incorrect visualizations.
5 participants
Summary
This PR adds 2 new box-and-pointer visualizers for when the draw_data function is ran, so that now there are:
Type of Change
Added Functionality
Files Edited
src/features/dataVisualizer/[Documentation.md](http://documentation.md/)src/features/dataVisualizer/images/BINARY_TREE_IMAGE.pngsrc/features/dataVisualizer/images/GENERAL_TREE_IMAGE.pngsrc/features/dataVisualizer/images/ORIGINAL_VIEW_IMAGE.pngsrc/commons/sideContent/content/SideContentDataVisualizer.tsxsrc/features/dataVisualizer/dataVisualizer.tsxsrc/features/dataVisualizer/dataVisualizerTypes.tssrc/features/dataVisualizer/drawable/ArrayDrawable.tsxsrc/features/dataVisualizer/drawable/ArrowDrawable.tsxsrc/features/dataVisualizer/tree/ArrayTreeNode.tsxsrc/features/dataVisualizer/tree/BaseTreeNode.tssrc/features/dataVisualizer/tree/DrawableTreeNode.tsxsrc/features/dataVisualizer/tree/FunctionTreeNode.tsxsrc/features/dataVisualizer/tree/Tree.tsxsrc/commons/sagas/WorkspaceSaga/index.tssrc/features/dataVisualizer/Config.tsHow to Test
Run the following code in the Source Academy playground and open the Data Visualizer:
draw_data(list(1,2,3)): should only give error for “Render Binary Tree” mode.draw_data([1,2,3]): should only give errors for “Render Binary Tree” mode and “Render General Tree” mode.draw_data(list(1,list(2,null,null),list(3,null, null))): should not have any error, generates tree structures correctly in any of the 3 modes.The appropriate box-and-pointer diagram should be generated. Boxes belonging to the same node should be the same colour, while those from the neighbouring nodes should be in different colours. Examples are shown in the documentation file.
Checklist