Good practice improvements discussion

[ghost]

Hi, sorry for opening this issue here but I'm not sure where to ask. Happy to move discussion somewhere else if not happy to have it here.

---

1. Adding SwiftLint .
   You may be familiar with it, it's a very powerful tool to enforce syntax and some concepts within a project. I think it's especially useful on larger projects, or projects with more contributors.

👍

• helps develop good practice
• can prevent some serious developer errors from happening
• formats code in a predefined manner (think tabs vs spaces, indentation fixes etc)

👎

• needs to be installed on developer machine, so if not already present, it's an extra hurdle before starting
• can enforce rules some people may not like. This can be mitigated by following Swift good practice guidelines, which will result in benefiting more novice Swift devs.

---

2. Private Outlets
   Generally it is good practice for the UI to be only accessible via it's owner (VC, view) and not by the parent of the UI owner. Although they are constants and cannot be overridden, they are reference types and as such can be modifier by an external user. For example, when creating a view, try calling view.outletElement.removeFromSuperview(). This usually results in outletElement becoming nil. Given it's a forced unwrapped property, this will cause a crash the moment it's accessed by its actual owner (the view). This is just one example, potentially there are many ways developer error can cause unintended consequences for exposed UI elements.

👍

• enforces separation of concerns by exclusively allowing access to UI elements to the elements owner
• safer

👎

• harder to test UI elements

---

3. Using private instead of fileprivate
   Brief description from HackingWithSwift
   Swift Docs on Access Control
   fileprivate allows something to be accessible by anything within the same file. Looking through the project, that is hardly ever the case and private would be a more appropriate access control setting.

👍

• follow good practice and most appropriate setting for requirement
• cleaner text which makes more important details to be slightly less obstructed by syntactic sugar

👎

• no difference in how code is build
• no benefit for encapsulating logic. It's already encapsulated.

---

These are all changes I'm happy to make if there is an agreement to go ahead. They may cause a lot of syntactic changes so could also be implemented little by little while touching parts of code that are affected.
SwiftLint is great for number (2) as it can prevent building project altogether if there is an agreement to consider that a serious issue.

I look forward to a debate on the merits of these ideas 😄

[CristiHabliuc]

Thanks for opening this up!

Here are my comments:

1. Adding SwiftLint .

I used it and it didn't give me any extra advantages. If you want to add it go ahead, but it's not necessary in my opinion. Also, it should be seamless to integrate using CocoaPods and a run script.

2. Private Outlets

For faster development, I don't do that, because by convention you should never assign anything else to an @IBOutlet or manually call an @IBAction. If you want to make them private in your code, go ahead, as it won't cause anything bad, it's just a waste of time sometimes.

For example, when creating a view, try calling view.outletElement.removeFromSuperview(). This usually results in outletElement becoming nil.

This is a different issue. It's mostly caused by the @IBOutlets being set as weak vars by default. If you plan on removing a view and adding it back later, you should remove weak and let it be strongly referenced by the controller.

3. Using private instead of fileprivate

I think this solution is definitely debatable. I use fileprivate a lot, as it's very useful when using extensions in the same file. I agree about the enforcement on separation, but in most cases it's way too much. I probably overuse it, since it's easier for me, but as you can see I (most of the time) have very few declarations in the same file.

Conclusion

These are all changes I'm happy to make if there is an agreement to go ahead.

My thoughts are you shouldn't refactor the project as it would take some valuable time that should be invested in new features that we plan on having. But if you want to implement these in the future code you write, be my guest 👍

[CristiHabliuc]

@colcalex while cleaning up the repo a bit after a while, I think some of the suggestions you raised make sense in going forward. I still don't think we should refactor the old code, but if you have the time and patience, no problem.

• SwiftLint - my opinion remains pretty much the same. I think we should only add it when it makes the most sense to do so (if we have a good reason to use some extended checks), but for the codebase we have so far I think we're good.
• Private Outlets - we could go ahead with them in new code, since it makes the most sense looking from outside the controller class (as black box as possible)
• Private instead of fileprivate - I changed my mind about this after using it for the past time in other projects, I think this one is very valuable

Thank you again for raising these up!