Implement Stacks and Queues in Rust. #924
Conversation
| if self.list.len() > 0 { | ||
| Some(&self.list[0]) | ||
| } else { | ||
| None |
There was a problem hiding this comment.
is it safe to return None as front element when Queue is empty?
There was a problem hiding this comment.
What do you mean safe?
You're returning an Option<> so returning a None is one of the expected values.
There was a problem hiding this comment.
Okay safe may not be correct word here.
I thought like, say i was doing something like Q.front() * 5, then returning None would not be right thing to do, right? instead executing panic!("Attempt to access element from a empty Queue") may be helpful.
There was a problem hiding this comment.
You can't do Q.front() * 5 anyway with an Option. panic! is not an exception, you are crashing the application to say something bad happened (and implied unrecoverable, which is not the case here).
Plus panic! only works at runtime, which is kinda late to warn on possible problems.
When you write a public facing function, you have to define what the function takes and returns. You could only return T, and crash/exception/error handle.
But Option says, "ok we can have either a legit value or None".
That None can mean many things, but it describes that no valid data can be returned. And you don't want your user to fall into cases where you get weird/undefined behaviours, so being explicit helps a lot. And the programmer than has to deal with the different cases (or can take shortcuts if he knows that special cases can't happen).
You don't really care what the programmer does, what you do care about is correctly defining and documenting what your function does, deosn't do, and special cases. And there is always several ways to do that, but I think in this case having Option does the job well enough.
You can see in the standard library for example here that Optional is also used in similar cases.
There was a problem hiding this comment.
Thank you for being so kind.
You completely make sense.
Amaras
added
Implementation
There was a problem hiding this comment.
This looks good overall. Could you just add a Cargo.toml and I think we should be good.
I think this should be enough, but feel free to edit it.
[package]
name = "stack_and_queue"
version = "0.1.0"
edition = "2021"
# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
[dependencies]
[[bin]]
path = "./Queue.rs"
name = "queue"
[[bin]]
path = "./Stack.rs"
name = "stack"
|
Like I've said before, if the name of the built binary isn't main then it won't be copied into the build directory by SConscript. Cargo can't build both files because into main, so I think that we should leave it to be compiled with rustc. SCons can't do multiple files from the same chapter yet, which will need fixing for #950 as you pointed out there. I think it might be best to hold off merging this PR (and #950) until we work that out. |
Since both files have a main function, the smartest move in my opinion would be to put a SConscript with two distinct rustc targets (say We'll merge once this is done, since there already is an approval. |
PeanutbutterWarrior
force-pushed
the
implement-stacks-queues-rust
branch
from
4c95e98 to
cadad08
Compare
|
I've had to rebase in order to get the latest SCons updates, as this branch was created before most of it. No conflicts had to be resolved. Git was confused, so I had to force push. I've added the SConscript file and checked that it builds |
|
Is there anything holding this PR back? I think it's mergable, but am not sure why we waited before |
I've done the same as the Java and Typescript examples code-wise, however I've added minimal code comments. One is Rust specific, pointing out where a reference is returned instead of the value due to the borrow checker. I've also added comments after the print statements in the test to show the output of them.