5

This is a 100% theoretical question, and maybe opinion based.

In a professional interview I got a printed page with a lot of awfully written and improperly formatted code of two classes to analyze them line by line in speech. Let's call these data structures A and B. There was no problem statement or any info about the expected behavior.

However one of their questions really pissed me off.

After identifying the suspected behavior of the algorithm and many potential and actual errors, they asked me to identify one more error. I identified several other obvious problems, but I didn't figure out what they want from me. Well, when they told me the answer it blew my mind. The simplified version of A and B is the following (I left only the methods which are part of their idea):

template <typename T>
class A
{
public:

    // elements are dynamically allocated on the heap

    const T& getValue(unsigned i) const // I'm not even sure it was const
    {
        // return element at position 'i'
    }

    virtual void setValue(unsigned i, const T &value)
    {
        // sets the element at position 'i'
    }
};

template <typename T>
class B: public A<T>
{
public:

    virtual void setValue(unsigned i, const T &value)
    {
        // set the element at position 'i' using A<T>::setValue()
        // then sort all elements in descending order
    }
};

Well, the solution is not a compilation, runtime, or logical error. Not even the potential risk of these. The error is the following: since B::setValue sorts the data after you place an element, you cannot test whether the data you access at a given position is the same what you stored at that given position. However with A you can do this. This unmatched behavior between A and B is considered as an error (and they added that it is not an error, but yet it is an error).

I think it's fully a design choice (since the whole point of B is to maintain sorted data) and I wouldn't say this is an error, especially without a problem statement or any information on the expected behavior. It doesn't even violate the idea or concept of polymorphism.

Would you identify it as an error?

plasmacel
  • 8,183
  • 7
  • 53
  • 101
  • why would anybody want to test if an element is still at the same position after sorting? – 463035818_is_not_an_ai Jan 17 '17 at 16:04
  • `A::setValue(1, 5)` has type `void`, for which there is no `==` expression. – Kerrek SB Jan 17 '17 at 16:04
  • I think they meant that it cannot be tested outside of the class (since they didn't provide any helper method to do that). – plasmacel Jan 17 '17 at 16:04
  • 5
    It seems to me like the override of `setValue` causes B's semantics to differ greatly from A's. Since the expected behavior of `B::setValue` is clearly indicated, I would say that it's likely an error for `B` to inherit publicly from `A`. Perhaps it should inherit privately and implement it's own getter. – François Andrieux Jan 17 '17 at 16:05
  • 1
    May be better suited for codereview? But I kinda agree - the interface should be self-explanatory about expected behavior. Sorting inside `setValue` is pushing it... – rustyx Jan 17 '17 at 16:05
  • @KerrekSB You are right, I expressed the idea here in a wrong way. They meant that if you a value at index `i`, they you get the same value at that index. – plasmacel Jan 17 '17 at 16:06
  • 6
    I would have kindly quit the interview at "I got a printed page with a lot of awfully written and improperly formatted code" point. – roalz Jan 17 '17 at 16:07
  • Think of the service they did to you by making it clear you don't want to work there, but honestly, Stack Overlow is **not** *post your rant* type of website. – SergeyA Jan 17 '17 at 16:11
  • I think this is not that self-explanatory. It depends on the desired usage, which is unknown. There are cases, where this behavior is desired. – plasmacel Jan 17 '17 at 16:11
  • @SergeyA You are completely right, however I wanted to hear (read) more point of views. – plasmacel Jan 17 '17 at 16:13
  • 4
    Actually, the answer that your interviewers wanted is the first thing that caught my eyes: If I have a container, and I can call `setValue(index, value)` on it, it is one of the most evil things that container can do to not subsequently yield `getValue(index) == value`. It means that the design decision of inheriting the ordered container from the unordered container is all wrong in the first place. That is the logical error that you should have identified. Compiler errors & such are just irrelevant compared to this. – cmaster - reinstate monica Jan 17 '17 at 16:33
  • @cmaster What about a priority queue? I think this behavior shouldn't be assumed. It is pretty unlikely that you would derive a priority queue from a regular array, but it is possible. If you only need an array of data, and some other common operations, then this can be fine. On the other hand there are many types of errors and to be honest searching for a design issue was the last thing I can imagine given that code. – plasmacel Jan 17 '17 at 16:44
  • A priority queue would not allow insertion at an arbitrarily user-defined position, it would allow insertion with a given *priority*. That is something that's radically different from a container with array semantics, which class `A` suggests. – cmaster - reinstate monica Jan 17 '17 at 16:57
  • Did you get an offer on the job? Did you accept the job? *(I would not have)* – abelenky Jan 17 '17 at 17:00
  • @cmaster Actually, class `B` also doesn't allow the insertion at a given index, since the data is sorted immediately. If we go this far, then the whole concept of `B` is pointless and shouldn't exist. Why would you want to insert an element at a given index if you sort it immediately? – plasmacel Jan 17 '17 at 17:04
  • 2
    @plasmacel exactly. Assuming competency, your interviewers would have regarded such observation highly. Although, instead of not existing, `B` should probably be something that isn't related to `A`. – eerorika Jan 17 '17 at 17:09
  • @user2079303 You speak my mind :-) – cmaster - reinstate monica Jan 17 '17 at 17:11

1 Answers1

9

We could decide that it is part of the interface of the class A and more specifically a post condition of A::getValue, that A::getValue must return the same value that was (immediately) previously assigned by A::setValue into the same index.

If such post condition is specified, then a derived class that violates the post condition would violate the Liskov Substitution Principle. It would be a design error.

It was not specified in the exercise whether such post condition exists, or should exist though. And without the post condition, there is no design error (at least not the error that I'm discussing).

Whether the (presumably assumed by your interviewers) post condition should exist is up to the designer of the hierarchy. Which in your interview was you. Whether the post condition is good design for the hierarchy would depend on what the classes are modeling. If A models something like std::array - and that is what it resembles - then the post condition is definitely a good design choice in my opinion.

Community
  • 1
  • 1
eerorika
  • 232,697
  • 12
  • 197
  • 326
  • If an interface violates a reasonable expectation of a reader without even giving due warning about dangerous behavior in suitably placed comment, that is definitely an error. It's something you cannot tolerate in a codebase, because it will necessarily lead to further errors down the road. Of course, if it's legacy code that you cannot cleanup yourself right away, it's permissible to just put out a "Here be dragons!" sign, but that does not make the error less erroneous. – cmaster - reinstate monica Jan 17 '17 at 16:41