such0299Feedback for first Rust Program
parse_oui_database takes in a file path as a &String that is used to open the file in a parsing function. IMO there are a number of problems here.
First, you should almost never take in a &String as a function argument. This basically means you have a reference to a owned object. It forces an allocation of everything passed into the function only to take a reference of it. It excludes types that are simply &strs forcing the caller to convert them to a full String - which involves an allocation. The function should just taking in a &str as it is cheap to convert a String to a &str (cheaper to use than a &String as well as &String have a double redirection).
Sometimes it might be even better might be to take in a impl<AsRef(str)> which means the function can take in anything that can be converted into a &str without the caller needing to do it directly. Though on larger functions like this that might not always be the best idea as it makes it generic and so will be monomorphised for every type you pass into it. This can bloat a binary if you do it on lots of large functions with lots of different input types. You can also get the best of both worlds with a generic wrapper function to a concrete implementation - so the large function has a concrete type &str and a warpper that takes a impl <AsRef<str>> and calls the inner function. Though in this case it is probably easier to just take in a &str and manually convert at all the one call sites.
Second. String/&str are not the write types for paths. Those would be PathBuf and &Path which both work like String and &str (so all the above applies to these as well). These are generally better to use as paths in most OSs dont have to be unicode. Which means there are files (though very rarely) which cannot be represented as a String. This is why File::open takes in a AsRef<Path> which your function can also.
Lastly. I would not conflate opening a file with parsing it. These should be two different functions. This makes the code a bit more flexible - you can get the file to parse from other sources. One big advantage to this would be for testing where you can just have the test data as strings in the test. It also makes the returned error type simpler as one function can deal with io errors and the other with parsing errors.
First, you should almost never take in a &String as a function argument. This basically means you have a reference to a owned object. It forces an allocation of everything passed into the function only to take a reference of it. It excludes types that are simply &strs forcing the caller to convert them to a full String - which involves an allocation. The function should just taking in a &str as it is cheap to convert a String to a &str (cheaper to use than a &String as well as &String have a double redirection).
I think i need to explore more about pointer and reference? its still a new concept for me :D, so i just throw everything with &String without a strong understanding about the things it does in the background. Thank you for pointing that out.
Second. String/&str are not the write types for paths. Those would be PathBuf and &Path which both work like String and &str (so all the above applies to these as well). These are generally better to use as paths in most OSs dont have to be unicode. Which means there are files (though very rarely) which cannot be represented as a String. This is why File::open takes in a AsRef<Path> which your function can also.
What i learned from this is that a misconception of the parameter input. Instead of using String, i have to treat it as PathBuf?, I’ll check it out later
Lastly. I would not conflate opening a file with parsing it. These should be two different functions. This makes the code a bit more flexible - you can get the file to parse from other sources. One big advantage to this would be for testing where you can just have the test data as strings in the test. It also makes the returned error type simpler as one function can deal with io errors and the other with parsing errors. And in this case you can just parse the file directly from the internet request rather than saving it to a file first (though there are other reasons you may or may not want to do this).
I do agree with this statement, I’ll refactor my code to seperate the read and the parse logic
Thank you Nous.
PS: How do we tag other users? XD
Whenever you make a String you’re saying that you need a string that can grow or shrink, and all the extra code required to make that work. Because it can grow or shrink it can’t be stored on the (fast and efficient) stack, instead we need to ask the OS for some space on the heap, which is slow but usually has extra space around it so it can grow if needed. The OS gives back some heap space, which we can then use as a buffer to store the contents of the string.
If you just need to use the contents of a string you can accept a &str instead. A &str is really just a reference to an existing buffer (which can be either on the stack or in the heap), so if the buffer the user passes in is on the stack then we can avoid that whole ‘asking the OS for heap space’ part, and if it’s on the heap then we can use the existing buffer on the heap at no extra cost.
Compare this to taking a &String which is basically saying ‘this string must be on the heap in case it grows or shrinks, but because it’s an immutable reference I promise I won’t grow or shrink it’ which is a bit silly.