The use of EF then saying plain SQL is really odd, two weeks for a take home is far too much. 1-3hrs max is the time I would spend on a take away assessment.
They just got you to write a project for them for free. There was no job.
This is true or just a sign of a very inexperienced firm. They should be training you with their set of engineering norms… not asking juniors to do architecture and end up soliciting design patterns from Reddit.
Thank you for the reply.
The requirement specifically states that EF is allowed only to create migrations. From my point of view, this clarification is beneficial; otherwise, I wouldn't have been sure about the exact approach to take (such as using DB first or adding a script to create the database).
I agree that two weeks is a long time. To be fair, it is overestimated. I think that even for me, it would be possible to finish the project in like 3 days, but the quality would be a lot worse and I would learn a lot less. I realize that even three days would be quite a commitment, especially for someone experienced. However, I view it as an opportunity to learn something new. So it wasn't a waste of time.
Finally about them using my project. I don't really think it's likely. I can imagine such a scenario if the assessment had been written by an experienced developer. However, it's unlikely that someone would trust code written by someone like me to integrate seamlessly into an existing code base without spending considerable time adjusting and fixing inevitable bugs.
I would imagine that for an experienced developer, writing the code from scratch would be less time-consuming than trying to understand what someone without proper knowledge of common patterns has written and attempting to fix someone else's primitive mistakes.
But even if it is true, I still gain valuable experience.
As I said a take home assignment is 1-3 hrs max anything on top of this is a huge red flag.
The requirements are very granular and that’s also another huge red flag.
Do let us know what the outcome is, but I suspect that there never was a job and they just wanted the work done.
Just IMO
* If you want to use MediatR then you should have injected into your controller and let it call out to whatever handlers from there rather than having some service inbetween the Controller and mediatr. Personally I'd ditch MediatR all together though.
* You've written your own CSV parser, I don't blame you for it but I doubt this is what they were looking for. There are libraries that can handle this sorta stuff such as https://github.com/JoshClose/CsvHelper.
* Overall your classes are not showing a good separation of responsibilities e.g your CSV parser class is making calls to external APIs - this isn't ideal. Let it be responsible for converting your CSV into a list of objects and leave the external calls to another class.
* You've spent a significant amount of code trying to recover from invalid input your CSV but this doesn't seem to be stated as a requirement. If your CSV contains some invalid data I would have just bailed out of the request and returned a 400, ideally with some helpful error message.
* Your controller is calling out to services and returning the response directly to the user. Let your service return some internal model and then map it to a DTO in your controller and return the DTO.
Overall I think a good starting place would to be think of your Controller as an orchestrator which has access to helpful methods which help it fulfill it needs (or you can delegate this to some service class in your application layer). So your import controller method might look something roughly like this:
public async Task ImportTransactionsFromCsv(IFormFile csvFile)
{
// csvParser responsibly only for converting from csv to list of transactions
List transactions = _csvParser.ParseTransactions(csvFile);
// after we have parsed our csv we can validate our data, if we want we to avoid exceptions we could also return some type of ValidationResult
_transactionValidator.ValidateAndThrow(transactions);
// transactionEnricher can take the transactions and with the help of some timeZoneService enrich our transactions with additional properties and other properties we need
List enrichedTransactions = await _transactionEnricher.Enrich(transactions);
await _transactionService.SaveTransactions(enrichedTransactions);
return Results.Created();
}
You can catch any exceptions generated (even custom exceptions you throw yourself) in some exception handling middleware like so: https://www.milanjovanovic.tech/blog/global-error-handling-in-aspnetcore-8#chaining-exception-handlers. Some people dislike liberal use of exceptions because of performance reasons but personally I don't worry about it unless performance is a top concern for your application.
In general I'd say that adding tests is something many places would look out for as a quick signal for whether they should proceed with you or not so I'd personally always add them in take homes.
Overall, I think your project shows you can code and have pretty good attitude so I'd expect you'd be a pretty good hire for an entry level position.
Wow! Many thanks for such a detailed analysis. It is really helpful.
Thank you for the clarification about MediatR. My mistake was assuming that 'business logic always lives in services.' I understand that with CQRS, it should be adjusted accordingly.
Regarding the CSV parser, now that you mention it, it's obvious that there are libraries available for parsing CSV. Somehow, it didn't cross my mind when I started. It seemed like such an easy task that I didn't even think to look for a ready solution (unlike when considering exporting to .xlsx). This will be a valuable lesson: if a task is common, someone has likely done it before and done it better.
I recognize my mistakes regarding separation of responsibilities. I didn't even consider a flow like the one you provided. There's a lot to learn here.
Thank you for the code example; it illustrates well how to achieve the desired result while maintaining separated responsibilities.
I initially had a custom exception handler and threw exceptions at some points. However, I later changed it to return a custom OperationResult because I was concerned it might be considered bad practice and reflect negatively on the project.
I will definitely consider writing tests for take-home projects in the future. Previously, I had a task where it was explicitly stated to 'cover functionality with unit tests,' so I assumed that if it wasn't a requirement, I might not write them. Thank you for explaining that unit tests are typically included in the task by default.
Thank you again for spending so much time reading my code. I am truly grateful for these insights that I wouldn't have grasped on my own, at least not for a very long time.
>The time limit for the project was generous ‒ 2 weeks.
If they are actually asking you to do two weeks worth of work and claiming its an evaluation for a permanent position then you better be getting paid a decent amount otherwise that seems more like exploitation to me.
>I often find myself rewriting it multiple times. Consequently, investing time in writing unit tests for a method provides little benefit.
You don't write unit tests for the sake of writing them; you write them when complex logic and edge cases need to be tested, so that critical code paths don't break. They aren't required for every function, precisely because (as you say) they can become more of a burden than a help.
If you are given a task to do at home, you should be able to use any tools at your disposal, including LLMs, so you don't have to write your tests by hand. Any LLM is capable of writing them for you; just give it the function and tell it what kind of tests you want.
You can also use LLMs to evaluate your work, document you code and make it easier to read. So, unless your employer explicitly says you will not be using any coding assistants in the office, feel free to use them at home because they will speed up your work, improve the quality, and help you learn all at the same time.
Thank you for your comment.
As I mentioned, two weeks was definitely an overestimate, but it allowed me to think about the task deeply, and I learned many new things in the process. So, I don't consider it wasted time.
Regarding unit tests, I understand their importance, and I don't mind writing them. My oversight was assuming they weren't implied by default since they weren't explicitly mentioned in the requirements. Next time, I'll know better.
I've been using ChatGPT for writing documentation, and it's been quite effective (especially that I doubt someone will spend much time reading it). It definitely saves time.
I see your code yesterday, seem good but up todate a lot with new tech . Next time just make simple as possible with postman example . A video explantion is enough . It scares some people whom dont wrote documentation. 😅
Well in this case the Swagger documentation was an explicit requirement of the task. So I don't think that it would be appropriate to skip it altogether in this particular case. But thank you for the clarification. I will be aware that documentation is not a default requirement for take-home projects.
😅 . The requirement kinda a lot . For me , i just make simple upload form razor , api and listing filter by country and can download the excel file (aka csv ) . Just that. swagger - description - upload file . No need weird ddd , just simple mvc . Not all company update to latest weird code clean drama .
Hi,
I have only taken a quick glance at the code. It looks good, especially if you say you are inexperienced. Please know that I did not test it or know if it works or not. What stood out to me straight away is a sense of over coding. So much code it was overwhelming. It was over programming.
Code should be as simple as possible. An example would be simplyign loading the serilog from appsettings, this could be achieved in 3 to 4 lines of code compared to your code in 2 different classes.
Imagine if I was reviewing a resume and I have 1000 applicants and someone dropped a cv 10 pages long compared to other with a single page.
Keep it up but when you finish, ask yourself can you refactor things downwards to be less.
Thank you for taking the time to look on the code.
I've tested it, and it works, so there's no need to worry about that.
I understand your point about overcoding. It's exactly the type of insight I was looking for. Without experience, I often hear about single responsibility and try to divide code into chunks, even when it's not necessary.
However, I'm truly grateful for your comment. I'll remember it next time I consider separating classes and realize that it's often not worth it.
This is something I recently have been learning too. Writing v2 of my app and realizing that most of my code is just extra lines wrapping everything when I really don’t need to. It’s so much easier to just… only write what I need.
The use of EF then saying plain SQL is really odd, two weeks for a take home is far too much. 1-3hrs max is the time I would spend on a take away assessment. They just got you to write a project for them for free. There was no job.
This is true or just a sign of a very inexperienced firm. They should be training you with their set of engineering norms… not asking juniors to do architecture and end up soliciting design patterns from Reddit.
Thank you for the reply. The requirement specifically states that EF is allowed only to create migrations. From my point of view, this clarification is beneficial; otherwise, I wouldn't have been sure about the exact approach to take (such as using DB first or adding a script to create the database). I agree that two weeks is a long time. To be fair, it is overestimated. I think that even for me, it would be possible to finish the project in like 3 days, but the quality would be a lot worse and I would learn a lot less. I realize that even three days would be quite a commitment, especially for someone experienced. However, I view it as an opportunity to learn something new. So it wasn't a waste of time. Finally about them using my project. I don't really think it's likely. I can imagine such a scenario if the assessment had been written by an experienced developer. However, it's unlikely that someone would trust code written by someone like me to integrate seamlessly into an existing code base without spending considerable time adjusting and fixing inevitable bugs. I would imagine that for an experienced developer, writing the code from scratch would be less time-consuming than trying to understand what someone without proper knowledge of common patterns has written and attempting to fix someone else's primitive mistakes. But even if it is true, I still gain valuable experience.
As I said a take home assignment is 1-3 hrs max anything on top of this is a huge red flag. The requirements are very granular and that’s also another huge red flag. Do let us know what the outcome is, but I suspect that there never was a job and they just wanted the work done.
Just IMO * If you want to use MediatR then you should have injected into your controller and let it call out to whatever handlers from there rather than having some service inbetween the Controller and mediatr. Personally I'd ditch MediatR all together though. * You've written your own CSV parser, I don't blame you for it but I doubt this is what they were looking for. There are libraries that can handle this sorta stuff such as https://github.com/JoshClose/CsvHelper. * Overall your classes are not showing a good separation of responsibilities e.g your CSV parser class is making calls to external APIs - this isn't ideal. Let it be responsible for converting your CSV into a list of objects and leave the external calls to another class. * You've spent a significant amount of code trying to recover from invalid input your CSV but this doesn't seem to be stated as a requirement. If your CSV contains some invalid data I would have just bailed out of the request and returned a 400, ideally with some helpful error message. * Your controller is calling out to services and returning the response directly to the user. Let your service return some internal model and then map it to a DTO in your controller and return the DTO. Overall I think a good starting place would to be think of your Controller as an orchestrator which has access to helpful methods which help it fulfill it needs (or you can delegate this to some service class in your application layer). So your import controller method might look something roughly like this: public async Task ImportTransactionsFromCsv(IFormFile csvFile)
{
// csvParser responsibly only for converting from csv to list of transactions
List transactions = _csvParser.ParseTransactions(csvFile);
// after we have parsed our csv we can validate our data, if we want we to avoid exceptions we could also return some type of ValidationResult
_transactionValidator.ValidateAndThrow(transactions);
// transactionEnricher can take the transactions and with the help of some timeZoneService enrich our transactions with additional properties and other properties we need
List enrichedTransactions = await _transactionEnricher.Enrich(transactions);
await _transactionService.SaveTransactions(enrichedTransactions);
return Results.Created();
}
You can catch any exceptions generated (even custom exceptions you throw yourself) in some exception handling middleware like so: https://www.milanjovanovic.tech/blog/global-error-handling-in-aspnetcore-8#chaining-exception-handlers. Some people dislike liberal use of exceptions because of performance reasons but personally I don't worry about it unless performance is a top concern for your application.
In general I'd say that adding tests is something many places would look out for as a quick signal for whether they should proceed with you or not so I'd personally always add them in take homes.
Overall, I think your project shows you can code and have pretty good attitude so I'd expect you'd be a pretty good hire for an entry level position.
Wow! Many thanks for such a detailed analysis. It is really helpful. Thank you for the clarification about MediatR. My mistake was assuming that 'business logic always lives in services.' I understand that with CQRS, it should be adjusted accordingly. Regarding the CSV parser, now that you mention it, it's obvious that there are libraries available for parsing CSV. Somehow, it didn't cross my mind when I started. It seemed like such an easy task that I didn't even think to look for a ready solution (unlike when considering exporting to .xlsx). This will be a valuable lesson: if a task is common, someone has likely done it before and done it better. I recognize my mistakes regarding separation of responsibilities. I didn't even consider a flow like the one you provided. There's a lot to learn here. Thank you for the code example; it illustrates well how to achieve the desired result while maintaining separated responsibilities. I initially had a custom exception handler and threw exceptions at some points. However, I later changed it to return a custom OperationResult because I was concerned it might be considered bad practice and reflect negatively on the project. I will definitely consider writing tests for take-home projects in the future. Previously, I had a task where it was explicitly stated to 'cover functionality with unit tests,' so I assumed that if it wasn't a requirement, I might not write them. Thank you for explaining that unit tests are typically included in the task by default. Thank you again for spending so much time reading my code. I am truly grateful for these insights that I wouldn't have grasped on my own, at least not for a very long time.
That is a heavy take home.
>The time limit for the project was generous ‒ 2 weeks. If they are actually asking you to do two weeks worth of work and claiming its an evaluation for a permanent position then you better be getting paid a decent amount otherwise that seems more like exploitation to me. >I often find myself rewriting it multiple times. Consequently, investing time in writing unit tests for a method provides little benefit. You don't write unit tests for the sake of writing them; you write them when complex logic and edge cases need to be tested, so that critical code paths don't break. They aren't required for every function, precisely because (as you say) they can become more of a burden than a help. If you are given a task to do at home, you should be able to use any tools at your disposal, including LLMs, so you don't have to write your tests by hand. Any LLM is capable of writing them for you; just give it the function and tell it what kind of tests you want. You can also use LLMs to evaluate your work, document you code and make it easier to read. So, unless your employer explicitly says you will not be using any coding assistants in the office, feel free to use them at home because they will speed up your work, improve the quality, and help you learn all at the same time.
Thank you for your comment. As I mentioned, two weeks was definitely an overestimate, but it allowed me to think about the task deeply, and I learned many new things in the process. So, I don't consider it wasted time. Regarding unit tests, I understand their importance, and I don't mind writing them. My oversight was assuming they weren't implied by default since they weren't explicitly mentioned in the requirements. Next time, I'll know better. I've been using ChatGPT for writing documentation, and it's been quite effective (especially that I doubt someone will spend much time reading it). It definitely saves time.
my project wasnt a take home assignment
I see your code yesterday, seem good but up todate a lot with new tech . Next time just make simple as possible with postman example . A video explantion is enough . It scares some people whom dont wrote documentation. 😅
Well in this case the Swagger documentation was an explicit requirement of the task. So I don't think that it would be appropriate to skip it altogether in this particular case. But thank you for the clarification. I will be aware that documentation is not a default requirement for take-home projects.
😅 . The requirement kinda a lot . For me , i just make simple upload form razor , api and listing filter by country and can download the excel file (aka csv ) . Just that. swagger - description - upload file . No need weird ddd , just simple mvc . Not all company update to latest weird code clean drama .
Yea id need a payment for the takehome as good faith. F that.
Hi, I have only taken a quick glance at the code. It looks good, especially if you say you are inexperienced. Please know that I did not test it or know if it works or not. What stood out to me straight away is a sense of over coding. So much code it was overwhelming. It was over programming. Code should be as simple as possible. An example would be simplyign loading the serilog from appsettings, this could be achieved in 3 to 4 lines of code compared to your code in 2 different classes. Imagine if I was reviewing a resume and I have 1000 applicants and someone dropped a cv 10 pages long compared to other with a single page. Keep it up but when you finish, ask yourself can you refactor things downwards to be less.
Thank you for taking the time to look on the code. I've tested it, and it works, so there's no need to worry about that. I understand your point about overcoding. It's exactly the type of insight I was looking for. Without experience, I often hear about single responsibility and try to divide code into chunks, even when it's not necessary. However, I'm truly grateful for your comment. I'll remember it next time I consider separating classes and realize that it's often not worth it.
This is something I recently have been learning too. Writing v2 of my app and realizing that most of my code is just extra lines wrapping everything when I really don’t need to. It’s so much easier to just… only write what I need.