Thread Rating:
  • 0 Vote(s) - 0 Average
  • 1
  • 2
  • 3
  • 4
  • 5
Offering help.
#3
Well you could have fooled me, Marvin! Things generally look really good, and yeah, I've seen some experienced programmers without Unity experience do things... in ways that might make sense elsewhere but very much clash with the Unity SOP, but at first glance I didn't think you were one of those. So... congrats! Although as I'm writing this post and finding a few more things, it is becoming a bit more obvious (Although you stating so much probably creates a bit of a confirmation bias) Wink

That said, UI and input seems a bit odd. For example, querying the input controller to advance the state of a UI script as opposed to just using the built-in event system. Every button having its own class, and not using the built in Button component or something deriving from it, seems a bit unorthodox too. Namely because you're increasing code verbosity while only retaining a subset of the functionality that regular Unity buttons have.

One nitpick is making fields public for the purpose of exposing them to the inspector. That's what the [SerializeField] attribute is for, but I'm guessing you missed that and the knowledge it exists will come as a relief (thanks, official Unity tutorials for always making fields public!).

Next is perhaps debatable, but turning MonoBehaviours into singletons can be a bit evil, since if you're trying to retrieve the instance before its Awake() has been called it returns a null reference. Worse yet, unless script execution order is explicitly set the order can vary between in-build and in-editor, which can be a huge pain to debug. I don't think there's any kind of general standard here, but my practice is to create a MonoBehaviour gatekeeper class that contains a static reference to every persistent would-be-singleton MonoBehaviour, which is then set to initialise before anything else in the script execution order so it can populate those static references before anything else happens.

And since I'm working through the codebase as I write this out, I'm also noticing a bit of lax encapsulation here and there. For example, DoorNameController.GetDoorName returns the text property of a TMPro component, which is then used in AstronautController to check which panel to open. This is, of course, not great. So if some schmuck comes along and decides that, say, those names would look better in ALLCAPS and changes the value in the inspector he'll actually break the program -- which should never happen from editing the contents of a text component. Rather, a separate id value or just a reference on the DoorController itself to the panel it manipulates would be much preferable.

Last is a total nitpick, and again debatable, but there are a few areas that are somewhat verbose versus a LINQ expression. Eg. the body of GameData.FindElementId can be simplified to a single simple line:

Code:
return m_elementList.FirstOrDefault(x => x.m_name == name)?.m_id ?? -1;

Aside from the benefits of expressing the same operation in fewer lines and characters, this is generally more readable. You can speak what's happening left to right just like a sentence. Of course, this is assuming the reader is mostly comfortable with dot notation and null coalescing (which a C# programmer should be), but there's an argument to be made for keeping things approachable for those who don't. And in performance critical code, you generally want to profile LINQ versus traditional loops too.

And I'm sure I'll find a few more things as I work my way through the codebase. But, I feel I should restate again how easy I'm finding it to follow everything in case I'm coming off as a bit too complain-y. That in itself is quite the triumph!

Do you have a Skype or Discord handle you wouldn't mind sharing with me? Would be awesome to be able to chat a bit before I touch anything so that we aren't at risk of walking on eachother's toes and all that.
Reply


Messages In This Thread
Offering help. - by Jeff Graw - 02-16-2019, 02:47 AM
RE: Offering help. - by Marvin Herbold - 02-16-2019, 03:36 AM
RE: Offering help. - by Jeff Graw - 02-16-2019, 04:59 AM
RE: Offering help. - by Marvin Herbold - 02-19-2019, 11:45 PM
RE: Offering help. - by Marvin Herbold - 02-17-2019, 12:10 AM

Forum Jump:


Users browsing this thread: 1 Guest(s)