Skip to content
New issue

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

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

Already on GitHub? # to your account

New TIME functions #2

Closed
wants to merge 2 commits into from
Closed

Conversation

Serhioromano
Copy link
Contributor

I ma planning to make some modifications to core functions. But for that I need helper functions. This PR I only send you those additional functions without changing your core functions. Then I'll send new PR for each core function modification. I plan significantly simplify TIME related functions.

SO if you accept this PR, I'll take it as "Yes continue please!"

@andyruckstuhl
Copy link
Member

andyruckstuhl commented Aug 13, 2019

Thank you for your PR. I won't merge it due to the following reasons:

  • most of the functionality is already implemented; the utility unit would be polluted with more code without added benefit
  • making the code faster by using internal DATE_TO_STRING isn't productive, the internal date calculation doesn't differ from the already implemented one and creates an overhead (I've tested that some days ago on our Siemens Simotion device)
  • you've used a function called DATE_TO_DWORD which isn't declared or present in standard library
  • parameters like WEEK_OF_YEAR's fd are unclear (better would be an ENUM with named options)
  • the time utility will only support ISO-8061, therefore a week starting with sunday brokes that contract

@Serhioromano
Copy link
Contributor Author

Serhioromano commented Aug 13, 2019

I would not agree

most of the functionality is already implemented; the utility unit would be polluted with more code without added benefit

Smaller functions always have benefit. They can be used when you do not need complex sets of data and only need one parameter. Yet it simplify other functions hugely. You could see that I refactored your most complex function to just this.

FUNCTION DATE_TO_PARTS : DATE_PARTS
	VAR_INPUT
		in: DATE;
		fd: BOOL; (* TRUE - first day Monday, FALSE - first day Sunday *)
	END_VAR
	
	out.year  := DATE_TO_YEAR(in);
	out.month := DATE_TO_MONTH(in);
	out.day   := DATE_TO_DAY(in);
	
	out.year_of_century := UINT_TO_USINT(out.year MOD 100);
	out.day_of_year     := DAY_OF_YEAR(out.year, out.day);
	out.day_of_week     := DAY_OF_WEEK(in, fd);
	out.week_of_year    := WEEK_OF_YEAR(in, fd);
	
	out.iso8601 := MID(DATE_TO_STRING(in), 3, 10);
	
	DATE_TO_PARTS := out;
END_FUNCTION

How is this not beneficial?

making the code faster by using internal DATE_TO_STRING isn't productive, the internal date calculation doesn't differ from the already implemented one and creates an overhead (I've tested that some days ago on our Siemens Simotion device)

Working with dates as a string is always more preferable way. This way there are not mathematical calculations. We only change representation of bytes in memory. No manipulation with that data. If you think it is not faster then input variable may be already date string, so you can convert it once and then use it though program. At the end it will be much faster anyway.

you've used a function called DATE_TO_DWORD which isn't declared or present in standard library

I can change it to date_to_duint. But I though it is standard conversion function.

parameters like WEEK_OF_YEAR's fd are unclear (better would be an ENUM with named options)
the time utility will only support ISO-8061, therefore a week starting with sunday brokes that contract

That I can also add as enum and anyway if you works with week you have to give option to user to set first day of week. That does not affect anything else except day of week and week of year. That is it. the rest does not change. But if you want I can reset it to Monday first week day without ability to change

@andyruckstuhl
Copy link
Member

andyruckstuhl commented Aug 13, 2019

Smaller functions always have benefit. They can be used when you do not need complex sets of data and only need one parameter. Yet it simplify other functions hugely. You could see that I refactored your most complex function to just this.
How is this not beneficial?

That would be true if the parameters are independent - in this case, they are not (the same principle as in math utility's statistic functions).

Working with dates as a string is always more preferable way. This way there are not mathematical calculations. We only change representation of bytes in memory. No manipulation with that data.

I disagree. Date is internally stored as days since 1992-01-01. When you invoke the DATE_TO_STRING function, all mathematical calculations has to be done exactly as in my function (it's not possible otherwise with the exception of precalculated values).

If you think it is not faster then input variable may be already date string, so you can convert it once and then use it though program. At the end it will be much faster anyway.

I disagree as well: Discussions about speed are futile. Just implement it, run it a million times, measure required duration and compare the results. Maybe there are some optimizations done by the compiler or runtime in your environment which speeds your code up. In my environment, your code is significant slower than mine.

you've used a function called DATE_TO_DWORD which isn't declared or present in standard library
I can change it to date_to_duint. But I though it is standard conversion function.

In fact, in my environment (Siemens) there are no DATE_TO_xxx functions at all.

Finally, I won't accept your PR now. Please, don't be disappointed about my decision. Either way, thank you for your participation!

@andyruckstuhl
Copy link
Member

After our discussion I've refactored the TIME utility (see commit 0f49e67 for details).

In fact, the calculations of day of week, day of year and week of year can be implemented independent; and the DATE_PARTS struct was overloaded with these values.
In addition to that, some functions has an ENUM type parameter to specify required format or standard.

Thank you for your participation and your pointing to some disadvantages of my solution!

@Serhioromano
Copy link
Contributor Author

Why 2 functions internal and not internal?

@andyruckstuhl
Copy link
Member

Due to the following reasons:

  • encapsulation (the internal functions weren't exported)
  • internal functions accept ANY_INT as parameter (which should be avoided in public interfaces if a specific data type is present in standard library)
  • internal functions can be added, refactored, removed or changed otherwise in the future (e.g. if another standard or format of output is required)

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants