Open
Conversation
Fixed misc warnings from initial repo fork/clone.
Objective/filtered search
Added company to API and Repo side.
Added seeds.exs with test data.
Added utility and implementation for converting money types.
Fixed company not calculating available credit and removed the persis…
Author
|
@thawk55, @jakerichan - I can't add you as reviewers, but thought I'd ping you in here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
@thawk55 and @jakerichan
Completed Objectives
The following are the objectives I was able to complete with comments and reasons behind my changes.
Due to extreme time constraints with my current job (being on support last week 😬), I was only able to fully complete 4 and partially complete 1 of the 6 objectives. Overall it took me about 1 week (working on it between work and family).
Also, I didn't know how to submit this other than giving one big PR, but if you want to see individual PRs and commits on my GitHub repo check it how here.
Objective: Filtering
The fuzzy search feature is something I've never actually done before, so I was torn between the options of using the database to process the search or the application logic. Although it goes against my norm (which is to leave calculation to the application, which is much easier to scale than a database), I went with the database option. First, it was the fastest and most intuitive to implement, because I was able to leverage pre-existing Postgres functions. Doing the fuzzy search on the application side brought with it questions about how much of a data set to query in order to do an accurate search and what is the performance hit on both the db and the app if I pull back too much.
Just like the fuzzy search, it was easiest and fastest to put the search on the database (adding indexes on the date fields would probably be preferred, but didn't have enough time).
Objective: Company Schema
The available credit implementation is basic, but isn't scalable. It uses every transaction for every related user. If there was more time, I would have considered having something else, like persisting "latest transaction id" and the "latest available credit" (perhaps).
Objective: Database Seeding
This was fairly simple. I only added enough data to be able to use it for the 3rd interview demonstration.
Objective: Decimals to Integers to Decimals
This was the simplest to implement. To avoid business logic creeping into the data models, the GraphQL resolvers used a money converter util which could convert any given structs "money fields" from floats to integers and back again.
Objective: Pagination (partially complete)
I wasn't able to complete this one fully due to time, however, I do have a branch on my GitHub repo that has two different patterns implemented.
To see more visit the changes on my branch on GitHub.
Other Thoughts
Knowing the goal of this assignment is to allow me to exhibit all aspects of my skill and experience, I have other thoughts to highlight.
Bonus Tasks
Bug with Transactions - The obvious/immediate bug I could find is that
creditwas not "cast" into the model object from the changeset. However, I would think the bigger bug would actually be that there are both adebitand acreditfield when the negative of one is the positive of the others, so you probably only need one of them (i.e.debit). I don't know enough about finance logic though to say if my assumption is accurate.Security Issue - Didn't spot this one, but wasn't really looking for it :(
Docs - I added documentation to my own changes, but didn't have enough time to add more to the rest of the already present code.
Testing
I wasn't able to introduce any tests, however, I'm usually very test-driven, heavily emphasizing unit tests (with the appropriate amount of coverage).
Elixir
The Elixir learning curve wasn't as difficult as it originally seemed. Once I got the hang of pattern matching and how powerful it is, the rest was mainly just getting used to libraries. I really enjoy the documentation
Package Structure (
../elixir/homework)I'm not yet familiar with idiomatic elixir package structure or conventions used at Divvy (including repo setup). However, if after considering all those things, and I still had some wiggle room left over, I might have explored some opportunities to structure the packages differently. 🤷
Soft Deletes
Soft deletes are a topic with good points on either side. Nevertheless, I lean toward having them because I've found them to be incredibly helpful to recover data when systems mistakenly "deletes" it. If I had more time, I would've added a soft-delete mechanism using an indexed
deleted_atcolumn on each table (much likeinserted_at).