So, I was applying for a job and they gave me a coding exercise. I feel like I did pretty well, but I got told that my code is not as elegant or robust as other candidates and I would love some feedback on how to improve it. If you have a moment, could you [look it over](https://github.com/zacksmash/finance-app) and let me know what I can do differently. Aside from tests. Since this was a trial app I didn't include tests...
Notable Directories:
app/Http
app/Imports
app/Jobs
app/Support
app/Transaction.php
resources/js
Thanks for your help!
Edit: The exercise was to create a financial ledger app. Adding, updating,, deleting entries and calculating the balance. With the ability to import transactions.
What was the task?
Did they say not to include tests? They probably wanted tests, especially since this seems to include API endpoints.
Also, the logic in [email protected] might be better handled at database level, and returning the new collection would be better than calling setCollection, that’s a bit odd.
The first thing I noticed was the lack of tests. This is honestly likely why others were considered a better choice over any specific architectural problems (especially because this looks like 99% boilerplate to me anyway).
Hi,
When dealing with APIs, I would expect yoo to use the API Resources provided by Laravel: [https://laravel.com/docs/7.x/eloquent-resources](https://laravel.com/docs/7.x/eloquent-resources)
This is more elegant than what you provided.
For the routes, going directly to the controller \_\_invoke method is not something I like to see. I prefer it to be more verbose.
Tests would have been great. Especially on a simple API like this one, it would have shown that you are used to TDD which is a great asset.
Otherwise, good job on fulfilling the assignment !
I noticed a few small things, some suggestions:
* Use Eloquent/model methods instead of DB (You did this in some places, not everywhere)
* Use tests
* Replace the standard Laravel readme, composer.json and package.json with your own
* Use `apiResource` instead of `resource` for the transaction routes, as you now have a create and edit route that cannot do anything
I haven’t run the application, but the code looks good enough to me. Like I said a few small suggestions, but that is all. I must say, for a job application this looks like a pretty big task.
I don’t see any models?
edit: ah i see just the 1
I looked over a little. First off, they screwed you. You should not do such an amount of work as a test to get hired unless they pay you for this.
Your JS/Vue looked good to me. A bit uncommon JS/HTML formatting, but that’s fine. Your Scss is full of [commented lines](https://github.com/zacksmash/finance-app/blob/master/resources/scss/components/_settings.scss), what for? Your class names seem *ad hoc*. BEM would be more readable in my opinion.
The Laravel part had some things not as elegant indeed. The first one I checked was the DashboardController.
public function __invoke(Request $request)
{
$total_balance = DB::table(‘transactions’)->sum(‘amount’);
return response()->json([
‘total_balance’ => number_format(($total_balance / 100), 2, ‘.’, ”),
‘import_status’ => jobIsQueued() ? true : false
], 200);
}
0. Why have `$request` as a parameter? You don’t seem to use it.
1. Why don’t you use the model? `Transaction::sum(‘amount’)` not `DB:…`.
2. What’s `jobIsQueued()`? I think most people would expect to see Laravel’s native `Queue::size()` there or a service (if it’s something more complex) not some global helper.
3. You could just return the array instead of wrapping it in `response()->json(/*..*/, 200)`. What you did is not wrong, but it implies you might not know that.
I also looked at the TransactionController.
1. Why do you wrap responses in `response(/*…*/)` all the time?
2. The `destroy` method would likely crash as it’s showing the transaction after it gets deleted.
3. The tinkering in `index` method seems a bit too complex. For example, do you even need a callback in `groupBy`? Didn’t `groupBy(‘date’)` work?
Anything outside controllers looks fine. But I would agree that “not as elegant” is a good way to describe your controllers. It’s not like you are doing it all wrong.
Finally, you should upgrade your git culture a little bit more. I see that mostly it’s in imperative as it should be. But sometimes a `Default date change` sneaks in. And sometimes you put too much in a single commit and the message shows that: `update pagination amount, add more styling/layout`.
For the jobIsQueued function in app/Support, it looks like you are just checking the jobs table for an entry to check it there are any jobs remaining. What if you swapped to a redis queue? And what if you had a few jobs in the table for other tasks?
Also not sure on the queue stuff in the app service provider. Using a switch before you have more than one case seems odd, though that’s more of a preference.
Overall there are definitely a lot if signs here that you are familiar with php and laravel (and packages like laravel excel).
> Edit: The exercise was to create a financial ledger app. Adding, updating,, deleting entries and calculating the balance. With the ability to import transactions.
How much time did you spend on this?
My 2cents :
I don’t think usage of \\DB is a problem, that does not show lack of programming skills/thinking, it’s just a manageability of code by puting extra functions to eloquent model or repository.
Test also don’t show your thinking, just your ‘long game’ and team ability – but that can be easily learned.
Your code structure is very clean, and business logic encapsulation very good. Code is very readable, so this is not the problem.
What I consider a problem, is that you are thinking only in succesfull code path, but you have complex user input. What happens when user gives you wrong file type, wrong columns, wrong column types, merged cells in xls, etc…
In other words: gracefull paths for error situations are missing.
And that shows your real world code thinking.
PS: I didn’t read js.
For me it is a not a technical issue on how to do this or that.
As some already pointed out, the purpose of this test was to know what was a priority for you.
You seem to choose the path “get the job done”. You wanted to build the entire app in the given time.
IMO the best approach in this case is to showcase your skills by writing good tests, good error handling and trying to be as bullet proof as possible. Even if all the features aren’t here.
This shows you can write solid and maintainable code the other can count on.
Freek from Spatie broadcasted this week the refactoring of the model-cleanup package. His method was interesting.
First he writes some quick doc to have ideas clear. Next he writes test. And the he writes the implementation. And so on. You can find this two part stream on his YouTube channel.
BTW, 8 hours of interviews is just full of disrespect for your time and work.
Hope it helps you, don’t be discouraged it’s a wonderful opportunity to learn 🙂
Sorry to hear dude. I’ve bombed coding tasks because the instructions were not clear on what they were looking for. It often comes down to the dev leads personal coding preferences. Also sometimes you just get unlucky.
Defs worth investing more into eloquent though. It handles complexity well while being quite readable. Worth it for the minuscule performance loss over a raw query.
I try to imagine what someone who reviews this code might do to filter out applicants. Sure some others have brought up some issues but I consider them too time consuming for an interviewer to find. If you were doing interviews with me I would open up your project in an IDE with some linting, I prefer PHPStorm.
– You left unused imports in HomeController.php
– Very often the PHPDoc comments do not match the return values of the function and the comments are not useful (they could be more specific)
– Sometimes you import classes but other times you don’t. In ImportController you could have used an import alias to import the Excel class which is not a Facade.
– ImportStatusController has more unused imports
– in the routes the api.php file has unused imports.
Just some things my editor quickly showed me when I opened your project and some things I noticed. Many developers don’t care too much about comments and documentation but where I work we are pretty strict about it. You could also add comments for classes like your controllers.
thank you for this post. i am currently learning laravel by my self (my first framework) and haven’t coded for a long time because i couldn’t find a job after my 4 year education. it is difficult to learn efficiently without working in a team where you can review code and discuss things. keep it up and good luck for your future job interviews
From a quick overview I don’t see anything major that stands out as bad or wrong. Sure there are some minor notes like some of the other comments mentioned with eloquent vs DB and tests.
One thing to consider is their mindset when reviewing these tests. If your code accomplishes the requirements, that is the most important part. It looks like your code fulfill the requirements in a very simple way. That is not necessarily a bad thing (KISS), specially with the time given.
When they say “not as elegant or robust as other candidates”, it could be that some other candidates over engineered their solution and did things in dozens of lines compared to you doing the same in 5 lines…. Sure their code might look more “robust” or “elegant”, but then you get into a subjective area.
If you can meet a requirements in 5 simple lines vs them doing it 100 complex and well written lines, which code is better?
Was this a take home assignment? Was this done in any way that tracked your time? Another possibility is that the other candidates used more than the allotted time.
At the end of the day, don’t lose your confidence. The hiring process in dev can be brutal. Just take it as a learning experience and keep pushing and learning. Good luck
I don’t know dude, looks pretty good to me, just small things.. Looks like they fucked you over by giving you a hard time and they might have been surprised about how much you delivered.
How did you feel you did in the interview? Because I would look more on the external factors if I was you. The code is more then fine don’t let anyone fool you.
I think people have covered most things to do with the code already.
I’m not sure how much you might’ve been expected to know about building a financial app, but, in accounting systems; practices like double-entry bookkeeping are very important. It could be that they scored you down because you neglected to implement some of the core system archictecture concepts they were looking for?
[https://en.wikipedia.org/wiki/Double-entry\_bookkeeping](https://en.wikipedia.org/wiki/Double-entry_bookkeeping)
Double-entry bookkeeping is just the start really. I suppose given you had 8 hours, it might not have been feasible to do that kind of research.
I’m surprised they asked you to do 8 hrs of work for them, but didn’t in turn take the time to give you some proper feedback on what they would like to see improved.
I consider myself lucky to never have done any of these “coding challenges” to get a job.
In importcontroller, you use a full namespace when you already imported it initially.
jobisqueued isn’t a good helper, honestly. It returns a result or null, when you’d expect it to be Boolean.
Just two I picked out.
Your code is fine. I’d gladly accept this. So long as you adapt the styles a team uses, these kinds of trials are mostly for weeding out the inexperienced. You’re not that. It’s important to realize that when someone says something is “more elegant,” it’s highly opinionated. Don’t value that kind of feedback on a task like this. It’s crap.
Edit: I’d just chip in on the consensus: don’t invest more than an hour on an interview. If they require more, they’d better damn well be paying a lot of money. Your time is valuable, treat it as such.
I trust that the other comments sufficiently judged your code, and since I’m on my phone I can’t contribute to that in detail. However, I noticed that you didn’t replace the default Readme.
When I need to hire developers (used so be a PM in laravel/vue projects) I value the ability to communicate efficiently with other devs. Good communication (eg Readme, clarifying comments, good commit messages) are as important writing good code.
It definitely sucks though, good luck om your next try! Make sure to show during your next interview that your not shy to ask for help and want to improve – that should help. Good luck!
Laravel has many issues if you want to write clean solid code.
I don’t have any feedback on your code, but I can honestly say that if an employer saw this thread they would hire you in a heartbeat. Your positive attitude and openness is a huge asset. I’d actually be inclined to share this thread in future interviews to show what sort if thinker you are.
\`jobIsQueued()\` was the biggest issue for me… I was looking around thinking wtf is this… Then I realised you created a global helper for barely any reason at all. They just make your code less readable.. other than a few minor tweaks you’re not doing anything particularly wrong.
What’s the purpose of overriding \_\_invoke in the DashboardController?
Tell me this wasn’t a junior position.
I’ve taken the liberty to make a PR with some feedback: [https://github.com/zacksmash/finance-app/pull/3](https://github.com/zacksmash/finance-app/pull/3)
Some of these are already addressed by the commenters here, but I also made some changes that I would personally do.
### Refactor importing of transactions
You don’t need to move the imported file to the `app/imports` directory, instead you can pass the `UploadedFile` instance to the import method. This way, you don’t need to delete the file after the job is completed.
Also, you don’t need a separate job to queue the import, instead you can use the `shouldQueue` interface on the `TransactionImport` along with the `WithChunkReading` interface.
Since you need to know the number of rows in the response, you can leverage the `BeforeImport` event to get the total number of rows. In the handler, I’m assigning it to a property that is read by the controller.
### Other opinionated changes
– Use API resources
– Use the correct @param and @return tags on docblocks
– Typehint parameters and return types where possible
– Remove helper and use `Queue::size()`
– Respond with `response()->json()` instead of `response()`
– Use `Transaction::count()` instead of `DB::table(‘transactions’)->count()`
– Sort transactions using `latest()` instead of `orderByDesc`
– Use array to separate validation rules instead of the pipe character
Seeing a few places where you’re doing short ternary with the end result still being a bool. e.g.: [https://github.com/zacksmash/finance-app/blob/master/app/Http/Controllers/DashboardController.php#L22](https://github.com/zacksmash/finance-app/blob/master/app/Http/Controllers/DashboardController.php#L22)
Looks like [jobIsQueued()](https://github.com/zacksmash/finance-app/blob/master/app/Http/Controllers/DashboardController.php#L22) is returning the first row returned, so this could be simplified, as we know we’re going to get data, or not. E.g.:
`’import_status’ => (jobIsQueued()),`
Alternatively, as this method is clearly designed (by name) to determine if a job is queued, you should probably just return a bool, rather than the row.
Also latest Laravel is PHP 7.2 and up. So consider scalar and return typing to ensure you’re not getting into a state of GIGO (Garbage In Garbage Out) and bailing early when data being input is bad.
I would use DB instead of Eloquent as well, because I’ve been doing SQL queries since dirt was invented. To me, Eloquent is not so … ‘eloquent’. And I know for a fact that my raw queries are faster than an Eloquent version, except for maybe the simplest query (like selecting a few fields from one table). If, as someone mentioned , the company doesn’t “like” the fact that you used DB, then I’d look for another company that has a more flexible attitude. I just find raw SQL easier to read, so it is simply my preference.
> Aside from tests. Since this was a trial app I didn’t include tests…
To be honest, this would definitely put me off of hiring you. No offense, but even if you didn’t know how exactly best practices in laravel worked, considering test to be somehow less important than features is a rampant misconception in our industry. For instance, if this was timed I’d rather have you hand in fully formulated tests (for laravel I’d concede mostly end to end tests are sufficient) covering most cases than the finished project.
Something real for you to consider is this
`Route::get(‘dashboard’, ‘DashboardController’);`
`Route::resource(‘transactions’, ‘TransactionController’);`
`Route::post(‘import’, ‘ImportController’);`
`Route::get(‘import-status’, ‘ImportStatusController’);`
By using ImportStatusController::class instead of the name as a string you can leverage your IDE in refactorings. The string won’t get detected in a renaming scenario.
Your Controllers do too much sometimes. Especially the TransactionController is an offender in this regard. As a rule I try to never go deeper in scope than the public functions. If you feel the need to do this, a method on a model or even a service might be in order. “TransactionController” would seem, from the name, to be a good place for business logic concerning transaction, but what it is supposed to do (especially in laravel) is control the dataflow towards your transaction business logic. This is true for any controller in laravel, since they are our entry points for outside data into our internal system.
I do hope this makes sense. Since I am not a native speaker sentence structure might be off. Hit me up if something is unclear.
This was really cool to see as I’m just learning Laravelx starting out. Thank you for sharing! I’m trying to read through all the comments; sorry is this has already been covered.
The biggest part that stuck out to me was the SASS code. It’s not often I see IDs used SASS. It’s also best not to over specify, `section.class` could simply be `.class`. I didnt understand the purpose of some of the mixins, even as commented, and they looked overtly complicated. Finally, try to keep nesting to a minimum.
Hey, a lot of people already habe good Feedback about the Job you did. I wanted to Share Something that realy helped me to Unserstand how to build scaleable Projects which are still manageable and testable even with great complexity.
First of all Design patterns:
https://github.com/kamranahmedse/design-patterns-for-humans
“Design patterns are solutions to recurring problems;Â guidelines on how to tackle certain problems. They are not classes, packages or libraries that you can plug into your application and wait for the magic to happen. These are, rather, guidelines on how to tackle certain problems in certain situations.”
The orher Thing is dependency injection that laravel uses almost everywhere.
“Dependency Injection in Laravel” von Vahit Saglam https://link.medium.com/wJHkuMdin8
I dont want to Go to much into Detail there since I think the links I sent explained it better than I every could.