Is your request related to a problem? Please describe.
I've been cleaning up the code base recently and unfortunately I found out that the module BDI is really in need of a big refactoring. I've already simplified a lot the code of this module but I currently cannot spend more time on it so here is everything that I found out if someone wants to continue:
[ ] The most important is the lack of class hierarchy, there's tons of duplicated code because the different mental states and emotion are not linked together by anything so many functions are rewritten 4/5 times almost identically at the exception of a type. Having a proper class hierarchy could lead to reducing the overall quantity of code by a factor of 2 or more I think
[ ] many variables are comprised between -1 and 1 or 0 and 1, but this is not written in their description, the only information about it is found here
[ ] add those information in the documentation of the variables themselves
[ ] maybe make sure those ranges are enforced directly in the setters ?
[ ] if the setters include range checks, then we can remove those checks in the functions using those variables
[ ] internally there's a variable called LAST_THOUGHTS_SIZE that seems to be a buffer size for LAST_THOUGHTS (thinking for the users) used only when adding thoughts
[ ] it is not explained anywhere that this list of thoughts has a fixed size while it has repercussions on the result of the different algorithms. So it should be added in the description of the variable thinking as well as in the architecture description as it's a component of the main algorithm
[ ] It should be possible to set this parameter as its value is currently arbitrary. It could just be a variable of the architecture for example
[ ] The different equals functions in Predicate are pretty similar and could probably be refactored
[ ] We could probably spare a lot of null checking everywhere in the code by initializing values in Predicate (and actually many other objects everywhere) with an empty map as it seems, it being null doesn't carry any information. If we do so we could also add a check in the setter that raises an exception in case of null value, just to be sure.
[ ] In CopingStatement there facets that are lists of predicates: new_desires, delete_desires etc. as well there single object equivalent new_desire, delete_desire etc. I don't see any reason to keep those two concurrent facets as to me they are meant to do exactly the same thing. I understand that this could be a little bit easier for the user in case they only want to add or remove one object, but in the code base it leads to a lot of code duplication and I think we should probably merge those. Either manually once in the code by appending the single objects into the lists of objects, so nothing changes for the users, or simply by removing the single objects facets and only keeping the lists.
[ ] In Emotion there's a facet called name, this forces the users to call the actions with the parenthesis only (cannot name the facets as name is a reserved word). I think it should be renamed to something else so users can call actions with named facets.
[ ] there's a magic number in SimpleBdiArchitecture: 0.00028, I don't know what it represents but it used in some functions to calculate the decay. I think that this decay calculation could be refactored and this number should be a named constant.
[ ] In the same fashion, the intensity is recalculated many times following the same pattern so it should get it's own function to do so.
[ ] I let a few TODO comment on all the parts of the code that I found dubious, many of which I think are creating bugs but I can't spend more time investigating so I let it as it was before
Is your request related to a problem? Please describe. I've been cleaning up the code base recently and unfortunately I found out that the module BDI is really in need of a big refactoring. I've already simplified a lot the code of this module but I currently cannot spend more time on it so here is everything that I found out if someone wants to continue:
LAST_THOUGHTS_SIZEthat seems to be a buffer size forLAST_THOUGHTS(thinkingfor the users) used only when adding thoughtsthinkingas well as in the architecture description as it's a component of the main algorithmvaluesin Predicate (and actually many other objects everywhere) with an empty map as it seems, it being null doesn't carry any information. If we do so we could also add a check in the setter that raises an exception in case of null value, just to be sure.CopingStatementthere facets that are lists of predicates:new_desires,delete_desiresetc. as well there single object equivalentnew_desire,delete_desireetc. I don't see any reason to keep those two concurrent facets as to me they are meant to do exactly the same thing. I understand that this could be a little bit easier for the user in case they only want to add or remove one object, but in the code base it leads to a lot of code duplication and I think we should probably merge those. Either manually once in the code by appending the single objects into the lists of objects, so nothing changes for the users, or simply by removing the single objects facets and only keeping the lists.Emotionthere's a facet calledname, this forces the users to call the actions with the parenthesis only (cannot name the facets asnameis a reserved word). I think it should be renamed to something else so users can call actions with named facets.SimpleBdiArchitecture: 0.00028, I don't know what it represents but it used in some functions to calculate the decay. I think that this decay calculation could be refactored and this number should be a named constant.TODOcomment on all the parts of the code that I found dubious, many of which I think are creating bugs but I can't spend more time investigating so I let it as it was before