Code Review - Discussion about help vs. review

9:33 AM

@rolfl In my Minesweeper Flags question I got an answer that one of my classes were doing too much and should be separated in two, isn't that "Reviewing design"? (yes, there was implementation in the code as well)

Note that my meta question actually says:

> 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.

Let's say that I primarily want the design of my (implemented) code to be reviewed, if I don't ask about it then answers can comment about it, but if I do ask about it then my question is off-topic? That seems completely illogical. — Simon André Forsberg 17 secs ago

1:40 PM

@Simon - I got some time ... try to make myself clear ...?

@rolfl Hey, I don't have that much time right now. I gotta make some food to eat. I'll be back later to discuss it though

No problem./

enjoy!

2:17 PM

@SimonAndréForsberg - added Summary and Conclusion sections. — rolfl ♦ 18 secs ago

3:52 PM

Design Only Question (DOQ) - Agreed. Implementation Only Question (IOQ) - Agreed. Design First Question (DFQ) - Do not agree. I don't see how you would answer that differently from the IOQ. In the IOQ you manage to somehow review the design while reviewing the code. Why should that not be possible in the DFQ? — Simon André Forsberg 2 mins ago

@rolfl ^^

@SimonAndréForsberg because DFQ is meaningless, when there's no I...

especially given I requires D

what makes a DFQ different from a DOQ?

@Vogel612 DFQ contains implementation

F = First, not Only

well if you have a DFQ with I, what makes it different from a IOQ?

or rather what makes an IOQ??

The code in the DFQ and IOQ is identical. It's only the "plain-english" that is different

--> how can there be an I without a D?

3:55 PM

@Vogel612 My point exactly!!

stop...

> In the IOQ you manage to somehow review the design while reviewing the code. Why should that not be possible in the DFQ?

you seem to try to pry apart Questions with Implementation from questions with implementation

I is a superset of D

> Questions with Implementation from questions with implementation

--> IOQ is automatically DFQ

> DFQ contains implementation

3:57 PM

I don't really know what in @rolfl's mind separates an DFQ from an IOQ

DFQ without I is just DOQ

and DFQ with I is IOQ

so there is no DFQ, but just a DOQ with missing I

Considering interface-only questions as off-topic, that I can accept (even though I don't think all such questions are bad and therefore don't want to make a rule for it), but considering a DFQ as off-topic just because of the plain-english part, that I totally don't agree with.

@SimonAndréForsberg it's not about the plain-english part..

Simon, my issue is with the value of the review.

it's about the code in the question

4:00 PM

what is the value of doing a review on code that does not do the intended job?

@rolfl "On Code Review it is acceptable to review all aspects of the code"

Wait a minute...

Correct, but we also require that the code is working.

which is why you can review D on any IOQ

who said that the code did not do it's intended job?

well it's these two options...

4:02 PM

If you don't know what the design is, then you don't know if it is working.

what part of the DFQ doesn't do it's intended job?

The design tells you what the code is supposed to accomplish

A DFQ is saying: I don't know what my code is supposed to do, is it doing it?

▲ that

@rolfl I disagree with that definition

If the design is open for debate, then the code is not ready for review.

4:04 PM

I think what we need to figure out is What do we mean by design? I don't think we have the same definition there.

@SimonAndréForsberg what is Design then?

@rolfl But according to you if you don't ask about the design, then it's open for debate.

If you do ask for a review of the design, then that should be reviewed before the code is reviewed.

Citation:

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

@rolfl Why?

@SimonAndréForsberg right. because code written without a "finished" design is not finished, and thus by definition "non-working"

4:06 PM

You can ask: does my code implement the design?. you cannot ask, is my design good?

or rather "asking about code not yet written"

To me, design is about the method signatures and class definitions. I.e. "you should put this method in another class", that's a design-aspect of the code.

I would substantially agree with that.

@SimonAndréForsberg Okay so someone comes here and has a Class with some methods and says:

> I am not sure about whether all these methods belong into that class.

off- or on-topic?

or rather: code not finished yet, or code "finished"?

@Vogel612 Where there is an implementation there is a design. It might not be entirely set in stone, but I would say it is "finished" and ready for review.

4:08 PM

@SimonAndréForsberg now I come here and challenge that design

@Vogel612 on-topic.

But if I got the answer "your design is okay", would I accept it?

Would it be a useful answer to me?

and exactly that is what rolfl doubts.

Simon, relating it back to the interface question. A person asks:
- is this a good interface for a tic-tac-toe AI?

@Vogel612 If it states why it is okay then I think it is useful

That is asking for a design review.

4:10 PM

Reviewing good aspects of the code is just as important as reviewing bad aspects of the code.

If he is saying: I have an interface for an AI for tic-tac-toe, is it neat and tidy? That is a code-review.

@SimonAndréForsberg while that is true, how would my question be different if I removed the implementation?

@rolfl Fine, I accept that that one can be considered as off-topic. I don't see how it relates to the DFQ though.

your call, counterexample..

Well, it relates to DFQ because a DFQ is saying: is this a good interface for a tic-tac-toe AI? and then, ALSO, it is saying: Is my code neat and tidy?

4:12 PM

@Vogel612 It would be harder to review your design. Seeing the implementation gives more context.

The first part of the question is off-topic.

@SimonAndréForsberg no it would not be harder, it would be impossible...

because all answers would be equally valid.

@rolfl Not when there is existing code that implements the interface.

same with one example from earlier, the EulerChallenceSolver

@SimonAndréForsberg but the it's no more a design review.

21 secs ago, by Simon André Forsberg

@rolfl Not when there is existing code that implements the interface.

which interface?

The one that is presented, or the one which you recommend he uses?

4:13 PM

1 min ago, by rolfl

Well, it relates to DFQ because a DFQ is saying: is this a good interface for a tic-tac-toe AI? and then, ALSO, it is saying: Is my code neat and tidy?

or the one that someone else recommends?

or the one that someone else recommends 6 months later?

I am not saying that you can't gove a decent review about code for which the design is not settled.

> is this a good interface for a tic-tac-toe AI?

Why all answers to interface-only questions are valid

23 hours ago, 4 minutes total – 13 messages, 3 users, 3 stars

Bookmarked 14 secs ago by Vogel612

what I am saying, is that Code Review will not work well if the design is not fixed before the review.

And what do you mean by "fixed"?

4:17 PM

fixed in this context is 'stable', un-changning, settled.

it is OK in a review to say the code could be improved if the design was different.

If you mean "un-changing", then how can reviewing design be on-topic if you don't ask for a design review?

From a reviewer's point of view, no code that is put up for review is "fixed"

OK, let me revise that one sentence that you originally quoted as being silly....

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

that should be:

> challenging the design is only on-topic when the question does not ask for a design review!

if the question says 'help me design this code?' then they need to go somewhere else first.

then come back with the code that implements the revised/clarified/'stable' design.

Code Review is not in the 'business' of helping people design their applications.

we are in the business of helping to ensure their code meets their design, or where the design is a hinderance for their code.

id they do not know what their design is yet, then they do not know if their code is doing things right yet either.

If there was a Design Review SE site, we would migrate the questions to there, just like we migrate broken code to SO (even though we could review the broken code too).

in the absense of a Design Review site, our only option is to close as off-topic.

4:35 PM

4

A: Reviewing "design"

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 ...

Added 'AppDesign.se' section. — rolfl ♦ 17 secs ago

@rolfl You presume that just because someone asks about their design means that the design is unfinished. I disagree with that.

If they are not sure their design is right, is it finished?

(to the best of their knowledge) ....

Is any code ever finished?

Maybe thinking of a poor design as a 'bug that was not there to the best of their knowledge'.

@rolfl As long as their code does what it is supposed to do, I don't see why it shouldn't be considered as finished.

4:38 PM

If they say "My code works", and you find an obscure bug, the question (and answer) are on topic.

@SimonAndréForsberg yes. sometimes there's code that's finished.

if they say "I don't know if my code works", then the question is off-topic.

I am saying that, if they don't know if their design is right, then they don't KNOW that their code is right either.

Code can work even if the design is bad.

true.

@rolfl I disagree with that too.

4:40 PM

@SimonAndréForsberg give a counterexample.

If you know that your design is not right, you're off-topic. you agree on that?

@Vogel612 Define "not right" design. How can you tell that your design is not right if your code is working?

define "working"

or rather, if you know your design is wrong why do you even have code?

Code Review makes assumptions about people asking questions, and their code.

one of the assumptions is that people know what they want their code to do.

the other is that, to the best of their knowledge, the code works at that task.

when those conditions are not met, the question is off topic.

I am saying that, if the asker does not know what their design should be, then they cannot know what their code should do.

(to the best of their knowledge)

Now, when they are wrong, then a good code review answer will point out issues/bugs with both their code, and their design, but, the assumptions have to be valid first, before the question is on-topic.

What you are saying, Simon, is that even though they do not know what their code is supposed to do, that they can still know (to the best of their knowledge), that their code is accomplishing the goals they don't even know (to the best of their knowledge) that they have.

@Vogel612 working = produces the expected results

reminds me of that guy, that somehow got his code to produce the expected results..

something about multidimensional arrays and padding stuff up with strings.

4:47 PM

RFC 1149.5 specifies 4 as the standard IEEE-vetted random number.

well the result was code that produced what he expected, but it was nowhere near any "working" state

either way IMa head home now.

cheers

@rolfl Again, I have to disagree. I consider the design to be more about how the code is supposed to do it than what it supposed to do. For example, if I know my code should calculate probabilities in Minesweeper, what methods do I need for that? That's where the design comes in.

@Vogel612 yes, I remember that one. That's really a grey area of CR. Don't remember how that story ended.

OK, let me stew on this some more, put myself on the other side for a bit.

5:10 PM

Alright, just ping me when you need me

5:23 PM

I edited the very last point in the conclusion, @Simon

I'm currently trying to think of how things will work if DFQ's are allowed.

How closely does the code have to follow the design, etc.

is the design even relevant?

If a person asks a question and says "I cannot change this interface", is my code/implementation OK....?

Is that different to someone saying "I can change the interface?

sure it is..

is that different to someong saying "I want to change the interface?"

I'd say here applies the external constraints rule.

and that again is a design question IMO.

the first is like "I get data from a 3rd party in format xy, and I need to..."

the second is like "I got some data, and I need to ..."

and if the person says "I want to change the interface", is it OK to say "how could the design be better", or even "can you help me design the interface?"

and the last is like "I need to ... , how would I organize my data"

5:33 PM

@Simon - if I were to say that I differentiate between two types of design components: the "external constraints, assumptions, and interfaces", the "implementation structure and implementation details"? Then I say that the first part is off topic, and the second part is implied-on-topic?

Saying "I don't know whether to use an Integer[] or List<Integer> is very different to saying `I don't know how to specify the interface for my AI bot for Tic-Tac-Toe'

5:50 PM

@rolfl What's the difference between interfaces and implementation structure?

Saying "I don't know whether/how ..." is also different from "What do you think of how I made it?"

@Simon - taking a step back, and confirming some of the things you have said are what you still think. (this will be an attempt to see if I can find out where the line is).

1. DOQ's are off topic

2. IOQ's are on topic

3. a DFQ is 'undecided'

the only difference between a DOQ and a DFQ is that the DFQ contains code that runs and does what it is supposed to do.

Agreed?

Or, is there a difference between the 'review-the-design' component in a DFQ?

In other words, a quesiton that says: "Review my design please" and one that says "Review my design please, also, review this code".

You are saying that the second question is on-topic, and the "Review my design please" component is suddenly on topic as well because there's now some code.

What is it about that code that makes the "review my design" suddenly on-topic?

I am making the assumption that the design process happens before the implementation though.

6:09 PM

@rolfl s/something/what it is supposed to do/

OK.

@rolfl I'm more interested in the difference between a DFQ and an IOQ question though, as I don't think that it's a big difference at all

@rolfl There's an implementation of the design.

OK, an IOQ has the implied assumption that 'what it is supposed to do' is well understood.

in my mind, a DOQ is deciding 'what it is supposed to do'.

@rolfl A DFQ question can also be well understood in what the code is supposed to do

then why is it asking for a design review?

6:12 PM

@rolfl in my mind, a DOQ is deciding 'what methods it needs to do what it is supposed to do'

@rolfl because it wants to know if the specified methods makes sense in doing it

If the design component is not 'well understood', then CR answers can do things like: you should change your design and use 'C++' instead of Java

if the design is 'flexible'.

if you can't make assumptions about some fundamental things, then everything is too broad.

things like the language, the task, etc. have to be assumed as being correct.

A person asking a COBOL question to calculate sieve of eratosthenes ..... has an implied design.

@rolfl Finally something I can agree with.

an answer challenging that design would be useful, like "why use COBOL"?

but, a question asking "should I use COBOL to do 'Sieve of eratosthenes', and here's a COBOL program that does it, please review?"

the review of the program will be totally different if the answer to the first question (the design question) is "NO".

So, a COBOL question asking for a review of Sieve-of-eratosthenes is on-topic, based on the asumptions that the design is right, etc.

@rolfl To me, C++ vs. Java is pre-design. It is not design of code but rather design of ... what language to use..

OK, so where is the line?

what parts of the design are 'pre-review' and 'post-review'

It is clear that using sockets, TCP, UDP, HTML, XML, etc. are design decisions.

using C++ or Java, or COBOL is also a design decision, of some sort.

Maybe if I can say, if your program is a 'library', then you need to design the API before your code is ready for Code Review review.

If it is a GUI, you need to decide whether to use Swing, or JavaFX.

Some things have to be 'certain'..... what are those things:

- language

- application (what is the goal you are trying to accomplish)

6:23 PM

@rolfl That's the thing, I'm not sure it is possible to specify one. Whether or not a question is on-topic or not is best determined by looking at the question overall.

@rolfl Isn't there a meta question saying that reviewing APIs is on-topic?

How about looking at what a good answer will be?

If you have a DFQ and the answer focuses only on the design?

Argh, we need to 'define' "design"

@rolfl "answer focuses only on the design" can mean many things

Bingo

well, it's certainly not a code review answer if it reviews only the design (whatever that may be).

That depends on the "whatever that may be" part, doesn't it?

Maybe knowing what it is not , is as valuable as knowing what it is

6:26 PM

I think there are already many well-received code review answers that reviews only the design

well-recieved in that the asker of the question appreciates them, or that they are good for Code Review as a site?

upvoted

And if it is upvoted, how is it bad for CR as a site?

I have no doubt that there are good design reviews on Code Review..... but, are they encouraging people to ask more design-review questions?

do we want to become 'the place to go to for design reviews' ?

@rolfl that depends on what a design review is

we are all smart people, and know a good design, and bad design, and can review designs too, but should we be doing it?

you are saying "yes", and I am saying "no" (at least that's the way I feel things are at, at the moment).

6:31 PM

So I propose: Maybe. Depending on the overall question :)

I think the bug-in-code is a good 'proxy' for this argument.

if we substitute =bug-in-code= for =design=....

A question that has code, and the asker asks: this code produces the wrong results, please review.

that is off-topic, even though the code can be reviewed.

it is off-topic because we want working code, and the question is on-topic on SO.

Yes...

I feel the same about design questions..... it should be questions like:

is my code doing 'this' well.

not 'is my code doing the 'right thing'?

@rolfl That is not how I interpret design questions

OK, so, if you put yourself in my definition for a moment.

are questions "Is my code doing the right thing?"

is that on topic?

6:36 PM

No, that is not on-topic

even though there is code?

An asker should know what their code does

They know what their code does, they wrote it, they just don't know if what their code does is 'good'.

they want an opinion on what their code does, not how it does it.

in my mind, a DOQ is looking for opinions on what the code will do

a DFQ is looking for opinions onf what the code does

and an IOQ is looking for opinions on how the code does it

you have to know, and want the code to do what it does before it is on topic on code review.

we are not here to debate whether the objectives you are trying to accomplish are good, or bad objectives.

we are here to review whether the code accomplishes the 'given' objectives well.

'if the objective is described using an interface, then the interface has to be 'given', not 'debated'.

if the objective is a coding challenge online judge, then the objective is clear, and not open for debate.

@rolfl I agree that what the code does is off-topic (although feature-requests can be embedded in answers and comments), but I don't agree that a DFQ is about what the code does.

I see the DFQ as being both a DOQ and an IOQ in one.

6:46 PM

@rolfl The objective should be described in plain English IMO

You see the DFQ as being something part-way along a continuum

A: Tool for creating CR questions

Well, I don't see how a DFQ can be both DOQ and IOQ when the O stands for Only

Speaking about that feature-request and what the code does... what do you really think about these two answers?

6

It's much more important to convey the code faithfully than to save a few bytes of output by replacing spaces with tabs: line = line.replaceAll(" ", "\t"); // replace four spaces with tabs, since that takes less space In fact, since you used your program to produce this question itself, I s...

6

A: Tool for creating CR questions

I would encourage you to produce more explicit output, particularly with the filenames. If I wanted to reverse the process and scrape the code into files on my machine, using a Python script such as the following… import json from lxml import html import re import requests FILENAME_HINT_XPATH ...

I upvoted them both.

But are they really Code Review answers?

Aren't they talking about what the code does rather than how it does it?

I did find your answer in my follow-up question much better. Even though I did appreciate those feature-requests I got from @200_success

Q: DataLayer Interfaces Genericized

Both answers have both feature suggestions, and implementation reviews.

(whew, my upvotes are OK).

Your question does not ask for a design review whatever that is)

I am not sure that;s a good example question to use for this discussion.

This question (which I also upvoted :( ) is a better discussion point:

12

I have some concerns on my refactoring for the data layer in my current Project. Just for a small info, I am currently building a CRM as "training JEE application". But enough of talk, let's talk code! IService.java: (7 lines, 160 bytes) public interface IService<T> { public void update(T u...

The purpose of the question is to design an interface.

6:57 PM

@rolfl actually (as OP of that question) this wasn't really the point...

There was that project.

and for each service-class, we had a separate interface for nice CDI usage.

and that led to duplication in code and inconsitencies in names.

so I ran this refactoring to "centralize" the interfaces under one naming convention.

I know, but, since the purpose of your application is to present an API, then anything we suggest tha tyou change is also changing the presentation of your project... it is changing what it does.

@rolfl actually it isn't.

because the resulting interfaces still expose the same methods.

your express question: Does this code follow good practices, especially concerning inheritance?

if the inheritance is wrong, the whole API changes.