<refactor> Refactor Node to standardised object #28

JoshdRod · 2026-01-21T22:36:13Z

Previously, nodes in the expression tree had no defined structure. Hence, it was hard to tell what a node needed, and what the correct syntax was for that property.
The standard Node class contains properties:
NON-COMPUTED:

type (*)
content (*)
leftNode
rightNode
parent
COMPUTED:
commutative
precedence

All website functions now interact with nodes using instances of this Node object.

(*) - properties type and content (when dealing with operators) have enums that define the values they can take.

Node class contains properties: - type - content - leftNode - rightNode - parent - commutative - precedence

Previously, expressionToComponentList wasn't able to use the new Node class, for a few reasons: - The Node class needs to be defined before it is accessed (see the Temporal Dead Zone https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let#temporal_dead_zone_tdz) - Move Node class to a new file - Add private type and content properties, which are accessed by the Node getter and setters

- Change node type references to the enum, instead of string - Fix spelling errors

- Change references to node type to reference the enum

- Replace references to string literals in the function to use the NodeType and Operator enums

Copilot

Pull request overview

This pull request refactors the expression tree implementation to use a standardized Node class structure. Previously, nodes were created as plain JavaScript objects with inconsistent properties. The new implementation introduces enum-like classes (Operator and NodeType) and a Node class with defined properties and computed getters.

Changes:

Introduces new Node, Operator, and NodeType classes to standardize node representation
Refactors all node creation and type checking throughout the codebase to use the new standardized objects
Removes manual property assignment for precedence and commutativity in favor of computed getters

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.

File	Description
pages/index/tree.js	New file containing `Operator`, `NodeType`, and `Node` class definitions with enum-like patterns and computed properties
pages/index/script.js	Updates all node creation and type checking to use the new `Node` class and enum values instead of plain objects and string literals
index.html	Adds script tag to include the new tree.js file before script.js

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-21T22:39:45Z

pages/index/tree.js

+					default:
+						throw `Attempted to create Operator node with non-operator content. Given: ${content}`;
+						break;
+				}


The content setter is missing a break statement after the OPERATOR case. This causes fall-through to the FUNCTION case, which may lead to unexpected behavior. Add a break statement after line 108 to prevent fall-through.

Suggested change

}

}

break;

Copilot · 2026-01-21T22:39:45Z

pages/index/tree.js

+				break;
+			case Operator.MULTIPLICATION:
+			case Operator.DIVISION:
+				return 1;
+				break;
+			case Operator.EXPONENTIATION:
+				return 2;
+				break;
+			default:
+				throw `Node of operator type has content ${this.content}, which should not be possible.`;
+		}
+	}
+
+	get commutative() {
+		if (this.type != NodeType.OPERATOR) {
+			throw "Attempted to query commutativity of non-operator.";
+		}
+		switch (this.content) {
+			case Operator.ADDITION:
+			case Operator.MULTIPLICATION:
+				return true;
+				break;


The break statements after return statements are unreachable. When a return statement executes, the function exits immediately, making any subsequent break statements unnecessary and dead code.

Copilot · 2026-01-21T22:39:45Z

pages/index/tree.js

+
+	get precedence() {
+		if (this.type != NodeType.OPERATOR && this.type != NodeType.FUNCTION) {
+			throw "Attempted to access precedence of non-operator or function.";


The error message is thrown as a string. For consistency and better error handling, this should be thrown as an Error object: throw new Error("Attempted to access precedence of non-operator or function.");

Suggested change

throw "Attempted to access precedence of non-operator or function.";

throw new Error("Attempted to access precedence of non-operator or function.");

Copilot · 2026-01-21T22:39:45Z

pages/index/tree.js

+				return 2;
+				break;
+			default:
+				throw `Node of operator type has content ${this.content}, which should not be possible.`;


The error message uses template literal syntax with backticks but is thrown as a string. For consistency and better error handling, this should be thrown as an Error object: throw new Error(Node of operator type has content ${this.content}, which should not be possible.);

Suggested change

throw `Node of operator type has content ${this.content}, which should not be possible.`;

throw new Error(`Node of operator type has content ${this.content}, which should not be possible.`);

Copilot · 2026-01-21T22:39:46Z

pages/index/tree.js

+
+	get commutative() {
+		if (this.type != NodeType.OPERATOR) {
+			throw "Attempted to query commutativity of non-operator.";


The error message is thrown as a string. For consistency and better error handling, this should be thrown as an Error object: throw new Error("Attempted to query commutativity of non-operator.");

Copilot · 2026-01-21T22:39:46Z

pages/index/tree.js

+						break;
+					default:
+						throw `Attempted to create Operator node with non-operator content. Given: ${content}`;
+						break;


The break statement after the throw is unreachable. The throw statement will exit the function, making the subsequent break statement unnecessary.

Suggested change

break;

Copilot · 2026-01-21T22:39:46Z

pages/index/tree.js

+	}
+	set content(content) {
+		// TODO: Add some input validation here
+		switch (this.type) {


In the content setter, when this.type is checked in the switch statement (line 78), the type property may not be set yet if content is being set in the constructor before type is fully initialized. This creates a potential timing issue where the content setter relies on this.type, but the order of execution in the constructor may cause this.type to not be properly set when content is being assigned.

Suggested change

switch (this.type) {

const currentType = this.#_type;

if (currentType === undefined) {

throw 'Cannot set content before node type is set.';

}

switch (currentType) {

Copilot · 2026-01-21T22:39:46Z

pages/index/script.js

 		default:
-			output += currentNode.content;
-			break;
+			output += currentNode.content; break;


The spacing and formatting is inconsistent. Line 743 combines the output statement and break on a single line, while the surrounding code uses separate lines. This inconsistency reduces code readability.

Suggested change

output += currentNode.content; break;

output += currentNode.content;

break;

Copilot · 2026-01-21T22:39:47Z

pages/index/tree.js

+				this.#_type = NodeType.CLOSE_BRACKET;
+				break;
+			default: // Invalid type!
+				throw `Invalid type! Got ${type}, which is not in the type list.`;


The error message uses template literal syntax with backticks but is thrown using the throw statement with a string. For consistency and better error handling, this should be thrown as an Error object: throw new Error(Invalid type! Got ${type}, which is not in the type list.);

Suggested change

throw `Invalid type! Got ${type}, which is not in the type list.`;

throw new Error(`Invalid type! Got ${type}, which is not in the type list.`);

Copilot · 2026-01-21T22:39:47Z

pages/index/tree.js

+						this.#_content = Operator.EXPONENTIATION;
+						break;
+					default:
+						throw `Attempted to create Operator node with non-operator content. Given: ${content}`;


The error message uses template literal syntax with backticks but is thrown as a string. For consistency and better error handling, this should be thrown as an Error object: throw new Error(Attempted to create Operator node with non-operator content. Given: ${content});

JoshdRod · 2026-01-21T22:42:58Z

Adding some documentation as to what each property means would be beneficial.

JoshdRod added 6 commits January 13, 2026 21:36

<feat>! Add boilerplate for Node class

ae66a91

Node class contains properties: - type - content - leftNode - rightNode - parent - commutative - precedence

<fix>! Make componentToPostfix work with refactored node object

52e61a9

- Change node type references to the enum, instead of string - Fix spelling errors

<fix> Make postfixToTree work with refactored node object

cee11fe

- Change references to node type to reference the enum

<fix>! Make normaliseTree work for refactored node object

b194b78

- Change references to node type to reference the enum

<fix> Make treeToMathJax work for refactored node object

0ab8c99

- Replace references to string literals in the function to use the NodeType and Operator enums

JoshdRod requested a review from Copilot January 21, 2026 22:36

Copilot started reviewing on behalf of JoshdRod January 21, 2026 22:36 View session

Copilot AI reviewed Jan 21, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

<refactor> Refactor Node to standardised object #28

<refactor> Refactor Node to standardised object #28

Uh oh!

JoshdRod commented Jan 21, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 21, 2026

Uh oh!

Copilot AI Jan 21, 2026

Uh oh!

Copilot AI Jan 21, 2026

Uh oh!

Copilot AI Jan 21, 2026

Uh oh!

Copilot AI Jan 21, 2026

Uh oh!

Copilot AI Jan 21, 2026

Uh oh!

Copilot AI Jan 21, 2026

Uh oh!

Copilot AI Jan 21, 2026

Uh oh!

Copilot AI Jan 21, 2026

Uh oh!

Copilot AI Jan 21, 2026

Uh oh!

JoshdRod commented Jan 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	throw "Attempted to access precedence of non-operator or function.";
	throw new Error("Attempted to access precedence of non-operator or function.");

	throw `Node of operator type has content ${this.content}, which should not be possible.`;
	throw new Error(`Node of operator type has content ${this.content}, which should not be possible.`);

-		switch (this.type) {
+		const currentType = this.#_type;
+		if (currentType === undefined) {
+			throw 'Cannot set content before node type is set.';
+		}
+		switch (currentType) {

	output += currentNode.content; break;
	output += currentNode.content;
	break;

	throw `Invalid type! Got ${type}, which is not in the type list.`;
	throw new Error(`Invalid type! Got ${type}, which is not in the type list.`);

<refactor> Refactor Node to standardised object #28

Are you sure you want to change the base?

<refactor> Refactor Node to standardised object #28

Uh oh!

Conversation

JoshdRod commented Jan 21, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

JoshdRod commented Jan 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants