[GeoMechanicsApplication] Fixed several code smells #12182
Conversation
- Rephrased a comment to avoid it being detected as commented out code. - Don't declare multiple variables (of the same type) using a single statement.
- Avoid loss of precision due to explicit (mismatching) type names. - Renamed a few local variables to adhere to the Kratos Style Guide.
| rValues.resize(IntegrationPointsNumber); | ||
| } | ||
| for (unsigned int i = 0; i < IntegrationPointsNumber; ++i) { | ||
| rValues.resize(number_of_integration_points); |
There was a problem hiding this comment.
Just out of curiosity, do you know if the resize checks if it's necessary to do it? (In any case I don't think it really improves performance if we manually check like we did before, but I don't know enough about boost::ublas)
There was a problem hiding this comment.
First of all, note that rValues has type std::vector<int>&, which means that we need to refer to the STL documentation. There it is says:
Resizes the container to contain
countelements, does nothing ifcount == size().
In other words, the caller doesn't need to explicitly check the container's size. Calling resize on a std::vector that already has the desired size, has no effect.
| } | ||
| for (unsigned int i = 0; i < IntegrationPointsNumber; ++i) { | ||
| rValues.resize(number_of_integration_points); | ||
| for (auto i = SizeType{0}; i < number_of_integration_points; ++i) { |
There was a problem hiding this comment.
Could be a std::transform 😄 , but leave it up to you if you want to do it, or just merge these improvements (since we're going to tackle the rest later anyway)
There was a problem hiding this comment.
Well, in this case that is unfortunately not so clean, since the value we're trying to retrieve is also returned through the parameter list. Bleeeh! (In addition to returning the value through the function's return value.) Since inside the lambda expression (which you need to pass to std::transform) you don't have access to the object where the transformed value will be stored, it means that you would need to create a dummy variable, just in order to call GetValue. In my opinion, that is not a clean solution (since you probably need a comment to explain what is happening there). So I'd rather leave it the way it is now. I would be in favor of changing it to std::transform if the function signature of GetValue would be improved (i.e. don't return the value through the parameter list, but only through its return value).
There was a problem hiding this comment.
Yep, I fell again for that one. I'll just try to keep in mind to check twice before making any suggestion which involves the GetValue function.
📝 Description
Last night SonarQube detected a few "new" code smells, after merging the branch which reordered the degrees of freedom of the non-diff U-Pw elements. Those are addressed by this PR. In addition, a few more code smells (low hanging fruit) were fixed.