sleepdefic1t/ARKKit

Improvements/feedback

Closed this issue · 1 comments

Awalz commented

I have spent some time working with this framework in attempt to use it in the monitoring app. In the end, there were several issues preventing me from using this framework for a production ready app.This lead meyme to write my own framework SwiftyArk. I wanted to share with you some of the feedback I have.

  1. The main issue I had with the app is the networking architecture. You are making asynchronous network calls to the ark node, but the method signatures are synchronous:
let allMainBlocks = ARK.main.allBlocks()

is a synchronous assignment with no error handling. However, behind the scenes this is an asynchronous call that has the potential for many errors, which are never handled. It is not documentation that this assignment will fail, or result in an empty array. The swift way to do this is with a completion handler and error handling:

ARK.main.allBlock() { (error, blocks) in
        if let aError = error {
              /// handle error
        }
        if let allBlocks = blocks {
             // all blocks are returned here and unwrapped
        }
 }

The difference between the two (besides error handling) is the second option returns data in an asynchronous manner. In your API, the assignment occurs on the main thread and will block the entire app until it returns or errrors out.

  1. The networking of this app boils down to using NSString.(contentsOf:URL:) to grab raw data and convert it to a UTF8 string. This api wasn't designed for being used for networking, but rather read files from inside your app. It is not documented anywhere the error handling and doesn't give you any access to HTTP data (headers, statusCodes etc). You should look into using URLSession as it is the primary function for networking in iOS/mac apps. This will also allow you to extend your API to allow for PUT and POST operations, something NSString.(contentsOf:URL:)

  2. Your data types are not very swift like. You declare your internal properties as variables, rather than constants. There is nothing in the API to suggest the types should be modified, so why not make them immutable?

  3. More on the data types, the initialization of them is very un-swift like and leads to several issues. Data types like Account do not have any initializers, but rather assignments in an protocol (which I will get to). This is not very safe as these are declared as ** non-optional options**:

public struct Account: Accountable {
    public var  address: String!
    /// ...

You are basically saying "I guarantee there will be an address for an Account, but you do not provide (or prevent) any initialization, so its not safe:

let account = Account()
let example = account.balance * 10
// CRASH!

I would recommend you get rid of the assignment inside of a protocol and move it directly to an initializer, make the types immutable constants/optionals and prevent any unintentional initialization. Swift 4 introduced the Decodable protocol to direct add an initializer from json. Using it in conjunction with URLSession is a winning combo.

  1. This last one is more of a style comment, but I have noticed you use Swift types in a non-typical way. Your primary API manager is an enum when it should be a class/struct. You also create protocols for each data type (Accountable, Delegable etc) when they essentially do the same thing for each data type. You could create one data protocol that would apply to all datatypes which can be decoded from son (like Decodable)

I hope this helps you. I want this framework to be great. You are doing a lot of great things for the community and I would very much like to see all the features you hope to implement. If you want some inspiration, you can check out SwiftyArk or if you need any advice: andrewjwalz@gmail.com

Andrew

sleepdefic1t commented

Beautiful!
Exactly the direction it needs to go before it's even close to a 1.0 release.

  1. The main issue I had with the app is the networking architecture. You are making asynchronous network calls to the ark node, but the method signatures are synchronous:

This is a priority over in the next releases for exactly the reasons you stated.

  1. The networking of this app boils down to using NSString.(contentsOf:URL:) to grab raw data and convert it to a UTF8 string. This api wasn't designed for being used for networking, but rather read files from inside your app. It is not documented anywhere the error handling and doesn't give you any access to HTTP data (headers, statusCodes etc). You should look into using URLSession as it is the primary function for networking in iOS/mac apps. This will also allow you to extend your API to allow for PUT and POST operations, something NSString.(contentsOf:URL:)

The move to the URLSession is necessary and planned. more below.

  1. Your data types are not very swift like. You declare your internal properties as variables, rather than constants. There is nothing in the API to suggest the types should be modified, so why not make them immutable?
  2. More on the data types, the initialization of them is very un-swift like and leads to several issues. Data types like Account do not have any initializers, but rather assignments in an protocol (which I will get to). This is not very safe as these are declared as ** non-optional options**:

Not pretty, and certainly not documented;
I mostly wanted these to be expanded into things such as address-checking, currency-conversion, etc.
I hadn't gotten to playing with Decodable much yet, but that makes perfect sense as a solution!
Thank you!

  1. This last one is more of a style comment, but I have noticed you use Swift types in a non-typical way. Your primary API manager is an enum when it should be a class/struct. You also create protocols for each data type (Accountable, Delegable etc) when they essentially do the same thing for each data type. You could create one data protocol that would apply to all datatypes which can be decoded from son (like Decodable)

This I know is not typical.
Basically, I wanted to play with name-spaced-constants, avoiding instantiation if all I need is a value;
and also wanted to experiment with making it highly modular.

ARKKit definitely has room for work, and your feedback is a great help that I appreciate immensely!
It's very kind of you to spend time checking it out and suggesting these solid improvements!

This project started as a quick and dirty way to get a custom Ark app running for the IoT projects I'm hyper-focused on.
There were also no Swift wrappers to be seen,
so I figured it'd be a great way to learn the Ark Ecosystem and help kick-start interest from the Swift community at the same time.

That said, there is definitely much that will change before a final release;
and I will certainly make good use of your fantastic suggestions!

Thank you again, @Awalz

SwiftyArk is lookin great !
Keep up the amazing work!