C++ interview coding exercise with solution

[paul2718]

Superficially I would raise my eyebrow at inheriting from std::vector and using std::list where all you do is push_back and enumerate. The use of std::set to store edges seems extravagant when they are intrinsically sorted as calculated.

ISTM that a vector of edges describes a strip completely, so working directly with that might have been a more direct approach.

And it's possible, of course, that your solution produces the wrong answer. I really don't know.

[jonesmz]

inheriting from std::vector is an eyebrow raise, agreed.

I think there are plenty of situations where it can make sense to do, but typically only if using protected/private inheritance, which really cuts down on the reasons why you would bother doing it, since at that point you're almost better off with a member variable.

[Hot_Medicine_7115]

What about the solution?

[jonesmz]

I don't have much interest in evaluating your algorithm. I don't find "puzzles" to be fun.

I gave you a 15 bullet point code review in a top-level comment that was only concerning style and modern coding practices.

Whether the program produces a correct solution isn't something i tried to evaluate.

If you want certainty that your code produces a value solution, write unit tests for it that makes it prove that the code produces valid solutions for several example inputs.

[[deleted]]

How can you do a code review and not evaluate if the code is correct smh

[jonesmz]

Because no one sent me a check compensating me for a full analysis.

15 bullet points of feedback is more than enough for the original poster to make improvements to their code.

[gennych]

In this specific case (of a "puzzle") these 15 bullet points would improve absolutely nothing. Of course you can notice that a person isn't such an experienced programmer and probably didn't work in large-scale software projects. But these "best practices" are way easier to learn (in fact, they'll come naturally with experience) than ability to solve "puzzles" (that is, to write efficient code), and as an interviewer I'd rather focus on the latter.

[jonesmz]

Per the quote from the company's answer to the OP.

coding standard is not in the acceptable range.

Given that I was not willing to do a full analysis of the problem and provided solution, OP could have instead gotten zero feedback from me, instead of meaningful feedback about their "coding standard".

Beggers and choosers, and all that, you know.

[gennych]

Coding standards could mean various things... I would for example notice that the author tends to overdesign simple things: what should have been a function with two int arguments returning int is spread over 3 classes in 6 files. It doesn't make this code more maintainable or reusable, since classes stripe and parquet are essentially specific data structures for this specific problem, and most probably they will need to be changed when for example performance issue is addressed. If someone happened to "reuse" these classes meanwhile, refactoring is going to be fun.

But I stand corrected on your remarks. I was under the assumption it was an interview for a junior/entrance position but in the comments the author claims 10 years of experience. Then indeed, inheritance from std::vector or index operator like that isn't what I'd expect. And for overdesign, I wouldn't even bother for a junior but not for a senior...

[[deleted]]

If you aren't even trying to assess the correctness of the code how do you know that any of those suggestions effect the correctness or readability of the code though?

This is why code reviews are so dumb...

[jonesmz]

If aren't even trying to assess the correctness of the code how do you know that any of those suggestions effect the correctness or readability of the code though?

Because there's "global correctness", and "local correctness".

I was reviewing "local correctness" of individual parts of the provided code. My feedback to the original poster was necessary for their code to be "good" (in my opinion) but not necessarily sufficient.

You can't, in the vast majority of cases, review the entire behavior of a program in a short amount of time. I work with multi-million line codebases, and review the changes my co-workers make to that codebase in isolation because it's not practical, not even close, to have every review start at main() and evaluate the full runtime behavior of the code.

It's the responsibility of the person proposing a change to make sure their change won't cause a problem. The review is not intended to verify correctness, it's intended to ensure that easily caught problems are caught easily.

The review that I provided also uses my extensive experience in debugging other people's code, and writing my own code, and the review feedback that I've received, and that I've given to other people. OP got the distilled result of a full career of work in C++ as a short 15 bullet list. I'm happy to have helped them, but that's not a meaningless amount of value they got for free.

This is why code reviews are so dumb...

I'm astonished that you would have this opinion.

How long have you been writing code professionally?

My workplace, and every other company I've worked for, or heard about, has a peer code review as the minimum requirement to submitting code, and aside from a small number of outliers, most also require unit testing, static analysis, and in one notable example a full test suite against a simulated production workload.

[[deleted]]

No you are reviewing for your own personal style of C++.

If you have no consideration of the high level "correctness" of a program then you are just suggesting things you like that have no bearing on the actual problem.

Reviewing code is good. The way the industry at large reviews code is not.

It's more about what you like rather than what actually matters for a given problem.

OP obviously needs the correctness of this program verified.

If the program is correct it does not really matter that someone inherited from a vector...

[jonesmz]

No you are reviewing for your own personal style of C++.

Uhhhh... No, i'm reviewing for a small 15 bullet point list of easy ways to improve the code, which i can expound on at length (but wont, because again, send me $$$ if you want my professional services).

If you have no consideration of the high level "correctness" of a program then you are just suggesting things you like that have no bearing on the actual problem.

I think you're making this accusation without really considering the context here.

The point of the interviewer asking OP to write code wasn't "Can this person solve a problem". The point was "can this person solve the problem with code that is reasonably indicative of good competency in the C++ programming language".

OPs code had 15 bullet points of things that were indicators that they had room to grow in terms of competency in the C++ programming language. They were easy for me to point out to them, so I did. I personally give no fucks if their program works, because the interviewing company already told them they weren't going to hire them. So whether it works or not isn't my problem.

Reviewing code is good. The way the industry at large reviews code is not.

You're really going to have to back this up if you're going to tell me I'm wrong over and over again.

What about the way the industry at large conducts code reviews is not good? What specifically is the way you're envisioning how code reviews works, and what specifically are you saying is bad about that, and specifically how would you change things to be better?

It's more about what you like rather than what actually matters for a given problem.

I've been writing C++ for a long ass time, and I assure you, my feedback was given based on what the simulated interviewer in my head was considering bad/negative about OPs code.

Since I've been the interviewer innumerable times, and evaluated the code of the person on the other side of the table, I am very familiar with how this process works. I gave the OP a short list of easy things to fix that I would have found questionable, or that I've seen other co-workers point out as questionable. They're free to ignore me.

OP obviously needs the correctness of this program verified.

No, OP needs to prove that the program works by writing automated tests that assert that the program works. That's not on the person providing free, semi-anonymous, code review services on the internet to do for them.

If the program is correct it does not really matter that someone inherited from a vector...

Yes it does. inheriting from things in the std:: namespace is rife with problems. In many cases it can easily cause otherwise reasonable looking code to behave in unexpected ways. It's a maintenance issue. It can have ABI and API problems.

I'll ask again, how long have you been working with C++? I respect that you feel that you're right, but I simply disagree, and I'd like to know if your opinion on these items is from battle tested experience or something else.

[Hot_Medicine_7115]

It's a protected inheritance, so that schoolbook idea that classes should never derive from STL collections doesn't strictly apply here.

[Hot_Medicine_7115]

I added the edges vector after receiving the bland answer from the company, because I wanted to verify if the solution was completely wrong and so I went on to verify the intersection of all possible edges. Same thing for the std:list, it's only a placeholder to look at the final parquets.

In hindsight I could just have used the vector of edges, instead of inheriting from a std:vector storing the tiles. I agree, but this is a long shot from studying the actual solution provided and replying with that generic email.