davimiku / json_parser

Implementation of a parser for JSON data
2 stars 0 forks source link

Written Errors and Suggestion in the Tutorial #3

Open Typas opened 1 week ago

Typas commented 1 week ago

Hi David,

I've already read the whole tutorial, and that is awesome! However, I've also found two written errors in your part 2.

First one is in section Unicode Escapes. The part

        total += (16u32.pow(3 - i) * digit;

should be

        sum += (16u32).pow(3 - i) * digit;

The second error is in section Arrays, in the function parse_array. The part

        let token = &tokens[*index];
        *index += 1;

should be

        *index += 1;
        let token = &tokens[*index];

. Otherwise I would fail the tests on one_element, two_elements and empty_array. Moreover, after this modification, it would also pass the test for the nested array.

There are also some suggestions.

First of all, Rust encourges the type of the parameter to be as general as possible, we often use the most general type. In the tutorial, we should use &[T] to replace &Vec<T>. Vec<T> implements AsRef[T], so we can use all immutable methods in [T] for &Vec<T>. It's common that we don't need to use Vec-specific methods, since most of the methods are mutable. Therefore, using &[T] to replace &Vec<T> is a common practice.

Another one is that the function parse would be better if it accepts &str as parameter type. because we don't really need to consume the string at all.

The last suggestion is that you can introduce pub use and as. Such as

pub use crate::Value as JsonValue;

to make the enums less possible to conflict with other structs.

davimiku commented 1 week ago

Thank you! Fixing the sum variable name in the tutorial in davimiku/blog.davimiku.com@5feacc59b75df07f5208eeccac95fb52ac23c48b, and also fixes the source code for consistency in dfef051734bb3ab4ab28570f5c8a52e4146abfb4.

For the error with the index, yeah, part of what I was trying to show was almost a "narrative" style where we make a bug, and then try to fix it. One thing that a lot of tutorials do that I don't necessarily like is they always give perfect code the first time, whereas in real projects it's important to struggle and learn how to recover from making mistakes. I think there are multiple ways that could be achieved for this function, but the way I chose was to show some trade-offs and also as a way to introduce the matches! macro, even if there might be a better way.

Using a slice (&[T]) instead of a &Vec<T> is definitely better, and there's also a Clippy (lint) warning "ptr_arg" that supports this. I do actually have a small note (it's hidden in a spoiler) about this in Part 1, I was trying not to introduce too many new concepts at the same time, so I left it as an "optional" concept if someone wants to use it.

Similarly, definitely agree we should use a &str when possible instead of String because there's no need to force the user of the library to move their string into our library code. When this is first introduced in Part 1, this concept isn't explained yet so I was just trying to keep it as simple as possible (for many people, myself included, having multiple kinds of strings is confusing at first and even somewhat of a turn-off to be interested to continue). It's a great idea still, so added this to the Appendix in davimiku/blog.davimiku.com@1fbd3d854930e603d35d73389af1c34b636dae50

Great suggestion for pub use! I've used that before with something like serde_json::Value to rename imports. I think for the purpose of this tutorial I'm going to leave it out, but that does make sense to me.

Great feedback overall, thank you! 😄