Fitbit.NET icon indicating copy to clipboard operation
Fitbit.NET copied to clipboard

Tidy and refactor

Open WestDiscGolf opened this issue 7 years ago • 3 comments

WestDiscGolf avatar Aug 03 '17 20:08 WestDiscGolf

Done!

WestDiscGolf avatar Aug 04 '17 08:08 WestDiscGolf

hey @WestDiscGolf so... I'm a little hesitant to merge this kind of sweeping change in. My read is this is mostly style, using shorthands and some of the new C# 6 syntactical sugar. My concern is that if lumped in to all the work @AlexGhiondea has done we add more migration burden. We do have a lot of folks on the existing library, and then we do want to them to move along to 3.0 where there is a lot of code delta. Should someone hit a snag and be confused what has changed, I'd want the code delta to be as manageable to get their head around as possible.

My personal motivations is that we don't use these syntax / shorthand formats in our own codebase internally (didn't exist when we started), and selfishly would prefer to keep it consistent, unless a strong case is made otherwise.

I do see a few type checks / safety checks that might be worth it, but seeing as how this was all done as a single commit, I can't really easily take those -- it's kind of all or nothing.

aarondcoleman avatar Aug 04 '17 18:08 aarondcoleman

Hi @aarondcoleman, Thanks for the feedback. I am however a little confused by what you mean by "migration burden"? I've not done anything which changes the API surface area. I have submitted the question about changing that to the team to discuss the number of constructors which would impact the API surface.

The usage of newer C# syntax internally to the library I think fits with the move forward to being able to support dotnet core. The code has to work with the newer compilers due to the dotnet core support and functionally it is the same.

I understand that some of it can look a bit odd at times, and yes I'm still learning some of the newer syntax myself (the expression body of properties is new to me!) so it can look odd. I'm happy to change the property expression body for the AccessToken back to a get/return (see updated PR).

The rest of the changes I've submitted are pretty light touches which help with future refactoring (eg. the nameof() usages), removing redundant code (extra this. references) with some tidy up of additional braces around conditions to make the code generally more explicit and readable as well as some small tweaks.

Happy to discuss further.

WestDiscGolf avatar Aug 05 '17 09:08 WestDiscGolf