Array Hackery not so pretty

I'm new to Rust coming from Python & TypeScript.

I'm trying to pretty-print (trusted) input data of unknown format. I spin thru object keys and if I find an array I have this code to print it out:

if value.is_array() {
    // Wow this is some hackery
    for e0 in value.as_array() {
        for e1 in e0 {
            for e2 in e1.as_array() {
                println!();
                for (i, e3) in e2.iter().enumerate() {
                    print!("     {} - ", i + 1);
                    let object = e3.as_object().unwrap();
                    for key in object.keys() {
                        let mut value = object.get(key).unwrap().to_string();
                        if value.contains(order_num) {
                            value = value.yellow().to_string();
                        }
                        print!("{}:{}  ", key.green(), value);
                    }
                    println!();
                }
            }
        }
    }
}

Sample input: '2022-06-02 13:32:19 UTC INFO [default] Sending {payload: {command: "tradeTransactionStatus", arguments: { order: 392202145 }, customTag: "0.6539318491355446"}, account: { id: 13372997, name: "Thomas Duna (Stav#6)", type: "demo" } }'

Sample output:

2 13964. 2022-06-02 13:32:19 UTC (245) Sending
  * account: 
    - id: 13372997
    - name: "Thomas Duna (Stav#6)"
    - type: "demo"
  * payload: 
    - arguments: {"order":392202145}
    - command: "tradeTransactionStatus"
    - customTag: "0.6539318491355446"

The code is here.

It's so ugly. How can I make this code better?

If your variable value is a serde_json::Value, it's already "Debug printable", but you could also deserialize it into a struct (or a Vec of structs) like in this playground ?

Yes, it should be a Value.

Debug printing works but I now need to format the data while printing, for example highlight the values in a brighter color if they are the important.

users.rust-lang.org_output

I would like to deserialize into a struct, that would be best. Since I don't know the structure of the data ahead of time maybe I can make some generic deserializer to handle cases for each data type.

Oh, maybe I just need serde::Deserialize in the derive thing and... yeah, I'll try that.

A few tips.

  1. If you're asking for a code review or showing some code, consider putting in on the playground if possible. This makes it easier for people to test how it works and to provide suggestions, simply by editing the snippet. Unfortunately, this specific snippet depends on the colored crate, which isn't available on the playground. But at least now we have a self-contained snippet which can be easily copied and tested locally. It also allows to check for errors which do not stem from the absence of colored.

  2. The function show_json takes an order_num: &String parameter. Having a parameter explicitly typed as &String is basically always a mistake. It means that whoever calls your function must allocate a String, even though within your function you can only call methods which are available on a &str (since &String is always immutable). There is a deref coercion from &String to &str, which means that the API for the consumer is strictly more general and ergonomic.

  3. Make sure to run Clippy on all your project, and clear all its warnings. It has a truckload of style, performance and correctness lints, and in particular it would catch the issue above.

  4. for e0 in value.as_array() -- yes, Option is IntoIterator and you can iterate over it in a for loop, but it's mostly a newbie trap and almost always bad for readability. There are very few cases where using Option as an iterator is a valid approach. One example is passing it into a function which expects impl IntoIterator if you want an iterator which has at most one element depending on some condition. Another is using something like iterator.chain(option), which allows you to optionally extend an iterator with a single element (since all iterator combinators return different types, composing them may be problematic otherwise). The proper pattern here is to use refutable pattern matching: if let Some(e0) = value.as_array() { ... }.

  5. You have a separate is_array check, followed by as_array().unwrap(). This is a common antipattern. You should combine those into a single if let Some(e) = value.as_array() { ... } block, which guarantees that the block executes only if you indeed have an array. In general, calling unwrap on an Option almost always means either not properly utilizing pattern matching, or ignoring probable errors.

  6. Similarly, you iterate over keys of a map for key in object.keys() and then separately extract the values object.get(key).unwrap(). You should combine it into a single iteration by key-value pairs: for (key, value) in object { ... }. If you really want to get a value for a key that you know is in the map, you should just use the indexing operator object[key]. Note that you cannot get a mutable reference to the value this way.

  7. Your function repeats several times some reasonably complicated value styling code (color the value yellow if it's the order number). I suggest extracting it into a separate (possibly local) function, or a closure.

  8. Note that printing to stdout via print!/println! is line-buffered in Rust. This means that if your json is large and spans many lines, printing it may be much slower than desirable. If your program is multithreaded, it may also lead to garbled output (print! acquires a lock on stdout, but it obviously doesn't persist between different print! calls). Consider first printing the struct to an in-memory buffer, and then writing it to stdout in a single call. This may be too much trouble depending on your use case, e.g. if the function is only used for debugging.

  9. Consider using the iterator combinators to merge nested loops and mapping/filtering logic. It is a bit of an art to know whether it will increase or decrease readability, and different people have different preferences, but imho in your case iterator combinators work very well.

Overall, here is how I would write this function without knowing any larger context or changing its behaviour. I wouldn't say it's too bad, good enough for what it's doing. If you want better reusability, composability and customization, you may consider introducing a more structured representation than serde_json::Value, which would include the key/value styling logic and the pretty-printing in its Display impls.

10 Likes

Let me first say what an honor it is to have such a great review by an obviously talented Rust programmer. I learned a lot from this reply, especially about: Clippy (awesome!), closures, deref coercion, if-let statement and key-value pairs (somehow I couldn't figure it out before).

But it was the iterator combinator within an if-let statement that I was looking for which turned this:

if value.is_array() {
    for e0 in value.as_array() {
        for e1 in e0 {
            for e2 in e1.as_array() {
                println!();
                for (i, e3) in e2.iter().enumerate() {
                    print!("     {} - ", i + 1);
                    let object = e3.as_object().unwrap();
                    for key in object.keys() {
                        let mut value = object.get(key).unwrap().to_string();
                        if value.contains(order_num) {
                            value = value.yellow().to_string();
                        }
                        print!("{}:{}  ", key.green(), value);
                    }
                    println!();
                }
            }
        }
    }
}

into this:

if let Some(arr) = value.as_array() {
    for inner_array in arr.iter().filter_map(Value::as_array) {
        println!();
        for (i, object) in inner_array.iter().filter_map(Value::as_object).enumerate() {
            print!("     {} - ", i + 1);
            for (key, value) in object {
                print!("{}:{}  ", key.green(), style(value));
            }
            println!();
        }
    }
}

The explanations were thoughtful and dense with information and well written in perfect English which I relish.

I made all the suggested changes from the nine (9) points in order one-by-one so excited to not only learn but learn with my own code which locked in so many puzzle pieces that I'm still giddy.

Then if that wasn't enough I got to the end of the reply and found a link to a playground with the final code all wrapped up with a bow on top. I typed the code from the playground into my editor and... cargo run... flawless.

Thank you so much Anton.

2 Likes

Glad I could help :slightly_smiling_face:

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.