-
Notifications
You must be signed in to change notification settings - Fork 16k
Computer Science: Add Graph Project Curriculum Files #30700
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
base: main
Are you sure you want to change the base?
Computer Science: Add Graph Project Curriculum Files #30700
Conversation
- Includes Khan academy article from the Knights Travails project - Adds a description of simple vs nonsimple graphs as this is somewhat necessary for the semantics of the addVertex, addEdge, and query functions - Removes various explanations about graphs because the Khan academy article allows us to offload in-house explanations
- Replaces old G4G link with new w3schools link because it is much better and has a nice interactive graph visualization module - Adds optional article comparing adjacency lists and matrices - Adds requirement for learners to implement toString() method because adjacency list is easy to visualize; don't need to provide a complex function to print something like a BST - Updates some of the language in the interface descriptions to be more explicit about the semantics of each method - Updates 'Test Your Graph' section to reflect the use of adj-list instead of matrix
|
@ maintainers, please review the graph image and the JS version of the project and let me know if it is sufficient, then I will commit the ruby version as well and mark this draft as "Ready for review". Some notes:
|
The specs for each of the methods on the class are updated to be much more explicit about the semantics of each function and what these functions should return if they should return anything meaningful. My idea for the mutator methods (add/remove methods) are for them to be idempotent and effectively silent. Other methods are more or less self-explanatory. This commit also removes a little hint about what data structures to use to represent the adjacency list because the w3schools article is actually quite sufficient for this, even though it doesn't give a concrete implementation.
|
Okay so after thinking about this PR for a bit more, I decided to just finish all the work for now and mark for review so I can get better, more complete feedback on the entire project as it is.
LMK how it is, open to any and all suggestions for improvements — at your convenience. |
mao-sz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave the content reviewing to a more DSA-oriented maintainer. Just a few quick formatting points from me:
- As per our style guide, headings should be in sentence case, not title case
- Some of your indentations are more than necessary e.g. children of ordered list items need indenting 3 spaces from the parent but on line 81 of both (and maybe other places), it's indented 4 spaces.
- List items don't actually need blank lines between them by default (only in cases where you have like a code block as part of it and the code block does need a blank line either side). The extra blank line in normal cases just makes an extra
ptag render in the HTML which isn't necessary. Most your lists don't have blank lines between items but your assignment list does, so we could do with some consistency there.
|
@mao-sz thanks very much for your input! admittedly I didn't review the layout guide before making all the changes. I've have rectified this and hopefully its good now. |
JoshDevHub
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty good. I have one small nitpick right now. I'll have to think on some things about where we are with testing before approving, but I'm hopeful I can accept and merge this soon.
| 1. `getNeighbors(value)`: Returns an array of all the vertices that are adjacent to the vertex `value`, or `undefined` if `value` is not in the graph. | ||
| 1. `getCommonNeighbors(value1, value2)`: Returns an array of all the vertices that are adjacent to both of the vertices `value1` and `value2`, or `undefined` if either `value1` or `value2` are not in the graph. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 1. `getNeighbors(value)`: Returns an array of all the vertices that are adjacent to the vertex `value`, or `undefined` if `value` is not in the graph. | |
| 1. `getCommonNeighbors(value1, value2)`: Returns an array of all the vertices that are adjacent to both of the vertices `value1` and `value2`, or `undefined` if either `value1` or `value2` are not in the graph. | |
| 1. `getNeighbors(value)`: Returns an array of all the vertices that are adjacent to the vertex `value`. If the given `value` isn't in the graph, return `undefined`.. | |
| 1. `getCommonNeighbors(value1, value2)`: Returns an array of all the vertices that are adjacent to both of the vertices `value1` and `value2`. If either of the given values aren't in the graph, return `undefined`. |
Just splitting the undefined return variant into new sentences because I find that reads a bit better.
Also I'm lazy and not going to make a suggestion specific to the Ruby lesson, but I'd like the same structure to for the Ruby lesson as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think it reads better like that as well (and sort of standardizes the phrasing for all the DSA projects). Several of the method descriptions here beyond these getNeighbor functions are phrased like:
"Returns X if condition or Y otherwise"
I'll make similar changes to your suggestions where I think it's appropriate.
Because
As decided in #28487, we will be adding a new graph project. @damon314159 started the work on this project in #29037, but, unfortunately, is not currently able to complete it at this time and handed off the rest of the project to me with the approval of @JoshDevHub.
This PR
(I will request a review for files this PR adds currently, which is just the image and the JS version of the project, then add the ruby version once the JS version is approved).
Issue
Related to #28487
Related to #29037
Related to #29783
Additional Information
For the complete addition of this project into the curriculum and website, we still need to:
Once this PR is merged, I will submit another PR to add the CDN links. However, all 3 issues referenced in the Issue section require these 3 points listed here to be completed in order to be closed. I am entirely willing to take on these tasks as well unless maintainers want to wait on fully adding this project to the site for whatever reason.
(Aside: In the original "Add new graph project" PR that was started by damon314159, josh asked if I would like to "Take ownership of the PR", and I didn't know what that meant because I don't have push priveleges for damon's fork, so I just opened this PR instead which also allowed me to rebase damon's work onto main)
Pull Request Requirements
location of change: brief description of changeformat, e.g.Intro to HTML and CSS lesson: Fix link textBecausesection summarizes the reason for this PRThis PRsection has a bullet point list describing the changes in this PRIssuesection