search

jdmonin / JSettlers2

Java Settlers project home, downloads, and GPLv3 source code. To download the latest version as a JAR, see https://github.com/jdmonin/JSettlers2/releases/latest .
http://nand.net/jsettlers/
GNU General Public License v3.0
157 stars 63 forks source link

Replace SOCResourceConstants with Data.ResourceType value constants #42

Closed generateui closed 6 years ago

generateui commented 6 years ago

Introduce a protobuf file with ResourceType enumeration. Use values of enumeration to do a 1-on-1 replacement with SOCResourceConstants value.

The next steps are:

  1. Replace the for-i loops with foreach-loops for Resources
  2. Disambiguate Gold and Unknown
  3. Move naming logic into newly introduced Resource classes
  4. Remove SOCResourceConstants

PS You probably want to import a directory into your IDE project file. The build/generated/source/proto/main directory I have marked as "generated sources root", so IntelliJ picks it up as source. I Excluded the generated source code since including that into the repo seems to be a bad pattern.

jdmonin commented 6 years ago

Thanks! This looks mostly straightforward, and a gentle introduction to the conversion.

There are some changes here like SOCPlayerNumbers.addNumberForResource and SOCStringManager.getSOCResourceCount where the javadoc says for example ResourceType#CLAY but code expects ResourceType.CLAY_VALUE. The javadocs there should have _VALUE. Do you want me to fix those, or would you rather (by pushing a new commit onto your protobuf branch)?

generateui commented 6 years ago

I will fix them. I think it makes sense that PR issuers are responsible to provide a PR which is correct.

jdmonin commented 6 years ago

Thanks again. It's been a while and didn't want to keep you waiting, so I've merged this to v3 as-is; please merge that and send a new PR for the javadoc fixes.