Customization of countries list

[freakdragon]

This improvement will allow to customize countries list in more ways.

class CustomCountryManager: CountryManager {
    //By overriding this method you can remove any country or add, or customize sorting, or even set your custom country class item
    override open func loadCountries() throws {
           let countryCodes = try! getCountryCodes()
        
           // Clear old loaded countries
           countries.removeAll()
        
           // Request for fresh copy of sorted country list
           let sortedCountries = countryCodes.map { Country(countryCode: $0) }.sorted { $0.countryName < $1.countryName }
                
            //something like this
           sortedCountries.removeFirst()

            countries.append(contentsOf: sortedCountries)
    }

    //By overriding this method you can put any country codes not only from plist of library
    open func getCountryCodes() throws -> [String] {
            return []
    }
}

This will allow to use code like this:

   let countryManager = CustomCountryManager()
   do {
       try countryManager.loadCountries()
   } catch {
       #if DEBUG
           print(error.localizedDescription)
       #endif
   }
   CountryManager.shared = countryManager

[codecov-commenter]

Codecov Report

Merging #76 (c64ec37) into master (5b451a6) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #76      +/-   ##
==========================================
+ Coverage   94.06%   94.08%   +0.01%     
==========================================
  Files          17       17              
  Lines        1214     1217       +3     
==========================================
+ Hits         1142     1145       +3     
  Misses         72       72              

Impacted Files	Coverage Δ
CountryPicker/CountryPicker/CountryManager.swift	87.30% <100.00%> (+0.63%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b451a6...c64ec37. Read the comment docs.

[SURYAKANTSHARMA]

1 comment rest looks fine

[SURYAKANTSHARMA]

Shouldn't we throws backs instead of generating crash here.
let countryCodes = try! getCountryCodes()
uses
let countryCodes = try getCountryCodes() because loadCountries can throw itself

[Sharkesm]

If we have a single shared instance in place why do we need to reassign CountryManager? And, If I'm not mistaken the main reason for having init() inaccessible is that no one else could re-create the initial shared CountryManager instance.

What do you think of this approach? @SURYAKANTSHARMA

cc/ @freakdragon

   let countryManager = CustomCountryManager()
   do {
       try countryManager.loadCountries()
   } catch {
       #if DEBUG
           print(error.localizedDescription)
       #endif
   }
   CountryManager.shared = countryManager

[SURYAKANTSHARMA]

If we have a single shared instance in place why do we need to reassign CountryManager? And, If I'm not mistaken the main reason for having init() inaccessible is that no one else could re-create the initial shared CountryManager instance.

What do you think of this approach? @SURYAKANTSHARMA

cc/ @freakdragon

   let countryManager = CustomCountryManager()
   do {
       try countryManager.loadCountries()
   } catch {
       #if DEBUG
           print(error.localizedDescription)
       #endif
   }
   CountryManager.shared = countryManager

CountryManager.shared = countryManager

Good Catch @Sharkesm
@freakdragon could you explain why you need to expose init() of singleton because then it became anti pattern. Could you think of other approach. I also think that doing this way can be produce many bugs in long 🏃‍♀️ . wdyt

[freakdragon]

May be I didn't fully understand something, but when I tried to override CountryManager with closed init function, I couldn't do it. And without it I couldn't extend CountryManager class for overriding of loadCountries() function. Am I wrong?

[freakdragon]

CountryManager.shared = countryManager allows me to change singleton for example in my AppDelegate and use it in my whole app further.

[SURYAKANTSHARMA]

on. Am I wrong?

Sure. Nothing is wrong in your approach. It just that we are thinking of if we can do better because Singleton pattern don't allow to make init public https://refactoring.guru/design-patterns/singleton/swift/example. Read Example.swift. You will better understand.

[freakdragon]

If you'll check analog libraries (iOS and Android) you can notice that most of those have exludedCountries. That must be implemented as I think. My approach can be not so usefull as adding of exluded countries. It can be not so difficult to implement as I think. I chose more simple way to achieve this. But excluded countries can be useful for lot of library users.

[SURYAKANTSHARMA]

@Sharkesm wdyt ?

[freakdragon]

Also you can change/add delegate which will have methods getCountryCodes() and loadCountries(). If those not implemented in delegate than using your default implementations.

[Sharkesm]

Also you can change/add delegate which will have methods getCountryCodes() and loadCountries(). If those not implemented in delegate than using your default implementations.

Yes, I agree with this we could actually move those into delegate methods

[freakdragon]

@Sharkesm and what do you think about excluded countries functionality?

[Sharkesm]

@Sharkesm and what do you think about excluded country's functionality?

@freakdragon You raised a valid point on allowing users to have their own country loading implementation, though I reckon we could expose certain functions to exclude countries as expected without breaking the singleton practice, or else we could give a look at other implementation that could satisfy your proposed requirements.

[freakdragon]

@Sharkesm there will be enough to move methods getCountryCodes() and loadCountries() to delegate. After implementing of this functionality we can think about exluded countries if it will be neccessary for somebody.

[Sharkesm]

@Sharkesm there will be enough to move methods getCountryCodes() and loadCountries() to delegate. After implementing of this functionality we can think about exluded countries if it will be neccessary for somebody.

Yes, I agree with this then it's a green light from my end 🚀 🤖

[SURYAKANTSHARMA]

Any update here @freakdragon

[freakdragon]

Unfortunately, I don't have time to implement now. Working two jobs. If I'll have some free time, than I'll think how to implement it.

[SURYAKANTSHARMA]

Closing for now Feel free to reopen if needed