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

Project - Binary Search Trees: Instructions revised for binary search tree implementation #28142

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kyak15
Copy link

@kyak15 kyak15 commented Jun 7, 2024

kyak15 changes to confusing wording/requirements for the BST assignment.

Because

The PR is to fix confusing wording/requirements in the Binary Search Tree assignment. The original issue asked the user to create a Tree class with an array, but to also use that array in the buildTree function. Thus creating confusion on an object or class directed approach for BST implementation. In my changes, I instruct the user to follow a class directed approach. The use of creating a BST with an unsorted or sorted array is not an option either. The 'Tree' class is initialized with a root of null. The user is then instructed to create the following methods inside the 'Tree' class:

  • findValue
  • insert
  • delete
  • height
  • depth
  • inOrder traversal
  • postOrder traversal
  • preOrder traversal
  • levelOrder
  • isBalanced
  • reBalance
  • buildTree

The first 9 methods allow the user to focus on core BST methods and operations, without needing to worry about inheriting an array to sort and check for duplicates. After successfully implementing the core methods, the user can then use an array of the current tree values to check for balance issues. If the BST is unbalanced, reBalance uses a traversal method to get an array of values from the tree, and buildTree is called to re-balance the tree.

This PR

  • re-wrote certain portions of the instructions to direct the user to take a class approached implementation.
  • re-writes included instructions to create methods within the 'tree' class.
  • re-writes included linking the user to the MDN page regarding classes and methods in JavaScript
  • re-ordered some of the existing instructions to allow the user to focus on core BST methods first.

Issue

Related to #27620

Pull Request Requirements

  • I have thoroughly read and understand The Odin Project curriculum contributing guide
  • The title of this PR follows the location of change: brief description of change format, e.g. Intro to HTML and CSS lesson: Fix link text
  • The Because section summarizes the reason for this PR
  • The This PR section has a bullet point list describing the changes in this PR
  • If this PR addresses an open issue, it is linked in the Issue section
  • If any lesson files are included in this PR, they have been previewed with the Markdown preview tool to ensure it is formatted correctly
  • If any lesson files are included in this PR, they follow the Layout Style Guide

kyak15 changes to confusing wording/requirements for the BST assignment.
@github-actions github-actions bot added the Content: JavaScript Involves the JavaScript course label Jun 7, 2024
Copy link
Contributor

@MaoShizhong MaoShizhong left a comment

Choose a reason for hiding this comment

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

Thanks @kyak15. This is a big PR that really should have started out as an issue, so you could discuss ideas with the team and we can collectively decide what work needs to be done, if any. This avoids you doing a bunch of work that we wouldn't accept for reasons that could have been discussed in the issue.

I will defer most of this to other maintainers who are more involved with the DSA side of the curriculum, but there are a couple of things I'd like to mention and some I'll push back on.

  • We purposely use class/factory because either are fine. We do not need to enforce the use of a class here.
  • If the instance accepts an array when instantiated, why set the root to null? That would make the array redundant. The point of passing in an array is that the buildTree method takes that array, builds the tree from it, then returns the root node which is set as the tree's root, which is our intention. In class terms, this would happen in the constructor.
    class Tree {
      constructor(array) {
        this.root = this.buildTree(array);
      }
      // ...
    If this step's wording has been a noticeable pain point in the community, then a more appropriate resolution would be to discuss improving its wording to be clearer on what's expected, of course without giving too much away.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content: JavaScript Involves the JavaScript course
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants