Seneral / Node_Editor_Framework

A flexible and modular Node Editor Framework for creating node based displays and editors in Unity
https://nodeeditor.seneral.dev
MIT License
2.01k stars 415 forks source link

Improved documentation and fixed garbage generation in loop. #117

Closed gil-ferraz closed 7 years ago

gil-ferraz commented 7 years ago

Also added a note regarding getting rid of exposed public fields as per C# design guidelines. I didn't do the change per se as the maintainer may or may not agree with the C# guidelines. (I'm new to the repo, so I'm still not sure haha)

gil-ferraz commented 7 years ago

Posting the note here for convenience:

As per C# design guidelines, there should be no exposed (public/protected) fields. To avoid this, they should be converted to proprieties with private backing fields with the [SerializeField] attribute to allow access to Unity's ScriptableObject.

Seneral commented 7 years ago

Thanks! Although now I feel the need to improve the comments anywhere where it's even more needed lol

Regarding the guidelines, I do not see it as critical to implement them in this framework, basically it is API-wise benefitial but it will increase code complexity by quite a bit which will make it harder to understand for beginners. Also, when we implement this here, then in every class (atleast core classes). This also means to break all existing save files (due to changed names), let alone the amount of work involved to do it.

Additionally before merging, can you make sure to replace space-indenting with tabs? Which to choose is subjective but this whole framework is using tabs right now and changing this just hides code changes.

gil-ferraz commented 7 years ago

Thanks, I'm glad you liked the doc changes. It was really just a trial to see if it fit your view of the project. But seeing as you accepted it well, I'd be glad to help with the docs. I will be making contributions as I'm developing my on project using this framework.

As for field exposing, I understand your concerns. But I'd also like to remind that the variable names would stay the same (camelCase, with the first letter being lower-case) , so it wouldn't break save compatibility. The only thing that would change would be the fact that other scripts would access each other with proprieties (CamelCase, with the first letter being Upper-case, thus avoiding naming conflicts).

I find this to be not only more robust, but also easier to navigate the code. For example, for people using Visual Studio, you can see where proprieties and methods are being references, but not fields. So although at first it may seem to increase complexity, we'd also be increasing robustness of the API and accessibility, IMO.

As for the indenting, I'm sorry, I wasn't aware that you were using tab indenting. I've since changed my IDE settings to tab indenting :)

Seneral commented 7 years ago

Well, I've nothing against improved comments. Personally I write method descriptions only using the main summary tag, without extras like referencing classes, simply because I don't see much benefit in it. If you do want to add it though, I appreciate it:)

Yeah, automatic formatting is a big problem unfortunately. I personally still use MonoDevelop without any automatic formatting, but I know some people use VS and may have to turn off autoformatting just when working with this project, so I'd like to hear other opinions about that.

I personally prefer compact code over largely spread code with lots of spaces, atleast to some degree, and like to use empty lines and brackets only when they're needed. I see you got a different style, and as that's generally not a problem, the question is if we do want lots of extra code changes only for styling reasons.

When we decide to do style changes, like adapting to the default VS autoformatting, then I'd suggest to do that all in ONE commit without any documentation or restructuring changes, to maintain clean commits with only one purpose (expecially with as many changes that come with a style overhaul).

Until then I'd again ask you to remove styling stuff and other stuff so it only contains documentation and maybe make seperate commits for other stuff like the for-loop to keep things tidy:)

I will open an issue talking about some general overhauls to the whole project that not necessarily affect content but increase long-term usability and productivity like folder structure (#115), general API changes to conform guidelines (including encapsulation) and said style changes...

Seneral commented 7 years ago

Issue #118 will adress this topic in the future:) For now, this commit is only for documentation changes if you agree.

Seneral commented 7 years ago

Will close this for now, plan to have seperate commits for either of these changes in the future...