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

Fixed decimal UTC time bug #99

Closed
wants to merge 4 commits into from

Conversation

SauravDharwadkar
Copy link
Contributor

fixed #90
floor down value because chart uses hours only

fixed vn7n24fzkq#90 
floor down value because chart uses hours only
@vn7n24fzkq
Copy link
Owner

vn7n24fzkq commented Aug 6, 2022

Thanks for your work!!

I would like to merge this PR, but maybe we can use Math.round here?

@vn7n24fzkq
Copy link
Owner

Never mind, I am just realize that we use Math.floor can avoid to turn 23.6 to 24, if we use Math.round will need to write some code to deal with some special cases,.

I think we can take this PR.

I will do some more review later, to check there are no other problems.

@SauravDharwadkar
Copy link
Contributor Author

SauravDharwadkar commented Aug 6, 2022

I thought that , utc time 0 , 0.3 , 0.45 in min where 0.6 = 1 https://en.m.wikipedia.org/wiki/List_of_UTC_offsets

So for avoid dealing with 0.5 nunber and 0.3 hour / 30 min i used floor.
We can add logic to round robin between hour or ceil if 0.45 . Avoided it becz it'll create un necessary complecation.

@SauravDharwadkar
Copy link
Contributor Author

Or we can simply check value after point , 0 / 3 / 45 and update utc accordingly and use floor for default to avoid incorrect utc format

@vn7n24fzkq
Copy link
Owner

Or we can simply check value after point , 0 / 3 / 45 and update utc accordingly and use floor for default to avoid incorrect utc format

I prefer this way, but throw an exception when the UTC number is invalid.
We can check if the value is invalid and throw an exception here to alert the caller the UTC format was incorrect.

And like you said, default to using floor.

@SauravDharwadkar
Copy link
Contributor Author

SauravDharwadkar commented Aug 6, 2022

When i looked chart i realise we need to use number.toFixed(2) // '5.30' this or treat utc as string bcz in chart time should display as e.g. +5.30 or -5.30
But now its just shows 5.3 .
Both are same but first glance it look like something is wrong with utc time .
Screenshot_20220806-201552_Chrome

@vn7n24fzkq
Copy link
Owner

vn7n24fzkq commented Aug 6, 2022

You are right, would you like to fix it in this PR?

@vn7n24fzkq
Copy link
Owner

Or we can create another issue for this if that need to modify many codes.

@SauravDharwadkar
Copy link
Contributor Author

Seting up utc time on chart as well handling offset simple task , but I'm not sure how will you display warning or handle the wrong utc and dont want to lean whole code. I'll handle chart display and utc offset u do something about warning .

@vn7n24fzkq
Copy link
Owner

Sounds good to me, thanks!

@vn7n24fzkq
Copy link
Owner

By the way, about the warning, I will just throw an exception like this.

if(!IsValidUTCTime(utcTime){
    throw Exception ("The UTC format is invalid, it should be ......")
}

@SauravDharwadkar
Copy link
Contributor Author

SauravDharwadkar commented Aug 7, 2022

const adjustOffset = function (offset: number, RoundRobin: {offset: number}): number {
    if (offset % 1 == 0) {
        return offset;
        // offset % 1 should be 0.3 or 0.7 but its js and it gives 0.29999 or -0.299999 thats why this frankenstein
    } else if ((offset % 1 > 0.29 && offset % 1 < 0.31) || (offset % 1 < -0.29 && offset % 1 > -0.31)) {
        // toggle up and down between hour
        RoundRobin.offset = (RoundRobin.offset + 1) % 2;
        return RoundRobin.offset === 0 ? Math.floor(offset) : Math.ceil(offset);
    } else if ((offset % 1 > 0.44 && offset % 1 < 0.46) || (offset % 1 < -0.44 && offset % 1 > -0.45)) {
        // distrubute 1 : 3 ratio for 0.45 utc time
        RoundRobin.offset = (RoundRobin.offset + 1) % 4;
        return RoundRobin.offset % 4 == 0 ? Math.floor(offset) : Math.ceil(offset);
    } else {
        // flood down , if utc is given right it will never be executed
        return Math.floor(offset);
    }
};

.
.
 let RoundRobin = {
        offset: 0
    };
.
.
 chartData[24 + adjustOffset(afterOffset, RoundRobin)] += 1;

or use simply this for 0.3 and 0.45

Math.ceil(afterOffset);

@SauravDharwadkar
Copy link
Contributor Author

By the way, about the warning, I will just throw an exception like this.

if(!IsValidUTCTime(utcTime){
    throw Exception ("The UTC format is invalid, it should be ......")
}

in api IDK how it will display

@vn7n24fzkq
Copy link
Owner

const adjustOffset = function (offset: number, RoundRobin: {offset: number}): number {
    if (offset % 1 == 0) {
        return offset;
        // offset % 1 should be 0.3 or 0.7 but its js and it gives 0.29999 or -0.299999 thats why this frankenstein
    } else if ((offset % 1 > 0.29 && offset % 1 < 0.31) || (offset % 1 < -0.29 && offset % 1 > -0.31)) {
        // toggle up and down between hour
        RoundRobin.offset = (RoundRobin.offset + 1) % 2;
        return RoundRobin.offset === 0 ? Math.floor(offset) : Math.ceil(offset);
    } else if ((offset % 1 > 0.44 && offset % 1 < 0.46) || (offset % 1 < -0.44 && offset % 1 > -0.45)) {
        // distrubute 1 : 3 ratio for 0.45 utc time
        RoundRobin.offset = (RoundRobin.offset + 1) % 4;
        return RoundRobin.offset % 4 == 0 ? Math.floor(offset) : Math.ceil(offset);
    } else {
        // flood down , if utc is given right it will never be executed
        return Math.floor(offset);
    }
};

.
.
 let RoundRobin = {
        offset: 0
    };
.
.
 chartData[24 + adjustOffset(afterOffset, RoundRobin)] += 1;

I think we can take this code, and after that, we don't need to throw exceptions, tho.

@vn7n24fzkq
Copy link
Owner

vn7n24fzkq commented Aug 7, 2022

By the way, about the warning, I will just throw an exception like this.

if(!IsValidUTCTime(utcTime){
    throw Exception ("The UTC format is invalid, it should be ......")
}

in api IDK how it will display

We don't care about the error display here, just throw an exception.

And Math.ceil(afterOffset); is accepted too, if we use this way, I will take the UTC validation part.

@vn7n24fzkq
Copy link
Owner

vn7n24fzkq commented Aug 10, 2022

@SauravDharwadkar Thanks for your hard work!

Are you still working on this?
Please let me know if there is any problem.

@SauravDharwadkar
Copy link
Contributor Author

was busy .
i didn't test negative value bcz process.arg not work with negative value

Copy link
Owner

@vn7n24fzkq vn7n24fzkq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!
I left some comments here, take a look, please.

@@ -39,10 +57,14 @@ const getProductiveTimeData = async function (username: string, utcOffset: numbe
// process productiveTime
const chartData = new Array(24);
chartData.fill(0);
// eslint-disable-next-line prefer-const
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If possible, I think we should avoid disabling the eslint here. Could we do that? 🤔

@@ -39,10 +57,14 @@ const getProductiveTimeData = async function (username: string, utcOffset: numbe
// process productiveTime
const chartData = new Array(24);
chartData.fill(0);
// eslint-disable-next-line prefer-const
let RoundRobin = {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use camel case, the first letter should be lowercase.

for (const time of productiveTime.productiveDate) {
const hour = new Date(time).getUTCHours(); // We use UTC+0 here
const afterOffset = Number(hour) + Number(utcOffset); // Add offset to hour
// covert afterOffset to 0-23
const afterOffset = adjustOffset(Number(hour) + Number(utcOffset), RoundRobin); // Add offset to hour
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vn7n24fzkq
Copy link
Owner

Hi, @SauravDharwadkar.
Are you still working on this?
Since this PR is almost done, if you don't mind I can take over the rest part and keep your commit in the commit history.

@SauravDharwadkar
Copy link
Contributor Author

You can take over , sorry for late reply busy last couple of months

@vn7n24fzkq
Copy link
Owner

Solved in #123

@vn7n24fzkq vn7n24fzkq closed this Jan 7, 2023
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.

[Question] How to display decimal UTC offset?
2 participants