Solve Event puzzler with tests #42
Conversation
src/mockTestData.ts
Outdated
| timestamp: 123123125, | ||
| eventType: 'view' | ||
| } | ||
| ] |
There was a problem hiding this comment.
new line.
Also, indentation is all over the place in this file, and between all files. Tab should be the same in all files. In general, I would go with 2.
There was a problem hiding this comment.
I'm not totally sure why the indentation is all over the place. My standard is 2 spaces for a tab. I did find three spot on my local branch that I need to tab in. But my local branch is not nearly as screwy as this.... I'll pay more attention after I push it up this next time.
src/index.ts
Outdated
| if(stream.length <= 5 ) { return stream } | ||
|
|
||
| const newStream: Array<IEvent> = [...stream] |
There was a problem hiding this comment.
shouldn't need a newStream or spread, as you are only going to iterate over the input stream.
src/index.ts
Outdated
| const newStream: Array<IEvent> = [...stream] | ||
| const newStreamValues: Array<number>= [] | ||
| stream.forEach( e => newStreamValues.push(assignPointValue(e.eventType))); |
There was a problem hiding this comment.
can you refactor this to use map or reduce
src/index.ts
Outdated
| let index = 4 //Start with the 5th event in the stream becuase the for loop looks backward and ends at the end of the array | ||
| let highestValue = 0 | ||
| for(let i=4; i<newStreamValues.length; i++) { // Same here, Want to start at the 5th event in the array |
There was a problem hiding this comment.
i'm not sure I understand why we need to look backwards over results.
In general, I'm finding the algorithm difficult to understand. One way to mitigate readability/maintainability, is to further encapsulate logic into smaller functions.
You may find it useful to build a helper function that takes an array of events and just provides a score. Or similar such function.
There was a problem hiding this comment.
I encapsulated finding the index into a helper function.
I managed to get rid of the nested for loop, but not really happy with the hard coding in finding the sum of the values. Is there a better way to sum the values of a sub array?
|
In general, I like that you've encapsulated different logic in to smaller, more maintainable code. however, the fact that we are still looking backwards into arrays smells fishy to me. can you think of a solution where your function can identify/record the index of the first event in the high scoring region? |
src/index.ts
Outdated
| value?: number | ||
| } | ||
| export type eventTypeState = 'newMessage' | 'screenShot' | 'view' |
There was a problem hiding this comment.
| export type eventTypeState = 'newMessage' | 'screenShot' | 'view' | |
| export type EventType = 'newMessage' | 'screenShot' | 'view' |
types/interfaces should be capitalized
src/index.ts
Outdated
| for (let i = 4; i < stream.length; i++) { | ||
| const currentSum = | ||
| stream[i].value! + |
There was a problem hiding this comment.
adding the value prop and then looking it up feels like an unnecessary step. Can just get the value directly here via assignPointValue(stream[i].eventType)
src/index.ts
Outdated
| readonly timestamp: number, | ||
| readonly eventType: eventTypeState, | ||
| value?: number |
There was a problem hiding this comment.
extra step. see comment in findLastIndex....
src/index.ts
Outdated
| let maxCumulativeSum = 0 | ||
| for (let i = 4; i < stream.length; i++) { | ||
| const currentSum = |
There was a problem hiding this comment.
getting the scores for an array of events can be delegated to a helper function.
The current solution, while working for this exact use case, is not very maintainable. For instance, what if the requirement for the function changed so that the length of the scored array was changed to 7. Now you have to add two more lines here, and adjust hardcoded values all over the place.
In another scenario, what if the the requirement was that the region length was completely variable? In that case, this solution won't work at all.
Encapsulation helps handle those types of solutions.
Instead of assigning the value to each object, I decided to assign the region value to each object. Completed that and realized it was just easier to create an array of the region values. So I refactored it again to it's current state. Create an array of values corresponding to the regions. Then find the max value, and index, then return the region from the original stream. This seems a lot nicer than my first approach. Thank you. |
Ticket:
What this Does:
Media:
Testing