Variable shadowing can lead to harder-to-read code. about swift-style-guide HOT 22 CLOSED

For me, seeing ! during code review is the red flag. That's when I start asking "Why didn't we optionally bind this thing?"

I worry about "unwrapped" being too long of a prefix for the books, it's super difficult to keep line wrapping to a minimum, and that is a lot of horizontal space used up. Line wrapping of code in the books is arguably worse as far as readability goes IMO. Of course this example would not suffer from line wrapping, but throw in a Cocoa API call and it would be a different story.

My stance on shadowing will probably vary case by case and day by day though :]

from swift-style-guide.

Without compiling that code, how do I know that the name variable being set is the unwrapped one rather than the passed-in optional?

Because that's how Swift works. If you're inside an if let scope, it uses the unwrapped version. There is no ambiguity here.

from swift-style-guide.

@designatednerd as someone new to swift, I think you were absolutely correct in this. It is confusing for beginners. I was really confused on how the scoping worked with this.

from swift-style-guide.

@hollance: There's no ambiguity to someone who knows Swift well, and there's no ambiguity when it's compiled. To someone who knows it less well (for instance: me) and is trying to read the code without checking it out and compiling it, variable shadowing makes the flow significantly less obvious.

@cwagdev Maybe prefix with just a u instead of unwrapped? At work our Android code standards require any variable passed in as an argument to have a as the prefix, and this has been a pretty solid compromise between clarity and variable length.

from swift-style-guide.

I guess this is where semantic code highlighting comes in handy. ;-)

By the way, did you know that if var also works for unwrapping?

  if var name = name {
    name += ", Esq."
    person.name = name
  }

from swift-style-guide.

@hollance It certainly helps if the optional you're unwrapping is a local property, but doesn't do much if it's a passed-in argument.

As for if var: https://www.youtube.com/watch?v=Cd7tEsAuspA - thanks!

from swift-style-guide.

The thing about semantic highlighting vs regular syntax highlighting is that it gives each variable its own color. So you'd be able to see at a glance that the unwrapped name is different from the parameter name.

There is an Xcode plug-in for this, but I don't know if it works with Swift. https://github.com/kolinkrewinkel/Polychromatic

from swift-style-guide.

I strongly oppose introducing any new variable or constant prefixes. Hungarian Notation has almost died out, there's no good reason to revive it!

I agree that we should rely on development tools to give hints regarding scope and context.

from swift-style-guide.

I certainly get the argument that within a compiled environment, this isn't necessary - that's why this rule was adopted in the first place.

The problem is that our code is often read outside a compiled environment, whether on the website or in books. This process becomes significantly less obvious without the aid of the compiler barfing if you try to use the wrong non-optional vs. optional var or let.

@ColinEberhardt and @hollance: Do you have any other suggestions on how we can give better context to these things in a read-only environment without resorting to a variable prefix? Or should we just say, "Screw it, type it into a playground if you don't get it."?

from swift-style-guide.

+1 for using the same name. In general I would oppose variable shadowing but in this case I believe it makes sense. It makes things simpler since creative names or prefixes don't have to be used. Also the shadowed variable really refers to the same variable, just the unwrapped version of it, i.e. its not really shadowing any other variable, its shadowing itself.

from swift-style-guide.

I like @jawwad's explanation that the shadowing is really "shadowing itself", so using the same name makes sense.

You could also argue the other side: if you had foo and unwrappedFoo and then you called some method on unwrappedFoo, it's possible people could be confused that these are two different things and that foo wasn't touched. They'll understand it eventually when they understand optionals, but that's the case for the shadowing way too.

Either way, we'll need to rely on people "getting it" eventually and if that's the case – I'd rather optimize for the eventuality of people understanding optionals.

Let's keep the discussion going though! Maybe we can beat the post count for the Objective-C style guide discussion on ivars vs properties!

from swift-style-guide.

A potential storm brewing from the God himself, https://devforums.apple.com/message/1127586#1127586

from swift-style-guide.

I think that post is more around whether variable shadowing should throw a warning - Laettener says no and that there's no specific guidance on whether you should shadow stylistically or not.

Since I started this fire, allow me to give an update: I've been doing more work in Swift on a prototype, and I still feel like I have a significantly harder time reading code without the shadows.

What it comes down to for me is this old chestnut:

Programs must be written for people to read, 
and only incidentally for machines to execute.

Yes, the compiler will tell you when you're screwing up when you're actually writing the code. But reading through others' (or even my own) code becomes significantly harder for me with variable shadowing. I have a much harder time intuiting where someone is accessing the unwrapped value vs. the wrapped value when I'm reading shadowed code.

I've also been working with newer programmers who have a lot of trouble understanding scoping (which I certainly did when I was starting out), and I only see this problem being made a thousand times worse by variable shadowing. How can a newer programmer tell that they're actually accessing an unwrapped constant in a particular scope rather than the original optional variable?

Having a clear delineation between the thing you are unwrapping and the thing that is unwrapped that is easily readable will make it far, far easier for noobs to tell which of the two they're accessing.

And I know it will make @ColinEberhardt very sad to hear, but I've adopted the u prefix on my personal code and I've found it helps tremendously.

from swift-style-guide.

I don't think Laettener gives advice one way or another here... he just says there won't be a warning.

It sounds like he's saying "do it if really you want to" to me... :]

from swift-style-guide.

Agreed, I was primarily being sarcastic and the thread reminded me of this 😄

I am still in the shadow camp but can see @designatednerd's point.

from swift-style-guide.

@designatednerd Shadowing is just neater. It is a readily deducible fact that inside the if let scope, you are referring to the unwrapped version. Explicit unwrapping is so common that anyone worth a few days of experience with Swift likely already knows this.

from swift-style-guide.

@annehiro I disagree with the assessment "anyone worth a few days of experience with Swift likely already knows this." Anyone with a few days of experience with swift and lots of other programming experience will likely get it, because they understand scoping. I went into detail above on why I think this makes it harder for n00bs who don't really get scoping to understand.

However, @gregheo, I don't know if this issue is worth leaving open anymore. While I still feel exactly as the title of this issue says (and am still using non-shadowed variables in my own code), making this change at a site level would require an insane amount of work updating the site and books at this point.

This set of code standards is used as a reference for a lot of workplaces still developing their own Swift code standards (including mine), but ultimately, these are for our site and books. And I think the horse has left the barn and run across half the country at this point on changing tutorials and books. 🏇

from swift-style-guide.

FWIW I've written quite a bit of Swift since this came about and I'd say that I shadow 90% of the time, I skip shadowing is when the variable is extremely verbose.

from swift-style-guide.

This is a tough one as there are good points on both sides of the argument. Personally, I don't like the term "shadowing" as it really is a static type transformation from optional to non-optional. I have never had trouble reading the code as it is easy to tell if something is optional or non-optional at the call site. Thinking of it as a type transformation also dovetails well with using guard to satisfy preconditions early and exit if not met. I hope that someday in the future we get syntactic sugar that looks something like this:

func compute(a: Int?, b: Int?) -> {
   unwrap a, b else { return }
   // compute with a and b Ints
}

Unwrap would also work with the mythical Result type.

There is often times when it makes sense not to use the same name but an abbreviated form. For example, coming from the Siesta framework (which I consider really great code modulo its funny whitespace):

guard let cache = config.persistentCache else { return }

Other times it just shadows.

I think @cwagdev is getting at. A few other projects I checked used a mix of shadowing and non-shadowing including Almofire, and Apple's open source implementation of Foundation. None of these projects, however, used Hungarian notation.

Based on these findings and my own personal gut I am going to rule that for now no standard should be made about to shadow or not to shadow. Closing this issue but we can revisit it again if the landscape changes.

from swift-style-guide.

@rayfix 👍 I'm also for closing this issue.

However, regarding

I am going to rule that for now no standard should be made about to shadow or not to shadow.

Actually, the style guide already has a ruling on this:

For optional binding, shadow the original name when appropriate rather than using names like unwrappedView or actualLabel.

When this issue was opened, there wasn't a lot of our code or tutorials that used "shadowing."

Now, however, is a different story:

• We have used it in many written and video tutorials.
• We used and explained it at RWDevCon 2015 and 2016.
• All in all, we've essentially backed the idea of "shadowing" the original name.

At this point, I think we're committed to it... Personally, I've grown to like it too...

Like fine 🍷, it grows on you? 😉

from swift-style-guide.

To me that says, "don't use Hungarian notation" which I am totally on board with. The "when appropriate" gives a little wiggle room for things like:

guard let json = content as? NSDictionary else { throw InvalidJSON }

from swift-style-guide.

FWIW, I stopped using the u and just wound up using different variable names, but I do realize I've lost this battle. 😄

from swift-style-guide.