Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use nil coalescing in ChartDataSet's entryCount (Fixes #631) #632

Merged
merged 1 commit into from
Dec 30, 2015

Conversation

aarondaub
Copy link

The force unwrapping in entryCount caused a crash when yVals was nil. Now entryCount returns 0.
#631

The force unwrapping in entryCount caused a crash when yVals was nil. Now entryCount returns 0.
@liuxuan30
Copy link
Member

I'm not sure if we should initialize yVals in init()?

    public override required init()
    {
        super.init()
    }

It does not initialize yVals, so it will crash. Not sure which way is better.

BTW, _yVals?.count is Int?, while entryCount is Int, though seems swift will help unwrap it. Not sure if we should make the return type consistent

@aarondaub
Copy link
Author

@liuxuan30 Having that initializer keep properties in the same state as other initializers would make sense.

I'm not sure I follow what you mean about making the return type consistent – making entryCount Int? requires changes throughout the rest of the codebase but having it fail safely (by using ??) only requires changing the getter itself.

@liuxuan30
Copy link
Member

@aarondaub, I am no expert of the details of swift, so what I want to ask is, _yVals?.count will return Int? instead of Int, while entryCount is declared as Int type. Let's say the yVals has 5 entries, so it will return Optional(5). I tried the code, and swift will help unwrap, and entryCount is assigned as 5.
I am just not sure, if this is the right way to do so, or we should follow the style in documentation:

if let count = _yVals?.count 
{
   ...
} else {
   ...
}

I am just being cautious about ? :)

@aarondaub
Copy link
Author

@liuxuan30 ah, let me clarify.

You're right that

yVals?.count

is of type Int? so it is not a valid return type for a function (in this case entryCount) that returns Int. In fact it wouldn't compile if the implementation was just:

public var entryCount: Int { return _yVals? }

To get around this, I used the nil coalescing operator ??. What ?? does is unwraps its left hand argument and return that if it is not nil, otherwise it returns its right hand argument.

So nil ?? 0 is 0, and Optional(1) ?? 0 is 1.

As far as style goes: I personally think the nil coalescing operator is fine, adding an explicit if-let/else would just be unnecessary bulk IMHO.

@danielgindi
Copy link
Collaborator

Seems fine to me :-) Thanks!

danielgindi added a commit that referenced this pull request Dec 30, 2015
Use nil coalescing in ChartDataSet's entryCount (Fixes #631)
@danielgindi danielgindi merged commit da03c94 into ChartsOrg:master Dec 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants