Reviewing "design"

Question

It has been said in various comments that on Code Review we review code and not design.

I find this statement not very clear though. Obviously, if someone has an UML and not any written code, that's off-topic on Code Review.

However, if there is a question with a finished implementation, such as a Trading Card-Game or a Sudoku Solver or whatever, then I believe that "reviewing the design" or asking for a "high-level design review" is not off-topic at all. After all, the design is part of the code, is it not?

When, and why, is "reviewing design" on-topic on Code Review and when is it off-topic?

Answer 1

There is one school of thought that says "the code is the design". The reason is that the completed system often bears little or no resemblance to the "design".

So I would say that if the question contains code, and the asker wants the design of that code reviewed, there's absolutely nothing wrong with that, but if the asker wants something that is not code reviewed, it's off topic.

Answer 2

Code Reviews in general (not specifically at Code Review Stack Exchange) will often review the design. The people doing the review also have influence on the design too, so it is natural.

The design itself though, is a very broad, and unsubstantial concept. On Code Review (and Stack Exchange in general), it would be too broad to ask:

I have this design in mind to implement FeatureABC. Is it a good design? Can it be improved?

The reason it is too broad is because ideas are intangible, there are too many right answers, and too many opinions, possibilities, and considerations.

So, on the one hand, pure design reviews are off-topic on Code Review, even though they are often reviewed in real-life code reviews.

This particular meta question asks, though: When, and why, is "reviewing design" on-topic on Code Review and when is it off-topic?

Off-Topic

• It is off-topic when the only thing wanted/demanded in the review is a 'design review'.

Further, the actual implemented code is heavily dependent on the actual design. You cannot change the design without heavily impacting the code. It follows that:

• if the design changes, the code changes, and
• if the design is not 'fixed', then the code is not 'fixed'.

Now, if a question provides a design description, and matching code, then requests: This is my design, is the design OK?, it will be off topic. This is because:

• If the design is still to be reviewed, it is logical that the design is not stable;
• If the design is not stable, then the code is not stable;
• Additionally, if the design is not stable, then the code is off-topic because one of the on-topic requirements is: To the best of my knowledge, does the code work?
• If you don't know the design, then you don't know what the code is supposed to do.
• If you don't know what the code is supposed to do, how can you possibly know that it works?
• The code is not ready for review if the design is not agreed.

Thus:

• Any question requesting a design review is off-topic, either because the code is not there, or because, even if there is code, the code is just an example of what one particular design would/could look like. If the design changes, then the code is irrelevant

So, any question requesting a design review is off-topic.

On-Topic

So, when is reviewing the design on-topic?

When there is real code (which implies a real & stable design), and the code is sub-optimal because of the design!

If the code is having to do awkward, inefficient, or otherwise broken things to make the design work, then it would be on topic to say:

This code would be better if you could restructure the design and do it this way instead.

• design reviews are only on-topic when the question does not ask for one!

(yes, exactly, only when the design review is not requested, is the design review on-topic.... and that is why we are Code Review, and not Design Review).

Using Examples

Let me try to describe what I mean by way of examples:

---

Design Only Question:

I want to build an HTML Parser in LangX. I will use an IO class, an HTML entity class, and a Document class. The IO class will do X, the Entity class will do Y, and the Document class will do Z. Is this design OK?

This is off topic - design questions are too broad, too many answers, and in this case, no code to review.

---

Implementation Only Question:

I have these three classes which together will parse and store an HTML document. The IO class manages getting data from the disk or a URL, the Entity class represents tags and other items in the HTML, and the Document class allows the entities to be contained and related. This is the code for each class: ....

This is on topic - the code is ready for review, and further, if the code could perform or be structured better, then challenging the design, suggesting an improvement (like You should have a parsing class instead of doing the parsing in the document class!), or even recommending a radical restructuring would be positive outcomes from the review.

---

Design First Question

This is the controversial one - the question requests a design review, and also presents code:

I have these three classes which together will parse and store an HTML document. The IO class manages getting data from the disk or a URL, the Entity class represents tags and other items in the HTML, and the Document class allows the entities to be contained and related. This is the code for each class: ....

I don't know the design is good, can the design be improved? ....

My opinion - OFF TOPIC - This question is asking for a code review on code that the asker knows is badly designed. Further, the code will be radically changed if the design changes.

There are two ways this code can be reviewed:

1. assume the design is good, and review the code as if the design is right
2. perform a design review first, and either confirm or challenge the design:
   • confirm - proceed on and do a code review to confirm that the code meets the design
   • challenge - say the design is bad, and thus the code is bad

In case 1, assuming the design is OK, well, that adds no value to the asker because their priority can only be to confirm the design. A code review on a design they don't trust (or want to keep) is useless. Saying "I don't know if your code is doing the right thing, but, whatever it is doing, it is doing it very neatly and performs well!" is also wrong.

In case 2, the question/answer should be split in to two stages. The first question should be: Is my design OK? The Second question should be: "Does this code implement the given design?"

Since part 1 is off topic on Code Review, and part 2 is meaningless until part 1 is decided, then the whole question is off-topic.

---

AppDesign.se

Putting it a different way again: Stack Exchange has the following sites that are loosely related:

1. Programmers.se - for when you want to discuss general programming concepts and methodologies
2. AppDesign.se - for when you want help designing what your application should do, how the parts should fit together, and deciding what the code should do in various conditions.
3. StackOverflow.se - for when you know what you want, and your program is not quite working yet, and you have a really good idea of where it is going wrong, but not why.
4. CodeReview.se - for when everything is working as intended, but you think the implementation could be improved.

In the system above, questions related to programming concepts will be migrated to Programmers. Questions related to design will be migrated to AppDesign, questions related to programming bugs and issues will be migrated to Stack Overflow, and questions about improving working code will be migrated to Code Review.

That's a really pretty picture, except there's one problem: There is no AppDesign.se site.

Just because there is no AppDesign site, does not mean that design questions are on-topic on Code Review.

Further, a working design always precedes working code. If you don't know that your design works, then you don't know if your code works.

In the absence of AppDesign.se, the only option is to close design-related questions as off-topic.

---

Conclusion

• Code Review is not the right place to perform "design only" reviews.
• If the system is still at the stage of design review, then any code is not yet ready for a code review (you have to know what the code is supposed to do before you can tell if it is doing it right).
• when the design is 'finalized' then the code that implements that design is ready for review
• if the design forces the code to be inefficient or 'ugly', then that should/could be brought to the attention of the asker.

Answer 3

When it comes to "General Principles of Programming Design" that should be left to the Programmers site. But what you are talking about is something totally different.

The "overall" design of an application is something that we can review (and have reviewed).

in terms of Design:

• Programmers is about "how should I model the code for my application?" (Before)

• Code Review is about "is this design implemented correctly?" (After)

---

If you remember the string of questions that start with this one I think.

Calculate balance wages

Answer 4

If someone posts some code and explains it and the problem then they are asking for a code review.

If it so happens that they have made a design mistake and said design mistake can be seen in their explanation then it wouldn't be off topic to address this. Same with edge cases that the code might not address.

So then if they include a UML diagram (or other such design tool) with their code then it also would be on topic. I would equate the design with an explanation of the originally devised solution. (see the Codeless Code 'Case 105: Navigation')

Therefore

A question without code and only explanation is off topic whether the explanation comes in the form of text only or text and some design.

An answer without review of the code would similarly be off topic/un-helpful.