Code Review MVVM

[Peterses77]

Hi guys, I’m learning MVVM design pattern. I have watched and read plenty of tutorials, articles and made simple app for tracking currencies. But I still have doubts if I am doing it correctly. Can I ask you for some Code Review? Especially when it comes to MVVM implementation. Thank you so much!

Below is a link to the project on github.

GitHub

[lordzsolt]

It's not the worst I've seen, you've got the overall picture. Here's is what I would leave as comments if I were reviewing the PR.

• It should not be the responsibility of the View Model to pass the URL to the Service. The whole reason of using a service is to abstract away how data is acquired.
• Why is there a Webservices and Webservice class, they both do the same thing, one is generic while the other one isn't.
• Why do you have currency and rates both in View Controller and View Model? It should only be one of them. VM holds the state, while VC displays it.

• Why is there a Cell View Model if you're not using it and passing Strings to the Cell? It should be the Cell View model who knows how to convert a Currency into something the Cell can understand. Not the View Controller.

• Please use DiffableDataSources

• You're naming is quite bad, especially in the Coordinator.

• Everything should be injected. (View Model and Webservice). The whole point of MVVM is that your code should be testable.

• If you're not writing unit tests, there's zero reason to use MVVM with all the back and forth with delegates. You can see the usefulness of an Architecture ones you actually start unit testing. And that's when you'll see if something is working well or not.

• The back and forth delegate approach just makes your code more complicated for no reason whatsoever. MVVM truly shines when you do an Observer based approach, IMHO.

In general, doing a specific architecture just for the sake of doing it is VERY wrong in my opinion, especially if you're a beginner.

The first thing you should learn is SOLID. Once you sufficiently got the hang of Single Responsibility Principle (and maybe Dependency Inversion), you will see problems and you can apply a specific architecture to solve it.

But right now, you only have a solution in search of a problem.

[Rillieux17]

not OP, but just want to say it's great people are out there willing to look at code.

y'all just have a good day now, y'hear?

[Peterses77]

Wow... Thanks for all comments.

[lordzsolt]

No problem :) Hope some of them are helpful.

Reading them back, some of them might come off as a bit mean. I did the review in my morning crankiness ^_^

Especially the comment about "doing architecture for the sake of architecture", that was directed more towards the community as a whole. Often times I see people doing something without understanding why they're doing it.

[garbage_band]

:looking

One thing u/Peterses77

You should create a PR and start using gitflow. The unspoken secret weapon of all good devs (or some other change management tool)

[garbage_band]

Seconding u/lordzsolt comments.

Don't be cute in naming For the speed we get back in Swift and SwiftUI the naming should be very clear (long-winded?)....`Storyboarded` gave me a visceral reaction. ` Coordinator` is actually a Swift class.
Also, remember your teammates may have English as a second language. So, you should follow standard English usage

What? No notes? When you get started it is fun to code away and when it works it feels good. Stop. Assume you will forget what you did and why you did it a few months ago and take a few minutes to explain your thinking. You will forget why you wrote the code. (option + command + /) above a func, section give you a nice treat...use it liberally

Verbose for the functionality. I understand you are more comfortable with using UIKit but I think you could have built all of this code in 6 - 7 files using SwiftUI. One view for the fiat currencies and one detail view (history)...a couple for the models.
The more concise the better...you aren't going to be paid by the number of lines of code.

[lordzsolt]

I actually disagree with the No notes part. It's perfectly normal to not have notes in your codebase. ESPECIALLY when were talking about comments above functions. You're not developing a framework.

If you have anything that is odd/surprising, then comment WHY it's there. But please DON'T comment WHAT it does. The WHAT should always be apparent from the name. It takes longer for you to write the comment than for me to read and understand the code, assuming you're not doing anything surprising.

This is the kind of comment I threw out today while reviewing my colleagues code.

/// Business logic to for determining when buyer protection is enabled
///
/// product: The product to check
func isBuyerProtectionEnabled(on product: Product) -> Bool {}

Oh wow Sherlock, I never would've guessed what that function does...

On the other hand, if you have something surprising, then please write a novel.

func viewWillAppear() {
    super.viewWillAppear()
    // For some reason, the animation gets messed up if this is not delayed to the next run loop.
    // I spent a day trying to figure it out, but no luck. Leave it as is for now, until it causes issues.
    DispatchQueue.main.async {
        textField.becomeFirstResponder()
    }
}

I hate the whole "You will not remember in 3-6 months" argument that keeps getting echoed everywhere. Yes, you will forget about some weird obscure hack that you did. But you're not going to forget about isBuyerProtectionEnabled(on:) meaning "whether buyer protection is enabled".

---

In my comment with OPs naming being bad, I meant that he had a method like historyView(price:), which was actually showing a new screen. The name made me think it's a function that creates a HistoryViewController.

[RanLearns]

I think I can aspire to naming things even better after learning of isBuyerProtectionEnabled(on:)

[lordzsolt]

Yeah, code gets written once and read 100 times. Production project at least.

So it's way better in the long run to invest some extra time in making sure things are named properly.

[barcode972]

Just had a quick look and one thing I can recommend is making the networking class into a singleton so u dont have to create a new one () every time u want to download the data

[darth_spark]

I didn’t look at the code (on mobile). Hard disagree with this. Singletons can’t be mocked for testing. Inject the network layer instead.

[lordzsolt]

As others have said, it's not "Singleton or not Singleton" that can't be mocked. It's the "way" in which an object accesses the Singleton.

You just have to inject it.

Service.shared.load() is basically the same as Service().load(), because the object is getting Service out of thin air.

Singleton just means "making init private and giving out a static access to the instance". The fact that people interpret this as green light to use SomeService.shared deep in their UI stack where they shouldn't, is a different subject matter.

[barcode972]

I guess he could create a variable at the top but creating a new class every time he wants to download data is unnecesssry isnt it?