Closed barath closed 5 years ago
@barath After skipping through this PR, I have three questions although one of them is trivial.
std::unordered_map<PlantProperty, int64_t>
because different parameters have different types.class Plant
has some public methods and private (or protected) member variables. However, this is a bit different from what I have done on other components (all components are struct
s only). Should other components be refactored to be like this as well?const std::string& foo
or const std::string &foo
?@barath After skipping through this PR, I have three questions although one of them is trivial.
1. Would it be great to start adopting the Unit library from this refactoring? If so, there are some benefits, such as readability and the easiness to convert between units. On the other hand, some problems may arise. One is that the library uses double internally, which causes floating-point error you mentioned in the comments. The other is that we need another approach to implement `std::unordered_map<PlantProperty, int64_t>` because different parameters have different types.
It would be nice to consider that, but yes this is the problem I ran into that motivated the use of int64_t. std::variant
is an option, but it could end up making things more confusing rather than less confusing, because if we use it then we would still only have type information but nothing about how those types are meant to be used for different fields. I figure int64_t will cover 90% of our use cases and probably 100% for now. But if you can think of a property that can't easily be stored in int64_t then let me know. Also, these are just for initial parameters. We can always use double or other types when doing simulation / calculations.
2. It seems that the `class Plant` has some public methods and private (or protected) member variables. However, this is a bit different from what I have done on other components (all components are `struct`s only). Should other components be refactored to be like this as well?
Yes. We should only use structs for data structures that have no methods. For all classes we should consider data hiding as appropriate using public/protected/private.
3. This is about style. Which one should be adopted, `const std::string& foo` or `const std::string &foo`?
I have always used the latter, and set clang format to use it (the .clang-format
file), because it is less error prone. For example:
const std::string& foo, bar;
creates foo and bar of different types but it's not obvious. const std::string &foo, bar;
makes it more clear.
2. It seems that the `class Plant` has some public methods and private (or protected) member variables. However, this is a bit different from what I have done on other components (all components are `struct`s only). Should other components be refactored to be like this as well?
Yes. We should only use structs for data structures that have no methods. For all classes we should consider data hiding as appropriate using public/protected/private.
@barath
My question is that should I refactor other components, using class
es on them instead of struct
s?
2. It seems that the `class Plant` has some public methods and private (or protected) member variables. However, this is a bit different from what I have done on other components (all components are `struct`s only). Should other components be refactored to be like this as well?
Yes. We should only use structs for data structures that have no methods. For all classes we should consider data hiding as appropriate using public/protected/private.
@barath My question is that should I refactor other components, using
class
es on them instead ofstruct
s?
Yes, go for it.
After skipping through this PR, I have three questions although one of them is trivial.
1. Would it be great to start adopting the Unit library from this refactoring? If so, there are some benefits, such as readability and the easiness to convert between units. On the other hand, some problems may arise. One is that the library uses double internally, which causes floating-point error you mentioned in the comments. The other is that we need another approach to implement `std::unordered_map<PlantProperty, int64_t>` because different parameters have different types.
It would be nice to consider that, but yes this is the problem I ran into that motivated the use of int64_t.
std::variant
is an option, but it could end up making things more confusing rather than less confusing, because if we use it then we would still only have type information but nothing about how those types are meant to be used for different fields. I figure int64_t will cover 90% of our use cases and probably 100% for now. But if you can think of a property that can't easily be stored in int64_t then let me know. Also, these are just for initial parameters. We can always use double or other types when doing simulation / calculations.
@barath Should we still use this unit library in other codes? And please check my comments. Thank you.
@barath I mostly did these things in the recent commits:
Action
s. (This is the work I was doing before the previous meeting.)Plant
Resource
in utility.h
to environment::Resource
Now this can be successfully compiled and pass all unit tests. Please check them. Thank you.
Thanks. Just left you a couple small comments. Feel free to make those small edits, and we are probably close to merging this.
@barath
struct Light
has been removed as well (it has the same problem like struct SoilRequirement
. Please this the very last time. I think we could merge this PR.
Then, I could move forward to refactor more. Seeing how you do in the class Plant
, I may want to remove the components of simulators. Instead, the functionality would all be in the environment.
This is an incomplete pull request beginning a refactor of Plant and related environmental classes.