General code refactoring #19
Conversation
|
Hey, thanks for all the PR's! I'll probably take a look this weekend and try to get these merged and push out a new release on NuGet. |
| if (payloadData.TryGetValue("exp", out value) && value != null) | ||
| { | ||
| // safely unpack a boxed int | ||
| if (!(value is int)) |
There was a problem hiding this comment.
Hrm. I suspect this isn't exactly identical. What if it's a long or a uint? Should we support float, double, etc.? There are probably a number of cases that would have been allowed before but not now, so I'm nervous about changing this.
There was a problem hiding this comment.
My tests show that it won't work either way, e.g.:
int f = (int)(object)5f;
int d = (int)(object)5d;
int m = (int)(object)5m;If you box long, uint, etc. and then try to unbox as int it will throw IvalidCastException.
So only int will work. I believe the approach I propose would be more efficient and readable.
Another way could be this:
int? i = o as int?;
if (i.HasValue)
// use i.Valuebut it's less efficient I was told.
|
I have concerns about one change, but otherwise looks fine. Also, there are merge conflicts now since I waited so long to review. Sorry! |
|
It seems I need to configure the upstream and then I'll be able to sync the fork. |
|
I hate resolving conflicts. And don't know how to do it well. So I simply rolled everything back, updated from the upstream and applied them once again. Please check take a look once again. Also I split the changes to Verify() into few commits:
|
|
Hey, any chance to review and merge, please? |
| } | ||
|
|
||
| var secondsSinceEpoch = Math.Round((DateTime.UtcNow - UnixEpoch).TotalSeconds); | ||
| if (secondsSinceEpoch >= (int)value) |
There was a problem hiding this comment.
I think this code may have been updated since you originally started this PR so that now it does Convert.ToInt32() which is more permissive than "is int" and "(int)value". I think Convert.ToInt32() is probably better, since it'll handle longs and whatnot.
|
I'll work on this in next few days. |
|
Please check it out. I moved the code extracting and validating exp into separate branch, see #28. |
|
Looks good to me. Thanks! |
|
Awesome, thanks! What do you think about the next one? |
No description provided.