Skip to content
This repository has been archived by the owner on Feb 12, 2022. It is now read-only.

Increase readability and maintainability: scheduler setValue #1635

Merged
merged 3 commits into from
Jan 26, 2016
Merged

Increase readability and maintainability: scheduler setValue #1635

merged 3 commits into from
Jan 26, 2016

Conversation

interactivellama
Copy link
Contributor

So that it doesn't change from the values set via setValue.

This means that the time offset is ignored in the startDateTime option.

@swilliamset
Copy link
Contributor

@interactivellama please look into the CI fail

@swilliamset
Copy link
Contributor

image

@interactivellama interactivellama changed the title Remove most of logic from setUtcTime Remove most of logic from setUtcTime and refactor setValue Dec 28, 2015
@interactivellama
Copy link
Contributor Author

Updated @swilliamset

I couldn't allow the trainwreck that is setValue to remain. Not only didn't the function use a variable named temp, it also re-used the variable multiple times and re-assigned values via array indexes when it was currently an array. I felt like I was back in "we-have-to-make-this-work-in-640k-memory-land." ->

swilliamset
swilliamset reviewed Jan 4, 2016
View reviewed changes
js/scheduler.js
var date = d.split('-');
var time = t.split(':');
setUtcTime: function setUtcTime(day, time, offset) {
var dateArray = day.split('-');
Copy link
Contributor

Choose a reason for hiding this comment

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

change variable name to dateSplit and timeSplit for consistency with other split results below. why not *Array? personal preference. varnameArray[0] feels redundant.

@swilliamset
Copy link
Contributor

Unable to determine if interactivellama@2979e6067733cb986059380e84b6df56b1ea86a2is the solution or just happy coincidence. Without a more complete range of unit tests the side affects are unknown. Please provide an assessment of the effort required to improve unit tests.

@swilliamset swilliamset mentioned this pull request Jan 5, 2016
Closed
@interactivellama
Copy link
Contributor Author

Will remove actual "bugfix" and submit the refactor part and open a new PR with the "bugfix" change.

@interactivellama
Copy link
Contributor Author

setUtcTime was added at this point. I will follow up with @BenjaminNeilDavis for background when he becomes available. 008788b

@interactivellama interactivellama changed the title Remove most of logic from setUtcTime and refactor setValue Increase readability and maintainability: scheduler setValue Jan 5, 2016
@interactivellama
Refactor scheduler setValue
aafcf7f 
- setValue is now "a one screen function"
- creates `_parseRecurrencePattern`, _parseStartDateTime`, `_parseTimeZone`
- Re-uses `_parseStartDateTime` for timezone parsing
- Removes commented out utcTime manipulation
- Renamed variables to understandable things
- Same functionality present, just increased readability
@interactivellama
Rename scheduler variables
526492a 
Like this: semiColonSplitPattern -> semiColonPatternSplit
@interactivellama
Copy link
Contributor Author

Updated with refactor only. "Bugfix" removed.

@interactivellama interactivellama added this to the 3.14.0 milestone Jan 5, 2016
@swilliamset swilliamset mentioned this pull request Jan 12, 2016
Closed
@swilliamset
Copy link
Contributor

still need unit tests. assigning to @dwaltz. @interactivellama, please pass any knowledge to him.

@swilliamset swilliamset assigned dwaltz and unassigned swilliamset Jan 19, 2016
@swilliamset swilliamset modified the milestones: 3.13.1, 3.14.0 Jan 26, 2016
swilliamset added a commit that referenced this pull request Jan 26, 2016
@swilliamset
Merge pull request #1635 from interactivellama/scheduler-set-value
928c70f 
Increase readability and maintainability: scheduler setValue
@swilliamset swilliamset merged commit 928c70f into ExactTarget:master Jan 26, 2016
@swilliamset
Copy link
Contributor

@dwaltz confirmed unit tests cover is good #1689

@dwaltz dwaltz mentioned this pull request Jan 26, 2016
Merged
@interactivellama interactivellama deleted the scheduler-set-value branch April 4, 2016 15:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@dwaltz dwaltz

Labels
scheduler
Projects
None yet
Milestone
3.13.1
Development

Successfully merging this pull request may close these issues.
3 participants
@interactivellama @swilliamset @dwaltz