INET-Complexity / housing-model

Agent-based model of the UK housing market.
MIT License
39 stars 26 forks source link

Reorg project to match Maven Java standards #38

Closed davidrpugh closed 7 years ago

davidrpugh commented 7 years ago

@adrian-carro This PR will reorganize the housing model code so that it matches Maven standard project template..

davidrpugh commented 7 years ago

@adrian-carro We need to create a pom.xml file that contains the dependencies for the housing model. Once we have a pom file we should be able to delete the /lib directory containing these jar files. No reason that we should be version controlling these files.

davidrpugh commented 7 years ago

@adrian-carro After the reorg it seem to be a lot off issues with class trying to access private/protected data of other classes. Doesn't look like there are any getters (and in some case setters) for the relevant properties.

adrian-carro commented 7 years ago

@davidrpugh Thanks for doing this. Please, let me know whenever this PR is ready (up and running) for me to review and merge it. In any case, I'll need some time before merging it, since there are a number of people using this code and I want to make sure I make everybody aware of this reorganisation before proceeding. Regarding the setters and getters, you are completely right, but bear in mind that 1) this is a pretty large piece of code with very few getters/setters when it was inherited, 2) that the organisation of the different classes might change in the near future as we will be trying to reduce the number of references between multiple objects, and 3) that a deep cleaning and refactoring of the code has not been the priority so far (though we obviously understand it is badly needed!).

davidrpugh commented 7 years ago

@adrian-carro Thanks for the background info. Will let you know when the PR is ready to merge. I will need to write getters (and possibly setters) in order for the reorg to work. This will probably be a painful merge for everyone using the code but it is definitely necessary.

davidrpugh commented 7 years ago

@adrian-carro I have added the necessary getters and changed the file paths to the data (which now lives in the resources directory). I compiled and ran the model and it looked OK but you should see if everything looks good to you.

Still need to finish the pom.xml file and delete the '/lib' folder.

davidrpugh commented 7 years ago

@adrian-carro This PR is ready to merge. Everything now works on my machine not that I have removed the /lib directory containing the external .jar files. All necessary dependencies that were available via Maven have been moved to the pom.xml file and will be automatically downloaded when the project is built by Eclipse or Intellij (or whatever IDE you are using for your work).

Unfortunately, recent versions of Mason are not available on Maven. In the past we had been VC the Mason jar file. I went ahead and deleted the jar file for consistency and also to possibly help with licensing issues. If you think it is too onerous on users to have them install Mason separately in order to use our code, then we can always add the /lib/mason.19.jar file back into this branch prior to the merge.

Feedback is much appreciated...

adrian-carro commented 7 years ago

Great! In the following days (as soon as I find some time to do it) I'll review it, check that everything works fine in a couple of common IDEs, warn whoever is working with the code, and merge the changes.

davidrpugh commented 7 years ago

@adrian-carro I expect that there will be IDE issues due to preexisting IDE project files. I deleted all of the Intellij and Ecplise IDE specific project files and then created a fresh Maven project using the housing model source code. Let me know if you get into trouble...

davidrpugh commented 7 years ago

@adrian-carro Ping! Just looking for an update to the status of this PR. I have some free time this weekend and would like to get this merged so that I can move on to cleaning up other parts of the code base.

adrian-carro commented 7 years ago

@davidrpugh Sorry about the delay, I've been busy with other stuff, but this is my priority for today!

adrian-carro commented 7 years ago

Hey @davidrpugh, I've just been checking this pull request and everything looks pretty much fine to me with the exception of MASON. Indeed, in order to be allowed to compile and run the code, I had to add the mason jar file somewhere in the project, then use maven to install it locally, and finally add it to the pom file as a (local) dependency... is there any simpler way to solve this? Looking forward, I'm pretty confident we will end up dropping mason, given that we are not really using it for anything but to make the code less readable. But still, we won't drop it just now, so we need a simple way for users to run the code using mason. Any ideas/thoughts on how to achieve this? In case there are some steps that users have to follow to install mason, maybe the best idea is to include a clear description of those steps in the Readme.md file? Once this is solved, I will directly merge the changes.

davidrpugh commented 7 years ago

@adrian-carro I have just added the Mason jar back into the repo in the /lib folder. I am trying to figure out how to add it to the poml.xml as a local dependency. Any ideas?

davidrpugh commented 7 years ago

@adrian-carro Can you test drive this and see if it works? I am having some weird Windows issues on my machine that I think is an issue with my laptop and not with this fix...but do let me know if you run into issues...for reference here was the instructions I followed...everything seems to work on my end now. All I needed to do was press the play button to get the GUI charts to start working properly...

adrian-carro commented 7 years ago

Oh, perfect! It works out the box now. If you are fine with it, I'll just merge the changes.

davidrpugh commented 7 years ago

Please!

Sent from my iPhone

On Jun 19, 2017, at 10:13 PM, Adrián Carro notifications@github.com wrote:

Oh, perfect! It works out the box now. If you are fine with it, I'll just merge the changes.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.