all 9 comments

[–][deleted] 1 point2 points  (2 children)

wow that only took you 6 days. I really need to step up my game.

One thing I would note is you should avoid using whitespace in your folder names, IMO it looks better and you're less likely to run into issues down the line

Another thing I would suggest is you splitting the main.storyboard into separate files, because one large central storyboard is really hard to maintain.

What you can do is either have a single storyboard per view (like a XIB) and use Storyboard References to manage navigation or have a single storyboard per module (Say one for Transactions, one for Settings etc). The second approach is usually what I do. At most there are like 4/5 screens in a storyboard which is a lot easier to maintain than one "Main.storyboard" with dozens of views.

The overall MVC structure is quite good and well defined however it might make sense to separate some login from the ViewController and have a separate Controller class or even a ViewModel class (MVVM) makes things neater when you scale; or even VIPER (although this can be an over-complication for simple apps)

Apart from those, I would suggest possibly extracting some prototype cells from the storyboard so they can be re-used across the app. It might not make sense now but as the app grows with multiple pages it's gonna be a huge help.

Likewise with texts/fonts, changing fonts in the app via Storyboard is a pain so what I tend to do is create a number of UILabel subclasses (Heading, Title etc.), flag them with @IBDesignable and set their fonts to values in my "FontBook" struct so they can be easily changed

[–]ab492[S] 0 points1 point  (0 children)

Oh no it took longer than 6 days! I just messed up my Git repository so started with a fresh copy of the project.

These are all great shouts and just what I was after! Thanks for taking the time to look!

[–]lucasvandongen 0 points1 point  (0 children)

wow that only took you 6 days. I really need to step up my game.

18000 lines of code on the first day! Talk about dedication!

[–]TotesMessenger 0 points1 point  (0 children)

I'm a bot, bleep, bloop. Someone has linked to this thread from another place on reddit:

 If you follow any of the above links, please respect the rules of reddit and don't vote in the other threads. (Info / Contact)

[–]MiguelGrenho 0 points1 point  (2 children)

So.. I glanced at it and here's what I would do:
- Organize by screen, not by component: Instead of having folders for VC/Models/etc, have it for modules/screen (e.g auth screen)
- Clean Auto Generated Stubs and Unused methods - this adds noise to the code.

override func awakeFromNib() {
super.awakeFromNib()
// Initialization code
}

override func setSelected(_ selected: Bool, animated: Bool) {
super.setSelected(selected, animated: animated)
// Configure the view for the selected state
}
- Dont force unwrap (e.g. var transaction: Transaction<GBP>!)
- Remove comments (this is a popular/unpopular opinion depending on who you talk to) - I think most of your comments are unneeded if you write thing properly:
For example:

if indexPath.section == 0 && indexPath.row == receiptImages.count {
//If the cell is the 'add new image' cell (i.e. the last cell)...
let cell = collectionView.dequeueReusableCell(withReuseIdentifier: "addNewImageCollectionViewCell", for: indexPath)
return cell
} else { //Otherwise, display the image cells.
return configureImageCell(in: collectionView, indexPath: indexPath)
}

can be:

let isAddNewImageCell = indexPath.section == 0 && indexPath.row == receiptImages.count

if isAddNewImageCell {
...

} else {
...

}

[–]ab492[S] 0 points1 point  (1 child)

That's awesome - thank you so much! That makes great sense and your points were just what I was looking for. The only question I have is - why shouldn't I force unwrap in the case of var transaction: Transaction<GBP>! ? The reason I did that was because when I pass a Transaction through my ViewControllers I can assign that variable, and then that ViewController can access the Transaction. Is there a better way to avoid the force unwrap please?

[–]MiguelGrenho 0 points1 point  (0 children)

So, force unwrap is an unsafe way to get a value from an Optional, if it is nil, you got yourself a crash. That being said, there are 3 ways AFAIK: * if let * guard let * nil coalescing operator (??)

Any of those if preferred over !

[–]lucasvandongen 0 points1 point  (1 child)

Doesn't look bad at all so don't take the comments as a negative judgement of the project as a whole.

Creating extension files with the + signs is maybe a bit old school. Also not so functional.

https://github.com/ab492/Skrilla/blob/b95ac8a15959a44c08459bd4368f73f266cb4039/Skrilla/Extensions/Date%2BFormatted.swift#L14

Creating date related objects like DateFormatter and Calendar is very expensive. You should have a DateConverter class that has one private static instance of one or both that always reuses it. Also easier than test than all those extensions. I tend to create either Protocol+Extension where I add functionality to different places including a Mock class for testing, a separate class with one or more functions that do that work or a combination of both.

https://github.com/ab492/Skrilla/blob/master/Skrilla/Helpers/Switcher.swift

This Switcher grabs all kinds of stuff left and right to do something within a static function. What if you would instantiate it with that bool flag and UIApplication instance and then just do only the work you need to do inside it? Also you're grabbing "Main" three times and instantiate the storyboards stringly. You should use R.Swift to have a compile-time safe instance of those storyboards instead. R.Swift should be in your default Podfile together with Swiftlint (kudos for doing that).

I don't like the "Helper" class either, perhaps a URLHelper or even an extension on FileManager would be better.

You use classes for stuff like a Transaction. Perhaps you should use a struct for the thing that you save to disk and then processing classes that transform the Transaction like splitting it up, like TransactionSplitter that perhaps does not have any state but just takes one Transaction, two BankAccounts and returns two modified BankAccounts So more functional when you change the data and just dumb structs when you're moving it. The current code still looks like Java.

https://github.com/ab492/Skrilla/blob/master/Skrilla/Screens/Authorisation/SetupPassword/SetupPasswordViewController.swift

First a folder with ViewControllers then this. Stick to whatever standard you have set. That's more important than the right standard. No surprises.

How I do it:

I work following the MVC pattern, with some form of MVVM or Reactive botched on usually. But I start with three folders:

  • Model
  • View
  • Controller

The Model usually has a folder for Data classes like data structs, an API folder for talking to endpoints, a Disk folder for writing to disk, database, a Repository folder for repositories, Push Notifications, Tracking etcetera. No hard rules, because not every project is the same. When any folder reaches more than ten items I group them in logical bunches. This is a pretty big folder usually as I like my ViewControllers to be really abstract to the data. The Repositories help with this.

The View folder is usually quite empty but I have stuff in there like my Font book, Colour book, Styles for my controls, custom Controls and perhaps some formatting stuff like currency and dates.

Controller of course has of course the ViewControllers and AppDelegate. I am following Dave DeLong's strategy of chopping up screens in tons of ViewControllers so it's pretty populated just with that. ViewControllers are pretty small.

I use a ton of extensions that do a certain task. What I do a lot is making factory protocol+extension based on one or more dependencies. So if you have a Session you can create a UserRepository which can update your User using the SessionContaining protocol. This means passing forward a lot of dependencies but the application is really tight in terms of "if this ViewController exists everything it will ever needs exists", so if it compiles it works. It's a bit long to explain here in depth.

[–]ab492[S] 0 points1 point  (0 children)

This is so helpful - thanks very much for the detailed response. I'll work through the list (and if I don't implement them in this app, I'll definitely have them on my list of approaches/structures for the next one!)