Refactored own class for receiving app properties

[Someone0nEarth]

Hello!

Refactoring for encapsulation of Properties.getValue() calls into one place.

• cleaner code
• remove redundant / duplicate code
• much easier to set properties for local on-the-device testing (main purpose for this refactoring)

Have fun,
someone

[JosephAbbey]

Should this be managed through the HomeAssistantService (service.settings), that way we will still only have one?

[JosephAbbey]

Why do we use a function instead of just a public property?

setttings.apiKey() as Lang.String vs settings.apiKeyas Lang.String

I don't particularly have a preference either way, just wandering.

[Someone0nEarth]

Hello!

Yes, using a public property would be fitting too. Could then also be part of Globals.

Using a class with methods for encapsulating the properties have the "benefits" for more comfy unit testings and refactorings. But yes, there are no unit testing atm in this project and the refactoring capabilities of Visual Studio Code for Monkey C is very limited. So you could say, using properties would be a better fitting solution.

I although had in mind using the settings class for in-app settings, where the user can be change settings like the item alignment or showing the icon/labels in the running app itself. But this could be done when implementing this.. .

In the end, I prefer here methods more than properties, because its more fitting in terms of OOP. But for the current circumstances, properties are good enough :)

[philipabbey]

Refactoring for encapsulation of Properties.getValue() calls into one place.

• cleaner code
• remove redundant / duplicate code
• much easier to set properties for local on-the-device testing (main purpose for this refactoring)

Okay, just so I'm clear, the plan is to create a brand new file Settings.mc so that Settings.get().apiUrl() can replace three instances of:

Properties.getValue("api_key") as Lang.String

And in doing so add another layer of indirection to the code, soley to provide the type? My current feeling is that Settings.mc is just a wrapper with syntactic sugar for Properties that provides too little added value. I do think there is such a things as "over factorisation" of code. It induces a form of obfuscation, in this case yet another file to open in order to understand where the API key (and others) is actually sourced from. On a small code base like this you could argue "so what?" But codebases grow, and future supportability eventually becomes "technical debt".

I'm missing the value add.

• cleaner code

Extra layer of indirection equals obfuscation. Also you now have one extra class with 5 extra methods, is the code really cleaner?

• remove redundant / duplicate code

Only a confirmation of type, that's all really.

• much easier to set properties for local on-the-device testing (main purpose for this refactoring)

Ah, now I can see where this is coming from based on previous conversations. You can do Propertities.setValue(), so you really don't need this class if you are trying to avoid publishing a private beta app on the AppStore. It already achieves the desired outcome without having to write 5 setters too.

Aside: This is a topic where I feel softare engineers may have lost the plot a little. When Dr Wheeler made his famous quote: "All problems in computer science can be solved by another level of indirection." I don't think he considered for one moment how he would encourage unnecessary indirection just for the cleverness of it, i.e all problems "…except for the problem of too many levels of indirection".

So I hope you'll understand my reticence to pull this code presently. I'm of the personal opinion that it has yet to meet a threshold or worthiness where it adds more value than it subtracts. It is "nice" don't get me wrong.

Sorry to sound negative.

[Someone0nEarth]

Hello!

Refactoring for encapsulation of Properties.getValue() calls into one place.

• cleaner code
• remove redundant / duplicate code
• much easier to set properties for local on-the-device testing (main purpose for this refactoring)

Okay, just so I'm clear, the plan is to create a brand new file Settings.mc so that Settings.get().apiUrl() can replace three instances of:

Properties.getValue("api_key") as Lang.String

And in doing so add another layer of indirection to the code, solely to provide the type? My current feeling is that Settings.mc is just a wrapper with syntactic sugar for Properties that provides too little added value. I do think there is such a things as "over factorisation" of code. It induces a form of obfuscation, in this case yet another file to open in order to understand where the API key (and others) is actually sourced from. On a small code base like this you could argue "so what?" But codebases grow, and future supportability eventually becomes "technical debt".

I'm missing the value add.

• cleaner code

Extra layer of indirection equals obfuscation. Also you now have one extra class with 5 extra methods, is the code really cleaner?

Depends on what is understood as "cleaner" regarding the collaborations circumstances of a project. If there is a relevant likelihood for a project that it has many different person dealing with the code (like every open source project and many closed source ones (for example in bigger companies)), then, for me, there are like four main factors for indicating cleaner code:

• How smooth it is for a person to read the code. In general, when lines of code can be read more like a good written "natural" text, it is smoother than when reading more technical lines (even for an experienced coder). A simple example for showing the point: The line if(personA.isOlderThan(personB))... is a little bit smoother than if(personA.getAge() > personB.getAge())....
• The amount of redundant values and "magic numbers": If you want to change one value, for example the font size of a text, you wouldn't be expecting more then one place to do it.
• How good the IDE can be used to deal with the code. IDEs are the workbench and tooling for coders. For OOP languages, this often means writing code in stricter OOP terms leading to better IDE support for features like refactoring, finding references and so on.
• How good the code can be tested (and "test code" is "production code" too.. so the first too points are although valid for it)

• remove redundant / duplicate code

Only a confirmation of type, that's all really.

And the property names like "api_url" were redundant too.

Aside: This is a topic where I feel software engineers may have lost the plot a little. When Dr Wheeler made his famous quote: "All problems in computer science can be solved by another level of indirection." I don't think he considered for one moment how he would encourage unnecessary indirection just for the cleverness of it, i.e all problems "…except for the problem of too many levels of indirection".

Hehe true, I am also not a fan of too many level of indirections.. but I am don't like reading assembler code too ;)

So I hope you'll understand my reticence to pull this code presently. I'm of the personal opinion that it has yet to meet a threshold or worthiness where it adds more value than it subtracts. It is "nice" don't get me wrong.

Its fine. As I answered already to Joseph's review comment.. that using "settings properties" (for example in the Globals file).. would be a better fitting solution for the current circumstances.

Sorry to sound negative.
:)

Have fun,
someone

[philipabbey]

Something that could push this over my personal "threshold of worthiness" comes from the conversion of the two booleans in the application settings to a more descriptive item selection from a pair of drop down menus.

See https://developer.garmin.com/connect-iq/core-topics/properties-and-app-settings/#arraysettings

The settings would then provide enumerated types like Settings.LEAN_UI, Settings.SubMENU, Settings.TEXT_LEFT, Settings.TEXT_RIGHT to match the values used in the listEntry.

[Someone0nEarth]

Something that could push this over my personal "threshold of worthiness" comes from the conversion of the two booleans in the application settings to a more descriptive item selection from a pair of drop down menus.

See https://developer.garmin.com/connect-iq/core-topics/properties-and-app-settings/#arraysettings

The settings would then provide enumerated types like Settings.LEAN_UI, Settings.SubMENU, Settings.TEXT_LEFT, Settings.TEXT_RIGHT to match the values used in the listEntry.

I liked this. The UX in the settings area could be better with this. Later, I should have time for implementing this (I would do this in this branch).

[philipabbey]

And another thought. Application.AppBase.onSettingsChanged() could then ask your settings class to update its values from the Properties backing store. Then there would be single point for responding to updates instead of locally cached values.

[philipabbey]

Due to conflicts and a desire to use a static class rather than a factory class pattern, this work will be completed under https://github.com/house-of-abbey/GarminHomeAssistant/tree/37-add-device-battery-logging-and-settings-class.