Refactor process_handle_drop_internal() in bevy_asset
#10920
Conversation
matiqo15
added
A-Assets
There was a problem hiding this comment.
Apart from a duplicated comment after the function, it looks good at a first glance. I don't have the time right now to examine it to the detail, since it involves a very complex control flow.
Next time, try to break down the PR in multiple commits like I did in #10911. It will take more time, but it will make the review much easier and reliable.
crates/bevy_asset/src/server/info.rs
Outdated
| if !watching_for_changes { | ||
| path_to_id.remove(&path); | ||
| return true; | ||
| } | ||
|
|
||
| for loader_dependency in info.loader_dependencies.keys() { | ||
| if let Some(dependants) = loader_dependants.get_mut(loader_dependency) { | ||
| dependants.remove(&path); | ||
| } | ||
| } | ||
|
|
||
| let Some(label) = path.label() else { | ||
| path_to_id.remove(&path); | ||
| return true; | ||
| }; | ||
|
|
||
| let mut without_label = path.to_owned(); | ||
| without_label.remove_label(); | ||
|
|
||
| if let Entry::Occupied(mut entry) = living_labeled_assets.entry(without_label) { | ||
| entry.get_mut().remove(label); | ||
| if entry.get().is_empty() { | ||
| entry.remove(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
This passage is too difficult to understand and duplicates the path_to_id.remove(&path) line, making it more obscure. It is more appropriate to extract it instead of inverting it. It is essentially a sort of subroutine anyway.
Basically you should revert the old if watching_for_changes block, and extract all the code inside it to a new function. Then you can invert the body of the new function as you usually do. If you use rust-analyzer there is a specific action to extract selected code into a function.
Once that's done, I think it's probably OK to go.
There was a problem hiding this comment.
I will say that all the individual instances of the if statement exit conditions (for example),
path_to_id.remove(&path);
return true;
is expressed multiple times which could make making certain changes dangerous as one could easily miss one of these, leading to bugs. Then again, this sort of thing is what testing is for.
It also feels like a "DNRY" (Do'Nt Repeat Yourself) thing although the only solution I can see is going back to what we had before which, as is the reason for the PR, has major readability issues. This bit specifically is the one thing that really bugs me personally but this is really the only issue I have with the code as it is.
Overall though, the code is, I do believe, identical in effect and it is extremely successful at reducing maximum indentation in an idiomatic way.
Nilirad
added
the
S-Ready-For-Final-Review
Objective
process_handle_drop_internal().Solution