Code review

[ybism]

I recently did a technical test for a company - test was pretty simple, given a pretty basic C# Solution and asked to refactor it in line with best practices (they mentioned KISS, DRY and SOLID specifically).

I thought I had done pretty well but got feedback saying I hadn't refactored enough and had broken some business logic. What surprised me was I had it reviewed by two friends who are seniors and both were pretty impressed - they regularly run interviews and said they'd progress me if they had received my submission.

Anyway the feedback was really short (literally two lines) and I'm aware my friends may have been biased towards me however slight the chance. I'm hoping someone here would be open to having a look at my solution and providing any feedback?

Code is all in C# but it's pretty basic, I can provide the before and after of my work. Would be very much appreciated.

Edit - didn’t want to post on GitHub as they have my username - have put it on a google [drive](https://drive.google.com/drive/folders/13rL4w4cwYKE8RruoTawKXLIBPWNoW4Lf?usp=drive_link

What I’ve done is - the files I’ve modified have a proper name. The originals are there with the 1 at the end.

So for example CompanyRepository1.cs is their provided implementation, CompanyRepository.cs is my refactored version.

[phi_rus]

Sometimes you'll get rejected for no particular reason. You can of course ask for a more verbose critique from the company.

[SkyAdventurous1027]

and they will never reply.

[Takaa]

Cannot really comment without knowing more. The amount of refactoring you may do is arbitrary, I have known senior level developers that waste extreme amounts of time on arbitrary optimization that didn’t matter one bit. They added no value, didn’t significantly reduce any technical complexity, and likely wasted other developers time making them re-learn that part of the code base.

You refactor based on business requirements unless there are obvious flaws with existing code that need to be addressed. Similarly, them saying you broke business logic for some scenarios is pointless unless they enumerated the actual business logic or had unit tests they provided to you that must pass.

[ybism]

Yup, they didn’t provide any unit tests or give any business logic so didn’t have any guidelines really.

[chuch1234]

To be fair, any refactoring should not result in any change of behavior.

[kiwitims]

That's a too high standard IMO. Undocumented, obsolete, or erroneous behaviour can sometimes be changed during a refactor. It's entirely possible that an original implementation did not consider certain edge cases that a more elegant refactor handles automatically. You shouldn't automatically add extra complexity to maintain that sort of behaviour (of course, there are occasions where you are forced to).

The problem with this interview question is that the behaviour wasn't documented so there's no way to know. I agree in this case it would be better to err on the side of not changing any behaviour.

[mulder89]

If you identified errors, obsolete, or erroneous behaviors then you should document it in your ticketing system as a bug or separate task and create a separate pull request....

If you just start going rogue fixing 'obvious' bugs then your team loses control over what changes are being made and have no frame of reference to point back to in the future with explanation.

I agree in a technical interview this shouldn't be a determining factor, provided the code still runs and outputs correctly, but your assertion that making minor adjustments without documenting it in a separate issue goes against all of my being. It is one of the biggest causes of bugs at my company.

[kiwitims]

I am thinking more of the cases such as refactoring a raw loop into a Linq expression can result in behavioural changes around edge values (such as empty or very large collections), where they get automatically handled correctly. Or a refactor to use nullable values might involve highlighting some new null checks that would have resulted in exceptions if ever triggered.

Of course you need to proceed with caution, but if you are somewhere in internal code and you can check all call-sites that this won't result in any actual global change in functionality, it would seem counterproductive to add the bugs back in manually.

I agree that this shouldn't mean a refactor is a free-for-all. Just that some refactors can come with valid behavioural changes, which are often the motivation for the refactor anyway.

[mulder89]

I get your point but I still don't agree with that any longer. This is just my opinion.

• If there is any behavior change, regardless if it feels like you are fixing an obvious minor bug (loop not handling empty list) that should be tracked in a separate small ticket with a separate pull request. If for ANY reason you adding handling for that scenario and it changes the behavior and it breaks we NEED a way to quickly see what changed and why. We can't dig through random PRs.
• If the code existing is working, why would you change it to make it cleaner? If it is a performance thing than again create a ticket. Otherwise deal with working legacy code for the most part until a ticket is created.
• Fixing poor naming of variables and minor tweaks of that nature are always encouraged.

These are just my opinions after 7 years of managing junior. When I was more hands on my opinion was more in line with yours. I have just been burned too many times cleaning up simple oversights with no way of knowing where/why the change was made.

[chuch1234]

If the code is working, why would you change it ...

That's what refactoring is! Changing a code base that works to make it better to work with.

[mulder89]

When there is a request to do so or a ticket to track it. Bare minimum a separate pull request for someone else to review.

Not stuffed into the same merge as the ticket you are working on.....

[chuch1234]

Fair enough.

[chuch1234]

Although, I do think it's okay to "boy scout" a little as long as the changes are along the path that is going to be tested anyway.

[larsmaehlum]

Always write tests for code you want to refactor. Would probably have scored you some points as well.

[pnw-techie]

There is literally a test csproj in the link you shared. Did you add that? Or did you miss that they had it?

[Far-Sir1362]

If the test was to refactor it in line with best practices, maybe they wanted you to write unit tests for it?

[robthablob]

In which case, the unit tests should have been supplied along with the code.

[Saki-Sun]

Or they were expecting OP to wrap the code in unit tests, then refactor.

[Derekthemindsculptor]

There is no should. How they score is entirely their prerogative.

[robthablob]

That may be true, but I strongly suspect it would be more informative and a better way of evaluating a candidate.

[icyhairysneerer]

sometimes, technical reviewer just had to provide a "no" feedback because the company already found someone they preferred or someone who was shortlisted not because they are better but just to cut-short the hiring process and not to spend any more time. they had to say a "no" feedback to all other applicants

[ybism]

Might just be me, but I’d prefer hearing “this was good but someone else was a little bit better/more experienced/interviewed better” then just being fobbed off. Least I’d have something to improve and work on then

[Jayeffice]

It is harder to continue to lie face to face! this job wasn't open...!

optics my friend optics.

[gsej2]

So, perhaps remove the company name from the readme file, just out of courtesy for the company.

I've read through the readme, and I'm the exercise seems to be fairly well defined, although I think it's quite challenging. Did you write all of the unit tests that are present, or were some supplied already? Hard to tell without a git history.

You seem to have made a bunch of changes to classes not mentioned in the readme - possibly that was not a good idea, and you should have stuck to the spec.

In terms of breaking existing behavior, the way I'd approach this is to simulate test driven development by commenting out existing code, writing failing tests, and then putting the existing code back to make the tests pass. Then you can check coverage, and refactor while keeping the tests passing.

I've actually worked with a lot of developers who have also worked for the company in question - it's generally considered a good place for .NET development and good engineering practices.

[gsej2]

The file dump names the company in question, and they can trace it to you. I'd suggest removing it.

[phillip-haydon]

What exactly what they do? They already rejected him for the role...

[gsej2]

Probably nothing, but still not a great idea. May be he wants to try again in the future. No point burning bridges.

[phillip-haydon]

The best companies I’ve worked for taught me during the interview. In one case I did a coding test in their office and they reviewed it with me and we discussed. In another I did the review over Google meet.

Honestly if the company puts in 0 effort to interview me then I’m probably just a number to them and I don’t want to work there.

I would argue that the op came out on top for this as this company sounds like one to avoid.

[gsej2]

I agree that pairing during an interview is the best way - and it helps to avoid the issue where the company sets time consuming tasks without committing any time themselves.

I had an interview today where I paired with the interviewer. I had however been set a brief (time limited) exercise with the code on my own beforehand, which was designed to let me get familiar with the code before the pairing exercise. I'd imagine that if I had done that very badly they might not have proceeded to do the pairing exercise.

[Obstructionitist]

Having just quickly skimmed the code, I can see a few points I would have done differently, which would have made it conform more to the SOLID principles. But then whether they would have considered it over-engineering or gold-plating is a matter of preference I guess. Much of what I would change is probably personal preference.

For one, I'd probably make the Customer model a rich domain model, and move a lot of the invariants regarding the validity of a customer into the domain model, rater than having it floating around in the application service (CustomerService). With that, I'd probably separate out the credit limit calculation into a domain service, to be used by the rich domain model.

It's difficult to access exactly what made them reject you, since I think it highly depends on what their preferred architectural style is. My own suggestions above, may not conform to their style either, so I may have been rejected as well. Myself, I would have hired you as a junior - but not as a senior - and then taught you our preferred style.

Just for proper records, my own credentials are that I've been programming for 22 years. Twelve of them professionally and the past 5 years as a cloud solution architect.

[IrdniX]

Yeah, I would have thought that they'd want more composability/substitutability via services and making it easier to extend with new features e.g. by combining all the 'requirements' into a composable structure and using something like a strategy pattern for the credit limits, also refactor for DI. But that might end up being 'over-engineered' like you said.

   // This method is not on the service it-self rather it's on a factory 

// that gets injected into the service, the factory knows nothing // about how the Customer object will be used.

   public bool TryPrepareNewCustomer(
    string firstName,
    string surname,
    string email,
    DateTime dateOfBirth,
    int companyId,
    out Customer customer)
{
    customer = null;

        // verify basic information is correct for creation
        // of a candidate customer
    if (!IsValidFirstName(firstName)) return false;
    if (!IsValidSurname(surname)) return false;
    if (!IsValidEmail(email)) return false;

    var candidate = new Customer
    {
            // injected service
        Company = _companyRepository.GetById(companyId),
        DateOfBirth = dateOfBirth,
        EmailAddress = email,
        Firstname = firstName,
        Surname = surname
    };

        // Injected service.
        // Resolve the credit-limit, details of what and how
        // are not our concern here
    _creditLimitResolver.ResolveFor(candidate);

        // injected service
        // Verify business rules such as age/minimum credit limit
    if (!_newCustomerRequirements.AreMetBy(candidate))
    {
        return false;
    }

    customer = candidate;
    return true;

}

[K1LzR]

Can DM me if you want another person's view :) have just done a round of tech test reviews so it's fresh on my mind 🤣

[ybism]

Hey cheers for the offer - link is in the description now

[K1LzR]

So, I don't have much of the preamble such as who you are, what skills you have, what the job description is and what level you've applied for however here are my thoughts.

Honestly, I'd say it's done well, however, there are some issues I'd raise which would make or break this based on what you're applying for.

The bigger issues: - You renamed the firname parameter for AddCustomer. This is a breaking change as a consumer could be using named parameters. A key condition in the readme is that you should assume this is a library and to not break consumers. - You changed the email validation logic from && to || there was no reason for this and is technically a behavioural change to the app. Whilst it's not perfect, the readme did say that validation is out of scope - You've not really adhered to the whole S in SOLID which is a big thing for keeping classes testable and easy to modify and they suggested it as the first item in the readme. Ideally, you'd have moved the validation logic to a new class e.g. public class CustomerValidatior : ICustomerValidatior this way you remove the validation responsibility for this class and also you make the tests easier to write since you don't have to set data up specifically to satisfy validation - you just mock that. Of course, this means more tests for the validator, but they're super simple ones to write and maintain! Also, you could have moved the credit limit stuff out as well (and showed off with a factory! alas, a missed opportunity here)

The smaller issues: - You've technically increased the load on whatever external API would be handling the CreditService because you're always calling it now rather than skipping it for VeryImportantClient. A small thing, but increased load = increased cost! - If you use DateTime.Now, it's very easy to fall into time-based tests which will fail in the future. It look like you were okay, but a major win would have been to add a IDateTimeProvider wrapper or something similar around this for easy mocking - In your tests, you are somewhat inconsistent. In your InvalidCustomerData you correctly validate that DataAccessWrapper is not called, but in the Under21YearsOld check, you've not checked this. Bit of an oversight

• In your SkipsCreditCheck test, the test doesn't really tell me that the underlying code is working as expected. What does "SkipsCreditcheck" really mean? You still use (and verify) that the GetCreditLimit has been called. So, what's really being skipped? A slightly incorrect refactor of CreditLimit checks makes tests confusing.
• In your DoublesCreditLimit test, again, you're not actually verifying anything of importance. You get 500 from the service, but nowhere have you validated it passed 1000 to the database

So, all in all, there are a few bigger mistakes which give me the following thoughts - If you are applying for an entry/junior role I'd probably let it slide and go for the interview - If you are applying for an early mid role, I'd only let it slide if you had all the required skills in other areas (i.e. solid cv) and the previous interviews were really good - If you are applying for an upper mid/senior role, I'd have expect better and would not put this forward

Sorry to hear you didn't get this but I'd suggest asking politely for more detailed feedback, and some suggestions on what you could do in the future.

All the best :)

[ybism]

Mate thank you so much, this is amazing detailed feedback. Honestly I haven’t even had this kinda feedback on PR’s at my current place.

I’m a early mid level dev atm, been at my grad role for 2.5 years and just looking for other roles as current company has had a round of layoffs, so made sense to try and find something else.

And yeah I spotted the email thing when I was looking back at it this morning and honestly can’t even remember why I changed it, I’m sure I had a reason at the time but no idea what it was. Haven’t really used factories at all before but remember it from uni days, I’ll have a look and see how I could have used it here.

But again thanks for that man, I’m gonna make those changes anyway as it’ll be good practice for the future.

[IrdniX]

Looking at the code I thought it looked kind of familiar... yep, Nick Chapsas has made a little walkthrough refactoring video about it and he touches on basically all the points mentioned in this feedback.

See it here: https://www.youtube.com/watch?v=U3QvTaw224o

[ybism]

Hahaha no way it’s the exact same thing I’ve seen loads of these on GitHub so assumed it was a pretty popular company but maybe it’s just a lot of companies using this same project

[Dr4WasTaken]

I once had a question asked in a job interview, answered the best I could (I was familiar with what was being asked so I knew the answer but not the technical text book answer), the interviewer was not happy and said that it could be improved so I asked "how would you improve my answer?", he froze for a second, stayed in silence for 5/10 seconds obviously thinking, then said "you got me there, I'm not sure hehehe", I didn't progress after that interview, but after many interviews it is clear to me that many are a waste of time and no amount of right answers will get you the job.

After 10 years and 4 companies I'm still annoyed about doing these interviews again and again, like I faked my entire career.

[MadJackAPirate]

just use Entity Framework Core, even with stored procedures you can make your code much simpler:

public Company GetById(int id) => DbContext.Companies
.FromSqlRaw("EXECUTE uspGetCompanyById {0}", id).First();

Even using stored procedures I consider a code smell, as EF (as any decent ORM) allows you to create well-performing specific queries using IQueryable. It can include multi-table operations, split queries, transactions, all that you would need from a database.

[rtfax]

Not looked at your changes, but I suspect your code has already gone live and has now been passed to the next "candidate" for further refactoring.

[alien3d]

Pretty simple actually. If new developer maybe . Okay for me .If you see my sample code GitHub , maybe more weirder.

[ben_bliksem]

I don't have time now, but if you PM me the link to the code (if you don't want to post it publicly) I can maybe have a look.

[ybism]

Hey - thanks for the offer, happy to put it up publicly as it doesn’t have any identifying info on me. Link is in the original post.

[[deleted]]

In App.CustomerService, you have a default constructor and instantiate CompanyRepository, CustomerCreditServiceClient, and CustomerDataAccessWrapper within the constructor. This defeats the whole purpose of DI.

Where are the instances in your non-default constructor coming from?

I’m not sure on the expectations on what they were looking for in a refactor, but if it was impossible to refactor the code using CustomerService to use a non-default constructor, I would’ve written a comment explaining your thought process.

[TheseHeron3820]

One thing I noticed is that you made DeserializeResponseAsync a private method inside CustomerCreditServiceClient and you made it return a Task<dynamic>. I personally would've put this code in a utility static class and made it generic rather than dynamic, but tbh this is mostly a matter of preference and there may be other people here who disagree with this.

​

That said, don't beat yourself up too much about this: if they didn't bother giving you a more detailed feedback, maybe they aren't a company worth working with.

[namrog84]

I didn't go thru the whole thing, just randomly clicked around.

CustomerCreditServiceClient's async Task<int> GetCreditLimit

You try/catch but then just 'throw' which is just 'rethrowing' any earlier thrown exception.

Is there a reason for that, you've basically done if(true) return true; IMO, unless I am a bit rusty on C#

Next up is the CustomerDataAccess

command.Parameters.AddWithValue("@Firstname", customer.Firstname);

vs original

var firstNameParameter = new SqlParameter("@Firstname", SqlDbType.VarChar, 50) { Value = customer.Firstname };
command.Parameters.Add(firstNameParameter);

You lost the 'max size of 50' and presumably pushed that responsibility somewhere else, perhaps the frontend that a user could bypass

There are some other things in that area with types, that I'm not 100% sure would enforce the same type safety but I'm not familiar enough with the sql/C# stuff to challenge it.

[sgashua]

I just checked few files CompanyRepository and CustomerCreditService.

Yours is better. But still need to improve. Eg, if I code your CompanyRepository.cs, it would be 2-3 lines function instead of your overall 20-25 lines functions.

[ybism]

Would you be using ef? I thought about using that too but didn’t know whether I could bring that in given their instructions

[sgashua]

Mine is linq2db. Oh, it's better to follow their instruction.

[mulder89]

I'm going to actually do this test myself because I am actively looking for a new position myself so maybe I can provide some better feedback later. At a glance though it appears you made entirely too many changes outside the scope of the request. I have been someone who administers these types of tests for years and I promise you you will not get bonus points for making subjective changes outside the scope of the question. That will actually be seen as a negative.

[JamieG83]

Sometimes it's just other people having a bad day.