Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

break GasPropertiesModel into smaller pieces #112

Closed
pixelzoom opened this issue Jun 7, 2019 · 1 comment
Closed

break GasPropertiesModel into smaller pieces #112

pixelzoom opened this issue Jun 7, 2019 · 1 comment
Assignees

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Jun 7, 2019

Of the 123 .js files in this sim, all but 5 are less than 300 lines, and they are:

325 ./common/view/GasPropertiesScreenView.js
362 ./diffusion/model/DiffusionModel.js
377 ./common/model/CollisionDetector.js
557 ./common/view/GasPropertiesIconFactory.js
692 ./common/model/GasPropertiesModel.js

I'm please with all of these except GasPropertiesModel.js. It's not huge, but it's a lot to digest in one "chunk". So I had the thought that I might break it up into smaller "sub models" related to PV = NkT. I started exploring this in my working copy, and it looks promising. My current thought is:

ParticleSystem - responsible for number of particles (N) and managing particles
PressureModel - responsible for pressure (P)
TemperatureModel - responsible for temperature (T)
BaseContainer - already exists, add responsibility for volume (V)

@veillette FYI. I don't think this is reason to delay the preliminary code review #109. But I may be working on GasPropertiesModel, and it may get divided up a bit.

@pixelzoom
Copy link
Contributor Author

This is done. GasPropertiesModel has been broken up as described in #112 (comment). I'm much happier with this. It's more modular, easier to understand, and should be more maintainable.

I reviewed DiffusionModel to see if something similar would be useful there. But it's a small model (362 lines), much less complicated, and I think it's small enough to digest "as is".

@veillette FYI, this is in good shape for #109.

Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant