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

Added code and tests for event stream puzzler #40

Open
wants to merge 17 commits into
base: Daniel-EventStream
Choose a base branch
from

Conversation

dlinds
Copy link
Collaborator

@dlinds dlinds commented Jun 14, 2022

Ticket:
N/a

What this PR does
This PR request adds the following

  • Planning document with notes
  • Converts class and methods code into single function, and two interfaces
  • Jest for testing function

Media
image

How to test
To test, clone the repository to your computer and navigate to the project with the terminal. Download all necessary packages with yarn install and when ready, run yarn run test to execute the application. You should see several tests and the results of these tests.

To use your own seed data, please enter the following at the bottom of index.ts, replacing [] with your event stream input array.

const seedData: ReadonlyArray<Event> = []
console.log(getWinningRegion(seedData))

Once you've added in your code, in the terminal run yarn run dev to see the output

@dlinds dlinds requested a review from DuHerb June 14, 2022 00:21
@dlinds dlinds requested review from DuHerb and removed request for DuHerb June 14, 2022 16:39
@dlinds dlinds marked this pull request as ready for review June 14, 2022 16:40
src/index.ts Outdated
export interface EventInput {
Copy link

Choose a reason for hiding this comment

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

Suggested change
export interface EventInput {
export interface Event {

src/index.ts Outdated
export interface EventInput {
timestamp: Readonly<number>,
eventType: Readonly<string>
Copy link

Choose a reason for hiding this comment

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

this type should limit values to only be one of the three defined options.

src/index.ts Outdated

private _scoreTable: Record<Readonly<string>, Readonly<number>> = {
["newMessage"]: 1,
Copy link

Choose a reason for hiding this comment

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

brackets shouldn't be needed here

src/index.ts Outdated

private _setSortedWinners() {
const numOfRegions: number = 1 + (this._data.length - 5)
Copy link

Choose a reason for hiding this comment

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

don't have to type this as number. easily inferred by TS

src/index.ts Outdated
const numOfRegions: number = 1 + (this._data.length - 5)
if (numOfRegions < 1) {
throw "Not enough events to score. Please provide at least five"
Copy link

Choose a reason for hiding this comment

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

if the number of events is less than or equal to five, the function should return all events.

expect(badEventStream).toThrow("Input of object was found. Please provide a valid array.")
})

it("should fail with not enough obj provided", () => {
Copy link

Choose a reason for hiding this comment

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

if number of events is less than or equal to five, return all items.

})
})

describe('getUnsortedScores()', () => {
Copy link

Choose a reason for hiding this comment

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

this is not a requirement of the puzzle.

})
})

describe('getSortedScores()', () => {
Copy link

Choose a reason for hiding this comment

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

not part of the requirments

src/index.ts Outdated
regionId: regionNum,
score: regionScore,
inputLocations: [...regionNumbers]
Copy link

Choose a reason for hiding this comment

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

i'm unclear about the purpose of inputLocations. If we know the starting index of the highest scoring region, then we can calculate the range of the region.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! It was for getSortedScores() but I will remove that, then the input locations, per your previous comment

@DuHerb
Copy link

DuHerb commented Jun 14, 2022

Try to refactor all of your for loops to use Higher Order functions like reduce or map

@dlinds dlinds requested a review from DuHerb June 14, 2022 22:04
export interface Event {
timestamp: Readonly<number>,
eventType: Readonly<"newMessage" | "view" | "screenshot">
Copy link

Choose a reason for hiding this comment

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

using the readonly decorator would also be fine.

...
readonly timestamp: number,
...


export function getWinningRegion(inputData: ReadonlyArray<Event>): ReadonlyArray<Event> {
const scoreTable = {
Copy link

Choose a reason for hiding this comment

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

for the sake of this excercise, i'd like you:
encapsulate the scoring of a single event into a single function. Should return a number
encapsulate the scoring of an array into a single function. Should return a number

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! Hopefully my changes are what you were looking for!

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.

2 participants