{"action":"created","issue":{"url":"https://api.github.com/repos/Interkarma/daggerfall-unity/issues/924","repository_url":"https://api.github.com/repos/Interkarma/daggerfall-unity","labels_url":"https://api.github.com/repos/Interkarma/daggerfall-unity/issues/924/labels{/name}","comments_url":"https://api.github.com/repos/Interkarma/daggerfall-unity/issues/924/comments","events_url":"https://api.github.com/repos/Interkarma/daggerfall-unity/issues/924/events","html_url":"https://github.com/Interkarma/daggerfall-unity/pull/924","id":365874050,"node_id":"MDExOlB1bGxSZXF1ZXN0MjE5Njg1MzQx","number":924,"title":"Cleaning up files","user":{"login":"Allofich","id":19624336,"node_id":"MDQ6VXNlcjE5NjI0MzM2","avatar_url":"https://avatars2.githubusercontent.com/u/19624336?v=4","gravatar_id":"","url":"https://api.github.com/users/Allofich","html_url":"https://github.com/Allofich","followers_url":"https://api.github.com/users/Allofich/followers","following_url":"https://api.github.com/users/Allofich/following{/other_user}","gists_url":"https://api.github.com/users/Allofich/gists{/gist_id}","starred_url":"https://api.github.com/users/Allofich/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Allofich/subscriptions","organizations_url":"https://api.github.com/users/Allofich/orgs","repos_url":"https://api.github.com/users/Allofich/repos","events_url":"https://api.github.com/users/Allofich/events{/privacy}","received_events_url":"https://api.github.com/users/Allofich/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":9,"created_at":"2018-10-02T12:28:25Z","updated_at":"2018-10-02T16:28:25Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/Interkarma/daggerfall-unity/pulls/924","html_url":"https://github.com/Interkarma/daggerfall-unity/pull/924","diff_url":"https://github.com/Interkarma/daggerfall-unity/pull/924.diff","patch_url":"https://github.com/Interkarma/daggerfall-unity/pull/924.patch"},"body":"@Interkarma, you don't have to accept this if you want, this is just some basic cleaning up of files to try to bring them into a consistent and attractive look. I installed a refactoring tool and, while playing around with it, decided to also take the opportunity to give the various .cs files a lookover. Progress on DF Unity is so fast, with several different people working on this project, I feel like it we need to take some time to clean up the mess and get a reign on things. I think you also have been wanting to do so.\r\n\r\nI also want to ask your opinion about various stylistic choices, to decide how to go forward. This is a bit long, just answer whichever ones you feel like.\r\n\r\nFor this PR, I went through the first .cs files, the ones in API/Save. These are also probably the cleanest files already, as they are very straightforward. They seem like a good testing ground for this.\r\n\r\nI went through and fixed them according to some rules that I thought would be fairly uncontroversial.\r\n\r\n- Remove unused using statements\r\n- Remove trailing whitespace\r\n- Add a trailing line (this is mainly to get rid of the little red icon on GitHub, but is also a convention that apparently is convenient for Unix. We could also go the other way and remove all trailing lines. But at the moment we have a mixture and should probably do one or the other)\r\n- Fix non-uniform spacing. In general I'm using the results you get with Visual Studio's auto-format (CTRL-K and CTRL-F to format selection), which I think everything I've submitted has generally followed)\r\n- Put declarations of same-type variables (put strings together, bools together, etc.) together, unless they are better organized some other way\r\n- Fix other non-uniformities. For example, I found just one file that had the using statements within their own #region, so I removed that.\r\n\r\nI wrote in each commit message the changes that were made.\r\n\r\nAny objections to these? Is it OK to continue applying them to other files? I'm okay with whatever style rules are your preference. I don't intend to try to impose any style rule that you don't like. This is largely cosmetic, after all, but we may also uncover some bugs, or just feel better about the state of the project with cleaned up files, which I think is itself a worthwhile, motivational benefit to have.\r\n\r\nIn addition to the rules I applied above, can I ask your opinion about some other cases? If you aren't sure about any of them right now, we can just hold off on doing anything about them for now, or you could change your mind later, or whatever is good for you.\r\n\r\n- Superfluous `else`, `this`, casts, etc. For example:\r\n\r\n```\r\nif (a)\r\n   something;\r\n   return;\r\nelse\r\n   something\r\n```\r\n\r\nwhere the \"else\" is superfluous, but my guess is you want these for readability. I think you have already said you want to keep casts in things like ```(float) something / 4f``` for readability, and I'm guessing readability is also the reason for using \"this\" in, for example,  ```other.streamLength = this.streamLength; ```\r\n\r\nSo, my assumption is that you would like to keep these. Any comment?\r\n\r\n- Using types like `System.Byte` instead of `byte`\r\n\r\nIn general, the files I went through use the `System.` versions, although Visual Studio offers the non-System ones as being simpler. There are a few cases, though, where non-System versions are used. Also, I don't recall seeing `System.String` used, just regular `string`. Do you have a rule or guidelines for which ones to use and why?\r\n\r\n- Commented code. I've seen you often fix Unity warnings by commenting out code, but I think you do this for the convenience of the authors, so that they can quickly recover it (and I think that is considerate). But what about commented code that's not in actively being-worked-on code? Over at OpenMW their policy was to not leave in any uncommented code, but do you prefer to keep it? If I come across some old commented code that seems obsolete and has no comment saying it will eventually be restored, is it OK to remove it or shall we keep it?\r\n\r\n- Default value assignments. Declared class members being assigned with values like bools to be false, integers to be 0, objects to be null, etc. that they would receive as defaults automatically. I expect this is for readability or completeness, but this is not done consistently. They are also values that are simply declared and allowed to take the defaults implicitly. What should the rule/guideline be?\r\n\r\nThanks. \r\n"},"comment":{"url":"https://api.github.com/repos/Interkarma/daggerfall-unity/issues/comments/426340096","html_url":"https://github.com/Interkarma/daggerfall-unity/pull/924#issuecomment-426340096","issue_url":"https://api.github.com/repos/Interkarma/daggerfall-unity/issues/924","id":426340096,"node_id":"MDEyOklzc3VlQ29tbWVudDQyNjM0MDA5Ng==","user":{"login":"Allofich","id":19624336,"node_id":"MDQ6VXNlcjE5NjI0MzM2","avatar_url":"https://avatars2.githubusercontent.com/u/19624336?v=4","gravatar_id":"","url":"https://api.github.com/users/Allofich","html_url":"https://github.com/Allofich","followers_url":"https://api.github.com/users/Allofich/followers","following_url":"https://api.github.com/users/Allofich/following{/other_user}","gists_url":"https://api.github.com/users/Allofich/gists{/gist_id}","starred_url":"https://api.github.com/users/Allofich/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Allofich/subscriptions","organizations_url":"https://api.github.com/users/Allofich/orgs","repos_url":"https://api.github.com/users/Allofich/repos","events_url":"https://api.github.com/users/Allofich/events{/privacy}","received_events_url":"https://api.github.com/users/Allofich/received_events","type":"User","site_admin":false},"created_at":"2018-10-02T16:28:25Z","updated_at":"2018-10-02T16:28:25Z","author_association":"CONTRIBUTOR","body":"> Also not convinced by auto-refactoring member definitions into groups. I know for my code I organise them for maximal readability and maintainability... guess I consider myself superior to an algorithm! Anyway I would rather not have that bit done to my classes.\r\n\r\nYou mean sticking strings together, bools together, etc.? That was actually not an auto-format, that was just me, and just for one file I think, where in that case I thought it would look better. I wasn't sure about whether or how strict to apply that as a guideline for other files.\r\n\r\nDoing this later is fine. It seems okay to me to do cleanups mid-project, though, like a periodic cleanup, sort of like going to the dentist every once in a while, to keep up good habits. I've heard how people are more reluctant to litter in a place that's already litter-free (or something like that). Something along those lines.\r\n\r\nAnyway, I'm taking this down for now. Interkarma, don't worry about it for now. @ajrb, @MeteoricDragon, @numidium, I suggest we keep the PRs to a minimum for a little while, give Interkarma a little breathing room. (Interkarma, if I'm off-base here, feel free to override what I'm saying)\r\n\r\n> Oh, and I also learned something new about c#... so don't need the \"new thing[]\" for static assignment of arrays.\r\n\r\nYeah, I don't know what you use for coding, but that was marked for me in Visual Studio. I don't know if Visual Studio detected it by itself or if it was part of \"Refactoring Essentials for Visual Studio\", which is a free tool I installed today and am playing around with, and which led me to do this PR in the first place."}}
{"actor":{"display_login":"Allofich"}}