1
\$\begingroup\$

This is what I implemented in Rust to get the maximum number of consecutive 1s in list of binary numbers.

impl Solution {
    pub fn print_values(nums: Vec<i32>) -> i32{
        let mut result = 0;
        let mut count = 0;
        // let mut end = 0;
        // NotE: Change the if so that it check
         // if 0 else have body for 1 condition
    
        if nums.len() < 10000 {
            for (index,num) in nums.iter().enumerate() {
                // println!("{}:{}", index, num);
                if *num == 1 && index != nums.len() {
                    count+=1;
                    // println!("count updated to {}", count);
                }else{
                    
                    if count > result {
                        result = count;
                    
                    }
                    count = 0;
                    // println!("count reset to {} from {}", count, result);
                }
                if index == nums.len()-1 && count > 0 {
                    // println!("Last if checked count {} result {}", count, result);
                    if count > result{
                        result = count;
                        count =0;
                    }
                }
            }
            // println!("afeter leaving for loop ending result :{} ", result);
            result
        
        }else{
            // what's the difference if  we put semicolon here at end or what if not 
            panic!("List is too much larger!! Kindly reduce your list.")

        }
    }
}

Though I've accomplished the desired resutl, I guess as long I tested. Kindly review strictly. How can I improve the speed and memory usage here?

\$\endgroup\$
0
\$\begingroup\$

I will mainly focus on style and design, instead of speed and memory usage, as I think that is where you have the most room for improvement. :)

  • Formatting: Always run cargo fmt (aka rustfmt) before posting your code online! It enforces standard formatting to improve readability.

  • Function arguments: This is a function that does not need ownership over its argument; you can count the ones in a purely read-only way. So it should take &[i32] instead of Vec<i32>. The idea is that this will be much faster and more memory-efficient if the user has a Vec<i32> but needs to retain ownership over it.

  • Naming: The name, print_values, is nondescriptive. A good name for this function would be count_consecutive_ones or max_consecutive_ones. Other than this, the names for your variables look good to me.

  • Program logic: The implementation is a bit convoluted. A few specifics:

    • && index != nums.len() is not necessary, here you know index is used to index nums so it is less than the length.

    • The logic in the block if index == nums.len() - 1 does not belong inside the loop, but outside. In particular, this fails if the vector is empty, but it is also just clearer to include "final" code at the end.

    • With this fixed, you shouldn't need index at all.

    • Local variables (result and count): don't initialize variables at the top of the function, but rather initialize as late as possible, when you need a particular variable.

    Here's a better implementation of the main part of the body:

        let mut result = 0;
        let mut count = 0;
        for num in nums.iter() {
            if *num == 1 {
                count += 1;
                if count > result {
                    result = count;
                }
            } else {
                count = 0;
            }
        }
        result
    
  • Length cap: Regarding the cap you put on the function (panic!ing if nums.len() >= 10000), I like the idea, but I think that you want to be more thoughtful about the way this is designed. Presently, if a user has a vector longer than 10000 to count, they will (1) not be able to use your function, and (2) even worse, probably run into an unexpected crash at runtime that they did not realize would occur.

    The rule in Rust is to always make the possibility of errors -- especially panic! runtime errors -- explicit. I would consider the following design:

    • First, make the magic number 10000 configurable: const LIST_TOO_LARGE: usize = 10000.

    • Second, provide a version that does not fail -- e.g. max_consecutive_ones(nums: &[i32]) -> i32. If you like, you can print a warning: if nums.len() >= LIST_TOO_LARGE { eprintln!("Warning: list too large! Function may be a bit slow.") }

    • Third, if you like, provide a version which fails by wrapping around your non-failing version. Basically if nums.len() >= LIST_TOO_LARGE { /* fail */ } else { max_consecutive_ones(nums) } I would return Result<i32, String> (expose the error explicitly) in this version instead of using panic!().

    Another design you could consider: you could define max_consecutive_ones_throttled(nums, throttle: Option<usize>), where if throttle is specified then you only check the first throttle values. That seems to go along with the idea of providing a cap for efficiency, but now you have the advantage that (1) users of your code can choose a throttle specific to their use case; (2) you don't just fail if the list is too long, but you still at least check the first throttle values.

  • Answering your question about semicolons after panic: whether you have a semicolon or not doesn't make any difference. It is up to you what style you prefer.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.