Swift: How do we feel about using fatalError in apps? #440

Closed
jakecraige opened this Issue Jul 27, 2016 · 4 comments

Comments

Projects
None yet
4 participants
@jakecraige
Contributor

jakecraige commented Jul 27, 2016

The usage of fatalError has come up a lot in code reviews and I think it leads to a lot of tension for people on both sides. We should discuss it and attempt to come up with a guideline for it's usage that we add to the guides.

Personally I like using it (maybe not like, but am happy to), because I use it to notify me of unexpected states as early as possible in the development cycle. It introduces a possible runtime crash, but I'm okay with that since that's a case I didn't plan for, it probably isn't going to work right anyways.

The alternative is to not use fatalError and introduce things like .Unknown enum cases or provide default values like something?.isSomeBoolean ?? false which leads to the app not crashing, but possible unexpected behavior.

A real world case where this comes up is using an enum for table view sections. The two sides might look something like this:

Using fatalError

enum Section: Int {
  case promos
  case products

  init(section: Int) {
    guard let value = Section(rawValue: section) else { fatalError("Unknown section \(section)") }
    self = value
  }
}

override func collectionView(collectionView: UICollectionView, cellForItemAtIndexPath indexPath: NSIndexPath) -> UICollectionViewCell {
  switch Section(section: indexPath.section) {
  case .promos: break // return a promo cell
  case .products: break // return product cell
  }
}

Not using fatalError

enum Section: Int {
  case promos
  case products
}

override func collectionView(collectionView: UICollectionView, cellForItemAtIndexPath indexPath: NSIndexPath) -> UICollectionViewCell {
  switch Section(rawValue: indexPath.section) {
  case .promos?: break // return a promo cell
  case .products?: break // return product cell
  default:
    // This was unexpected so what is the right behavior here? Return an empty cell?
    return UICollectionViewCell()
  }
}

The problem I see with the non-fatalError case it it puts us in a weird spot because that should never happen, but it might, so what do we do if it does? I'd argue there isn't really a correct answer other than crashing, but I'd love to here others thoughts.

@thoughtbot/ios

@jakecraige jakecraige added the Swift label Jul 27, 2016

@gfontenot

This comment has been minimized.

Show comment
Hide comment
@gfontenot

gfontenot Jul 27, 2016

Contributor

"should we use fatalError" is probably not a useful conversation because the answer is always going to be "it depends". A large part of that "it depends" answer is the developer's personal preference, and I think it would be weird to make a guideline about this forcing people to use a pattern that they aren't comfortable with.

I think this probably belongs not in the guides, and should instead be a conversation somewhere else (I don't know where) about when/why people have found this pattern useful. Maybe a blog post?

Contributor

gfontenot commented Jul 27, 2016

"should we use fatalError" is probably not a useful conversation because the answer is always going to be "it depends". A large part of that "it depends" answer is the developer's personal preference, and I think it would be weird to make a guideline about this forcing people to use a pattern that they aren't comfortable with.

I think this probably belongs not in the guides, and should instead be a conversation somewhere else (I don't know where) about when/why people have found this pattern useful. Maybe a blog post?

@jakecraige

This comment has been minimized.

Show comment
Hide comment
@jakecraige

jakecraige Jul 27, 2016

Contributor

I agree that the "it depends" is the correct answer to the title, but I think the spirit of this is more about where should we use it, and have guidelines around that. This whole repo is made up of rules that are personal preference, but they're ones we all came to an agreement on to avoid future issues with understanding and reviewing code. I think we should have some guidelines around this.

Guidelines are meant to be broken in certain contexts, and as I've seen it, not having any related to this cause issues in reviews and I'd like to try and come up with some sense of consensus.

Contributor

jakecraige commented Jul 27, 2016

I agree that the "it depends" is the correct answer to the title, but I think the spirit of this is more about where should we use it, and have guidelines around that. This whole repo is made up of rules that are personal preference, but they're ones we all came to an agreement on to avoid future issues with understanding and reviewing code. I think we should have some guidelines around this.

Guidelines are meant to be broken in certain contexts, and as I've seen it, not having any related to this cause issues in reviews and I'd like to try and come up with some sense of consensus.

@sharplet

This comment has been minimized.

Show comment
Hide comment
@sharplet

sharplet Jul 27, 2016

Contributor

Regarding the specific example of table sections, I personally feel that
crashing is the correct behaviour. Swift itself sets a precedent with the
behaviour of subscripting, which also crashes if your index is out of
bounds.

However, that case itself feels perhaps too specific to actually include a
guideline. Are there any examples where things have swung the other way? Or
has every case been polarising in code review?
On Wed, 27 Jul 2016 at 12:15 PM, Jake Craige notifications@github.com
wrote:

I agree that the "it depends" is the correct answer to the title, but I
think the spirit of this is more about where should we use it, and have
guidelines around that. This whole repo is made up of rules that are
personal preference, but they're ones we all came to an agreement on to
avoid future issues with understanding and reviewing code. I think we
should have some guidelines around this.

Guidelines are meant to be broken in certain contexts, and as I've seen
it, not having any related to this cause issues in reviews and I'd like to
try and come up with some sense of consensus.


You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
#440 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAJA5mSYAqZnmCLDWm2WjZdjgMsraZhKks5qZ6A2gaJpZM4JWb2a
.

Contributor

sharplet commented Jul 27, 2016

Regarding the specific example of table sections, I personally feel that
crashing is the correct behaviour. Swift itself sets a precedent with the
behaviour of subscripting, which also crashes if your index is out of
bounds.

However, that case itself feels perhaps too specific to actually include a
guideline. Are there any examples where things have swung the other way? Or
has every case been polarising in code review?
On Wed, 27 Jul 2016 at 12:15 PM, Jake Craige notifications@github.com
wrote:

I agree that the "it depends" is the correct answer to the title, but I
think the spirit of this is more about where should we use it, and have
guidelines around that. This whole repo is made up of rules that are
personal preference, but they're ones we all came to an agreement on to
avoid future issues with understanding and reviewing code. I think we
should have some guidelines around this.

Guidelines are meant to be broken in certain contexts, and as I've seen
it, not having any related to this cause issues in reviews and I'd like to
try and come up with some sense of consensus.


You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
#440 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAJA5mSYAqZnmCLDWm2WjZdjgMsraZhKks5qZ6A2gaJpZM4JWb2a
.

@jamescmartinez

This comment has been minimized.

Show comment
Hide comment
@jamescmartinez

jamescmartinez Feb 16, 2017

Member

Perceived results: use fatalError if necessary, but mostly "it depends"

Member

jamescmartinez commented Feb 16, 2017

Perceived results: use fatalError if necessary, but mostly "it depends"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment