Code Review - Discussion about help vs. review

Vogel612

19:00

so if I had written my question differently, like: Well I had this duplication in my interfaces, and ...

rolfl

Now, what I am saying is that the people on code review are smart, and can answer your question, and in fact, they have answered your question.

Vogel612

while that is true, isn't it somewhat beside the point?

the smart people on CR can and have reviewed broken code.

rolfl

but, your question really boils down to: is my API design good, and here's some code that implements it.

Vogel612

does it?

or rather, would it be if the plain-english part wasn't there?

rolfl

Simply by asking whether your inheritance is right, you are suggesting it could be wrong.

to the best of your knowledge, is your API design right? NO!

Vogel612

19:02

While correct, that would imply that my refactoring was wrong.

and not the API design in itself.

because the API was there before it, or not?

Simon André Forsberg

@rolfl In my mind, what it does and the methods it makes available is not necessarily the same thing.

Vogel612

Think of this question as: How is my "implementation (in interfaces)" of the given API design?

rolfl

@SimonAndréForsberg ... that's one of the grey areas.

Simon André Forsberg

@rolfl But if you ask about if it is right doesn't mean that the answer to "To the best of your knowledge, is your API design right?" is necessarily "No". I'd say that the answer is: "I don't know, that's why I am asking"

rolfl

if you don't know, then you want a design review to be sure.

Simon André Forsberg

19:05

Exactly. And, as I've already written a design of it, I think it's on-topic.

rolfl

if you were to split the question in to two parts, a design review, and a code review, then the design review would be off-topic.

now it is a question of timing.

Simon André Forsberg

If you were to split the question....

rolfl

the code review is based on an uncertain design until the design review is complete.

if the design review comes back and says the design is crap, then what's the point of the code review?

that's the way I feel about bugs.

the code review of a known-buggy program is pointless until the known-bugs are squashed.

Vogel612

@rolfl is it?

rolfl

for the purposes of Code Review SE, yes.

it is explicitly off-topic.

just because we can review buggy code, does not mean that we should!

just because we can review code that is based on a (possibly/probably) changing design, does not mean that we should

if we say we can review code without having a well defined design, then all we are left with reviewing is the style and syntax, because who cares about the functionality?

that could be changing any time!!!!

Simon André Forsberg

19:10

@Vogel612 But we (usually) have not suggested how to actually fix the problem itself. When we review broken code, we still do it in a CR type of way. Use arrays. Make an enum. private fields. Use final. etc.

@rolfl If design = what the code does, then I agree. If design = method interfaces, then I don't agree.

Vogel612

@SimonAndréForsberg of course, but with interfaces that's kinda ... inapplicable isn't it?

@SimonAndréForsberg how is: "what the code does" different from "method interfaces" ?

rolfl

If the purpose of the program is to implement a specific interface, then method-interfaces are the 'important' design.

If the purpose of the program is to solve Euler 1, then that is the 'important' design.

The interfaces used are implementation details.

Maybe the word to use is 'behaviour'.

You need to know how your program should behave before you come to Code Review.

If your program is simply an interface, then that is the program's behaviour, and you need to know what that should be before asking for a review.

Vogel612

btw. a DFQ for you to break your heads over.

2

Q: Building a better Collection. Enumerable in VBA

VBA's 'Collection' is.... lacking, so, I've been working on a better Collection object that implements many of the features of C#'s Enumerable. This is very much inspired by this question and a follow up to Sorting a Collection and More imitation of Enumerable. I'm concerned that the API is inco...

rolfl

That is a great example.

How should my program behave?

Vogel612

just asked, upvoted by me and mat from what I see.

rolfl

19:17

here's some code!

Vogel612

hmm.

no It's clear how the program should behave.

rolfl

Really?

Vogel612

> I've been working on a better Collection object that implements many of the features of C#'s Enumerable.

that's how the code should behave

rolfl

I started out writing everything to be static, but the calls felt weird. To clarify, it's the difference between this

Dim c as New Enumerable
Set c = Enumerable.Intersect(collection1,collection2)

and

Set c = collection1.Intersect(collection2)

I opted for the latter wherever it made sense to, (made sense to me) but it makes things inconsitent, because of calls like this.

How should it behave?

Vogel612

I opted for the latter wherever it made sense to,

rolfl

19:21

> I'm concerned that the API is inconsistent. Some methods like 'Range' and 'Repeat' are meant to be called from a "static" default global instance, while others are to be called on instanced.. instances of 'Enumerable'.

Vogel612

Does my code do what I want in a nice way?

rolfl

he want's his design validated, and that should happen before it is implemented.

Vogel612

well It does what I want, but it feels like my ass looks fat now

rolfl

No, the purpose of his code is to expose an API.

Vogel612

nono, after the third block of code it's all hard implementation

rolfl

19:22

Who cares about the hard implementation if the design can change?

Vogel612

But if the implementation already does what I want, but it feels clumsy to use the interface

rolfl

More importantly, what's the point of revieing the implementation if the implementation implements the wrong API?

That question is 'classic' is my program doing the right thing, and, if it is, is it doing it well?

Vogel612

It's not like it's that much of a refactoring to extract methods (and their headers) to a separate class.

rolfl

let's do that one thing at a time:

the design review is off topic. When you have that settled, come back, and we will tell you if you implement it well.

Vogel612

I think this is the same as: I got code xy, and it does what I want, but I think my ass looks fat.

ckuhn203

19:25

@Vogel612 Yes Vogel. That.

Simon André Forsberg

@rolfl There's probably going to be a lot of same code from now to later. And even if it's not, CR is a lot about teaching people to code better. If you say "use private final on your fields" then hopefully the OP can apply that to their code in the future

ckuhn203

I haven't had a chance to catch up yet.

Vogel612

@ckuhn203 take your time.

rolfl

If that was a project euler question, it would be "I don't know if I should do a sieve first, or check each number for primeness individually", by the way, here's a version that does it using individual primes.

Vogel612

I don't think so,.

It's more like "Here's an implementation the check for the primes with a sieve and an individual check, and it feels clumsy"

ckuhn203

19:26

So, what's the issue with my question exactly?

Vogel612

@ckuhn203 rolfl says it's off-topic, because It's asking for a design review

ckuhn203

I'm not understanding all of the talk.

rolfl

@ckuhn203 - can I bombard you with some questions?

they may not make much sense until later.

ckuhn203

Yeah. Sure.

rolfl

Is the code you have there for a 'library' to emulate funcitonality in .net?

ckuhn203

19:28

Yes.

rolfl

Are you implementinging a range operator in there?

ckuhn203

It is library code. A utility.

Yes.

rolfl

are you implementing an intersect operator for collections?

Will your code expose it as result = Enumerable.intersect(setA, setB) or as result = setA.intersect(setB)?

ckuhn203

As it is, it calls result = setA.intersect(setB)

rolfl

Why 'As it is'?

ckuhn203

19:30

Because I'm not sure it's a friendly, understandable way for it to be called.

rolfl

Thanks.

In my mind, that is a 'behaviour' review that @ckuhn203 s requesting... ;-)

ckuhn203

I wonder if Enumerable.intersect(setA,setB) would be better.

Vogel612

okay, your win

off-topic.

rolfl

when that is 'decided', only then is the code ready for a 'Code Review.se' Code Review.

ckuhn203

Ok. Now clue me in, because you're saying my question is off topic.

Vogel612

19:32

@rolfl if we now assume "as it is" as "I decided"

then It'd be on-topic?

rolfl

And then, at that time, we can say" you know, if you change the API to do Enumerable.intersection(setA, setB) then you could do X, Y, and Z`

and that would be a 'challenge' to the 'design'.

Vogel612

Why can't we say that now?

ckuhn203

That's what I'm wondering.

Vogel612

isn't it implemented like that right now?

ckuhn203

Isn't it better to ask that question when I'm unsure of it? It's already implemented, and I'm unsure if it's best.

rolfl

19:33

yes, but you don't know if your code is implementing the design right, if you don't know what the design is!

ckuhn203

Yes I do.

I know exactly what the design is.

Vogel612

but you are not sure whether the design is right

ckuhn203

I'm asking if it's right. Ok. Got it.

Vogel612

and then we have a design review.

and that's off-topic

rolfl

@ckuhn203 If your whole question was just: should I implement my API as either Enumerable.intersect(setA, SetB) or should I do it as setA.intersect(setB)" then your question would be off-topic.

Vogel612

19:34

btw. for the example at hand: why not expose both?

ckuhn203

So let's go deeper. What exactly about "design" review is off topic to begin with.

Vogel612

every answer to a design review is equally valid.

they are too broad by nature

ckuhn203

@Vogel612 Using optional parameters made it even clunkier.

Vogel612

Why all answers to interface-only questions are valid

yesterday, 4 minutes total – 13 messages, 3 users, 3 stars

Bookmarked 3 hours ago by Vogel612

@ckuhn203 damn... no overloads..

rolfl

It's the difference between "does my code have a fat ass", and "what is a thin ass"

(or, if you like fat asses, "what is a fat ass"?) ;-)

ckuhn203

19:36

That bookmark is awful contrived.

Vogel612

@ckuhn203 sure it is, but we're kinda talking extremes here ;)

ckuhn203

So, if you're making that argument, what kind of API question would be on topic, because from where I stand all of them would be off topic.

So, we're eliminating a whole subset of code and otherwise appropriate questions.

rolfl

I think this may be a good place to read for 'my' take on it:

1 hour ago, by rolfl

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

If you know what your code is supposed to do, then your code is on topic.

ckuhn203

That's exactly what I'm asking.

I know what my code is supposed to do.

rolfl

7 mins ago, by ckuhn203

I wonder if Enumerable.intersect(setA,setB) would be better.

You don't know.

ckuhn203

19:39

Intersect for example. It returns a new Enumerable. I'm unsure of the signature.

No. I do.

rolfl

The whole purpose of your code is to be a library.

Vogel612

now it's getting interesting.

rolfl

you are inviting a debate about what your library should look like.

Vogel612

@rolfl depends on how you see it.

ckuhn203

No. I'm inviting feedback on whether or not I've created a clunky interface.

Vogel612

19:40

What the code should do: Take 2 enumerables and return one

whether he takes one by instance and one by param or both by param is irrelevant

ckuhn203

And it does that. What I'm asking is does it do that in the best way.

rolfl

No, you are asking "what is the best way".

currently it does it "this way".

Vogel612

@rolfl I am not sure whether he is.

He knows what the code is supposed to do.

He knows that the code does what he is supposed to do.

ckuhn203

It's been said before that best practice is on topic when it is asked as "is this best practice"

That's what I mean to ask. So, what could be different about how I asked it?

Vogel612

But he's not sure whether the code does that in the best way.

rolfl

19:43

@ckuhn203 - I am not meaning to pick on you. You happen to have stumbled in to the path of a lively debate.

And, nothing is being decided right now about your question, so don't worry about it being closed or anything.

Vogel612

and for any API creation, the exposed interface is not "what does my code do", but "Does he do it well?"

rolfl

There is a big difference between "what is a good interface for my program", and "Does my program implement the interface well".

Vogel612

@rolfl you got the wrong interface here.

rolfl

If you can't say you know what the interface is, you can't have your code ready for review.

Vogel612

(removed)

ckuhn203

19:44

@rolfl Yes, so out of my interest in the debate, not the question, what would be stated differnently?

rolfl

I use interface int he classic sense.

Vogel612

and that is already defined here.

rolfl

that is how people call it/use it.

ckuhn203

@Vogel612 There are interfaces, but I'm not using them here.

Vogel612

The problem is, that the goal of any API is to provide a "good" interface, whatever that is.

and what's behind the interface is What it does.

the interface itself is How it does what it does.

rolfl

19:46

@ckuhn203 - if you had said: I decided to implement it using Enumerable.intersect(setA,setB) instead of setA.intersect(setB), is the implementation good?

ckuhn203

I really do see this as a "Does my code look fat?" question.

> I'm concerned that the API is inconsistent.

rolfl

instead you (effectively) said "I am not sure which one to use. For the moment I am doing X".

ckuhn203

Are we reading the same question?

rolfl

Argh.

@ckuhn203 - I see your question as having two parts.

part 1 is "is this a good design"

part 2 is "assuming this is a good design, is the implementation good".

Vogel612

aka "does my code look fat", right?

rolfl

19:49

Now, on code review, questions containing only part1 are off-topic.

ckuhn203

I'm still not convinced that part 1 here is off topic.

rolfl

questions containing only part 2 are always assumed to be "based on a predetermined" design, and the code review assumes that design is good, and, if the design turns out to be faulty, then, like any other 'bug' found in the review, it can be commented on.

ckuhn203

In summary, This is how I wrote this. It feels clunky to me. Does it actually smell?

rolfl

sure, that is how you think of it.

Simon André Forsberg

Thunderstorm here but i agree with chukn. Will comment more later

Vogel612

19:51

@rolfl I think that is too ...

I don't think suboptimal design-choices are as heavy as bugs.

rolfl

depends on the design, depends on the bug.

Vogel612

They're often easier to spot and in many cases not that hard to fix

ckuhn203

Idk. Maybe I'm too close to this.

But distancing myself as far as I can, I don't see an issue with it.

There's code. It works. It does something. I'm not asking anyone to design it for me. I'm asking if it's usable.

rolfl

It is my point that you are not sure what the 'something' in 'does something' should look like.

there is a two stage part to the review.

the first stage has to be to either confirm, or counter the 'signature' of the method.

If the reviewer agrees that your signature is right, then the review is easy

if the reviewer disagrees with the signature, then the whole thing is complicated.

ckuhn203

Since when do we determine scope based on how hard it is to review something?

rolfl

19:56

all the time.

Vogel612

@ckuhn203 it's not about hard to review

rolfl

if the code has bugs, even though it is otherwise reviewable, the question is off-topic.

Vogel612

but about: "makes sense to review right now"

rolfl

bugs get fixed first before the review happens.

design decisions are the same.

Vogel612

Imagine someone came and said: "your design is crap"

rolfl

19:58

you have to decide what your code should do, and how it should look, before you ask us to review it, and confirm or challenge whether it it doing things right.

Vogel612

would there be any point in him telling you that your method implementing said "crap" design could be better?

rolfl

if you can't tell us what it should do, how can we tell you it is doing it right.

and code review is not the right Q&A site for deciding what your code should do.

ckuhn203

But this isn't asking what it should do. It's asking if how it does it is okay.

And that's certainly on topic, no?

rolfl

how it does it is on topic.

ckuhn203

It returns the same either way.

rolfl

19:59

my concern here is that, in your particular case, the actual method signature is critical to 'what' the code does.

Vogel612

@rolfl I think the problem here lies in the fact that, how it does it, is already implemented.

@rolfl I disagree

Simon André Forsberg

Metthod signatures = how it does it

2

ckuhn203

What makes a method signature off topic?

Because that's what you're saying.

Simon André Forsberg

Writing on tablet = extremely slow

ckuhn203

So, here's an example, OP didn't ask, but the method signature is certainly a valid part of the review. So, what's wrong with being unsure about it, and asking.

1

A: Adding string binary numbers

For now, let's ignore the fact that there are easier ways to add binary numbers. There are other issues with this code. Inherits System.Web.UI.Page Why is code that adds binary numbers inheriting from a UI class? There's no need for this. Separate the concerns and create a module for this...

rolfl

20:02

OK, Let me put together a more clear example of what I am talking about:

Fizz buzz

Fizz buzz is a group word game for children to teach them about division. Players take turns to count incrementally, replacing any number divisible by three with the word "fizz", and any number divisible by five with the word "buzz". == Play == Players generally sit in a circle. The player designated to go first says the number "1", and each player thenceforth counts one number in turn. However, any number divisible by three is replaced by the word fizz and any divisible by five by the word buzz. Numbers divisible by both become fizz buzz. A player who hesitates or makes a mistake is eliminated...

ckuhn203

Assuming there's an implementation underneath it.

rolfl

FizzBuzz.

ckuhn203

ugh.

Okay.

rolfl

A question comes up on Code Review:

I need to implement FizzBuzz. Here's my code.

On topic, or off topic?

On topic.

check.

A question comes up on code review:

should I implement fizz buzz using a % 3 and % 5 and string concatenation, or with a %15 for FizzBuzz"

off-topic, design decision only.

ckuhn203

Agreed.

rolfl

20:03

Now a quesiotn comes up and says:

should I implement fizz buzz using a % 3 and % 5 and string concatenation, or with a %15 for FizzBuzz, Oh, and here it is and I implemented it using %15

On topic, or off topic?

ckuhn203

Off topic.

rolfl

why?

Vogel612

On IMO

ckuhn203

Because it's contrived....

Seriously though, because OP hasn't made a decision.

Simon André Forsberg

Edit out thhe first sentence

And then it is on

Vogel612

20:05

@SimonAndréForsberg that

rolfl

edit out the first question and the design is 'decided'.

that is my IOQ.

ckuhn203

Here's one @rofl.

rolfl

no, no more.

Damn, I am hitting the chat rate-limiter like mad!

ckuhn203

I wrote this implementation of fizzbuzz using %15. Would %3 and %5 have been better?

rolfl

2 mins ago, by rolfl

should I implement fizz buzz using a % 3 and % 5 and string concatenation, or with a %15 for FizzBuzz, Oh, and here it is and I implemented it using %15

ckuhn203

20:06

On topic, or no?

rolfl

off topic in my version of things.

Simon André Forsberg

On chukkn

ckuhn203

I think it's on too.

Vogel612

I agree, too

rolfl

So, an answer to: I wrote this implementation of fizzbuzz using %15. Would %3 and %5 have been better? could be:

> use a switch statement!

Vogel612

20:07

@SimonAndréForsberg on a sidenote... mobile < tablet

rolfl

that question invites a discussion unrelated to the code review.

Vogel612

@rolf why not?

Simon André Forsberg

It is a question about "does this question follow best practices"

ckuhn203

In A vs. B questions, C is to be expected.

Vogel612

How is that not a valid review? May I remind you that you did that yourself?

rolfl

20:09

My past actions in no way reflect the discussion at hand ;-) !

ckuhn203

And to point, Vogel suggested C to me earlier right in here.

I don't see how this is pertinent to the conversation.

rolfl

Also, when I said use a switch statement, I was challenging a pre-determined design.

the question was not "which design should I use", it was "here's my implementation", and my answer was "use a different design".

ckuhn203

So what' wrong with OP challenging his own design?

rolfl

everything

Vogel612

I wouldn't put that into design FWIW

rolfl

20:11

Code review questions should be "is my ass fat", not "what's the best way to make it skinny".

Vogel612

How nice would that be.

rolfl

OK. This discussion is a really challenging one, and time consuming.

Vogel612

My Cr questions are= my ass is fat, but I don't know how to make it skinnier.

rolfl

I am getting stubborn about it too, and I probably need to step away from it for a while.

Vogel612

And if by that I challenge my own design decisions, so be it.

rolfl

20:13

@ckuhn203 - I appreciate your participation as a 'dummy' we can poke spears at!

2

Simon André Forsberg

I think you are trying to create a line @rolfl that people will have a hard time understanding. I think it will cause lots of problems.

Code review does not need another gray area

rolfl

That's what I think you are doing ;-)

ckuhn203

You're welcome guys. I apologize if I was a little too close to this. I am interested in the broader discussion.

Vogel612

The cool thing is probably: if we could understand @rolfls point and he'd understand ours, we probably wouldn't even realize what @Simon said

Simon André Forsberg

To me it boills down to: here is code I have made. Does it follow best practices?

rolfl

20:17

To me it boils down to: how should I write my program, and, by the way, this is what I have right now!

Simon André Forsberg

Now I don't understand what you are saying @vogel

Vogel612

What is it?

Simon André Forsberg

Such question should be asked as here is how I wrote my program, can it be improved?

Vogel612

@Simon well without the trouble understanding it, we couldn't imagin it's hard to understand...

rolfl

9

Q: Quadtree design

I have an application which is used for displaying and modifying huge volumes of point cloud data from lidar files (up to few gigabytes each, sometimes loaded in simultaneously). In the app the user is able to view a 2D image of loaded points (from the top) and select a profile to view in another...

c++ performance data-structures tree opengl

Simon André Forsberg

20:23

Logging off for now, back tomorrow

rolfl

Now look at the answers.

cheers Simon, TTGH for me too.

(and catch up on some work when I get there).

Vogel612

Yep it's more of a programmers question with excess code...

But ima head to bed now.

ckuhn203

Just one last parting thought, because I'll be heading home soon.

My question. It is a design question of sorts, but it's the last chance to change the design before making changes to it will break client code.

The design was decided on and implemented, now it asks for one last sanity check before it can't be changed.

Just food for thought.

rolfl

exactly - we are not the place to be helping with debating the design (even though we are smart enough to).

and, if it is wrong (the design), then it needs to be fixed before the code is ready for review

ckuhn203

And how would a single developer know his implemented design is broken if it's not "reviewed" by someone else?

rolfl

20:27

He can't.

ckuhn203

I don't want an answer. Just throwing that out there.

rolfl

and it sucks.

it's horrible.

but, just because there is no other good place, does not mean that Code Review is the right place.

ckuhn203

True, but that doesn't mean it isn't either.

rolfl

And, you, personally, have plenty of buddies in the 2nd monitor, and stuff, that can help you with that.

ckuhn203

I do. Yes.

Idk. Lots to think on. I've gotta go.

rolfl

20:30

Asking if the design is OK is Off topic. Asking whether an implementation implements a design is on-topic. getting 'unsolicited' feedback about the design is what makes an OK answer a good answer.

Madara Uchiha

I half expected there to be a single message here with either "Yes" or "No" and the room being frozen :P

rolfl

Let me do that for you....

No

Stack Exchange

rolfl has frozen this room.

Madara Uchiha

Cheers

Feeds

Madara Uchiha has unfrozen this room.

rolfl

20:40

hehehe.

Madara Uchiha

Although, why would interfaces not be on-topic?

rolfl

11

Q: Questions about interfaces, protocols, and APIs

Are questions about interfaces/protocols and APIs on-topic for Code Review? And when I say "protocol", I mean this in the Objective-C/Swift (and potentially other languages) version of the word, which is basically the same as a Java interface. There is this meta discussion: “Code not yet writ...

discussion scope

3

I have to go home, for real, but, it's basically that, if the interface needs to be changed, then the design is wrong too. So, only formatting and style reviews make sense for interfaces. At least, that's one argument.

Deciding whether a Java interface method should return int or Integer could be a big decision.

Gone

Phrancis

Has anyone considered this, perchance:

programmers.stackexchange.com/questions/tagged/interfaces

Seems like interfaces is pretty active there and is getting answers

But I don't have a horse in this race so I will leave it at that :)

ckuhn203

21:41

Per the earlier conversation @rofl. Doesn't this answer cover my question? I understand that things change over time, but if we're going to change it, then we better have a good reason.

7

A: Are questions about public interface (API) on topic?

Are questions about public interface (API) on topic? Yes! If they are from a specific working piece of code. Quoting from the relevant part of the FAQ: If you are looking for feedback on a specific working piece of code from your project in the following areas… Best practices and de...

Why all answers to interface-only questions are valid

Transcript for

Code Review - Discussion about help v…