kwsch / pkNX

Pokémon (Nintendo Switch) ROM Editor & Randomizer
https://projectpokemon.org/home/forums/topic/48647-pknx-nintendo-switch-rom-editor-randomizer/
GNU General Public License v3.0
338 stars 106 forks source link

Fixed compatible TRs reading incorrectly. #328

Closed TalonSabre closed 1 year ago

TalonSabre commented 1 year ago

TR compatibility for each pokemon had been saving to the same list as TM compatibility, and for some reason it was not working as intended. Seemed like an issue with the way the data merged, so I had TRs read and write to an entirely separate variable. After finishing, and seeing that it built and ran, I tested the impacted features, and confirmed it is working.

TalonSabre commented 1 year ago

If I do anything incorrectly or backwards on GitHub, sorry. This is the first time I have really submitted anything to GitHub for use. This was also my first time using C#, so if there is a better way to write any of my code, I am open to learning. This issue annoyed me enough that I taught myself the basics of C#: SwSh TR wrong offsets. I will probably attempt to fix the issue with evolutions next for the same reason. Everyone who contributed to pkNX, thanks for the awesome work up to this point. Have a nice day!

TalonSabre commented 1 year ago

(besides the small comments)

Realizing I type a lot, I put the important bits in bullet points. If you have the time, you could read/skim over the rest.

I remember being told that keeping documentation is a good idea in general, and adding comments is one way to do that. My assumption is if I don't write comments, I would put the info somewhere like a commit description.

That said, some of the comments are probably redundant anyways, and I only put them in for consistency. For example, where the program iterates through TMs, it has "// TMs" next to it, so I added "// TRs" next to code I added. I guess I probably shouldn't bother with comments unless I know a way to describe the code when isn't self-explanatory?

I'm asking because, even though I don't know how much experience you have coding, you probably know what you're talking about. I'm relatively young, and I'm trying to learn the proper way to do things so that I can find work as a programmer. Part of the issue is that I've never had a teacher/professor say they use comments that much, and I've never been required to add any. As a result:

duckdoom4 commented 1 year ago

(When I mentioned 'small comments', I was referring to my own small issues I pointed out about your commit :p)

I guess I probably shouldn't bother with comments unless I know a way to describe the code when isn't self-explanatory?

That's how I usually comment code. I try to think like this:

I think it's great you're asking for feedback, don't be afraid to ask. That's how we all become better programmers. I'm also self-taught, but with years of experience I'm now working for large companies in the gaming industry, so it's totally possible!

Lastly I want to say, it's generally a good idea to copy the styles and behaviours used in a particular project (like you did now, so great work!). But don't be afraid to change something if you think it's worth changing. For example the //TMs comment someone else left. Seems kinda useless, so why is it there? Can you add a variable name to make it more clear? Or is it already clear enough without the added comment?

(If you want to make a large change, then I would advise making an issue or discussion thread so you can point out why and other collaborators can point out their ideas/concern's)

TalonSabre commented 1 year ago

It has been a couple days since, but I am just now seeing this. I cannot thank you enough for all the feedback. Your first three bullets make a lot of sense. The first one is something that I try to keep in mind at all times. The next two are things that I understand, but don't think about as much. I never thought about the fourth point before, so that is something I will keep in mind as well.

(When I mentioned 'small comments', I was referring to my own small issues I pointed out about your commit :p)

It is still something I struggle with, so I went ahead and asked, but I didn't think that was what you meant xd

duckdoom4 commented 1 year ago

Let get this merged 🎉