MRC-CSO-SPHSU / LoneParentsModel.jl

An initial implementation of an ABM for social and child care
0 stars 4 forks source link

more DRY code? #73

Closed mhinsch closed 1 year ago

mhinsch commented 2 years ago

At the moment, with getters/setters, imports, exports and delegation, adding one property to one of the agent blocks requires 10 largely mechanical, repeated additions (the property itself, getter/setter, export in Block, import in Person, export in Person, 2xdelegate in Person, 3 constructors), which seems a little excessive. I'm not sure what the best solution is, but the current state is not ideal, I think.

AtiyahElsheikh commented 2 years ago

Sounds like a macro-based solution is needed

mhinsch commented 2 years ago

Hmm, maybe. It seems a bit pointless, though, to impose modularity and code discipline (explicit import/export) on ourselves only to then use macros to circumvent them again. Let's think a bit about this, but in my experience the less friction there is in changes to the model code the better.

mhinsch commented 2 years ago

Plus, of course, import in Simulate, so 11 additions!

mhinsch commented 2 years ago

Possible solutions:

In addition I think explicit imports of types and methods should be dropped for XAgents. While I generally think it's a nice idea to have explicit imports (although, to be honest, after using it for a bit I find the idea more attractive in theory than in practise...), it's just not very practical for the dozens of methods exported by person. Also, with delegation and using the getter/setter macros most of these methods are explicitly typed, so the risk of mixing up methods is pretty low, I think.

mhinsch commented 2 years ago

One more issue with explicit imports - it's pretty easy to get out of sync by removing method calls and forgetting to remove the corresponding import (or mistakenly assuming that the method is used somewhere else). Especially since code tends to be quite fluid when working on an ABM (you usually keep adding and removing stuff all the time) - which again is also made more annoying by having to add and remove imports. So, while I still find the idea conceptually attractive I'm starting to think this looks more and more like Java's checked exceptions - nice in theory, but too unwieldy to be useful in practise.

AtiyahElsheikh commented 2 years ago

I need examples to be sure about what you are aiming at. Just one thing in mind. I guess it is ok, to have a minimal set of sets/gets (those who are really needed), and add the others later by demand, if ever needed.

mhinsch commented 2 years ago

I need examples to be sure about what you are aiming at. Just one thing in mind. I guess it is ok, to have a minimal set of sets/gets (those who are really needed), and add the others later by demand, if ever needed.

With the current structure every property in one of the Block parts has to have a getter/setter plus all the additional things (delegate, imports, exports, etc.) as otherwise it would not be usable in Simulate (there are a few exceptions for properties that are only used internally, within the Block, but these are rare).

mhinsch commented 2 years ago

I've thought about this a bit more. I think how we solve this issue will also have consequences for our attempts to keep the simulation itself somewhat modular (in the sense that it is easy to add or swap out parts of it). While it's probably a bit ambitious to try to solve all of this at once (and now) this is in my opinion a good argument for keeping the agent modules as separate structs for now.

My proposal for a solution to the issue at hand (high friction for adding/removing agent properties due to excessively repetitive code) would therefore be the following:

It's not a perfect, nor even a great solution, but it solves the problem and lets us focus on implementing the model for now.

@AtiyahElsheikh unless you have any concerns with this I'll go ahead and implement it.

mhinsch commented 2 years ago

BTW, this leaves the door open for a solution that overloads get/setProperty, so that we can use person.something in simulate (will need to do some benchmarking first, though).

mhinsch commented 2 years ago

@AtiyahElsheikh sample implementation above. For now helper functions implemented within the blocks (e.g. isSingle) have to use the same API for field access, but we can re-enable direct access later (needs more macros).

AtiyahElsheikh commented 2 years ago

Ok. Looks good.

AtiyahElsheikh commented 2 years ago

A question jumped to my mind. Why is Kinship not a module any more?

mhinsch commented 2 years ago

It would make things a lot more complicated and given that all the access functions live in XAgents anyway, I think it would be a bit pointless to keep the modules.