4

Might've gotten a bit ahead of myself on my previous meta.

It'd be helpful to have some guidelines to determine if a given question is or is not a code-review question.

The intent here is to start with a broad definition, and then comments/answers can refine it. Hopefully, we'll end up with a clear winner in terms of votes.

So, to start things off...

What makes a question a request for a code-review?

  • It contains code that is currently working (if not the most ideal/following best practices)
  • The "problem statement" of the question primarily revolves around improving the code rather than overcoming an error, learning specifics about a piece of functionality, getting code to work in the first place, that kind of thing
  • Instead of focusing on fixing a single thing, there are likely to be multiple components where improvements can be made or best practices can be followed

I think that this question fits the criteria for being a code-review question
I think that this other question does not fit the criteria for being a code-review

5
  • One of the rubrics that feels right to me is "does the asker know what's wrong?". I don't read the first question linked as a code review because the asker knows where their problem is/has identified the not-best-practice, but doesn't know how to fix it. Their question provides some level of direction and specificity, which to me is the core of on-topicness. "How do I improve this code?" without any indicator of specificity is clearly off-topic to me.
    – David Reed Mod
    Commented Jan 21, 2020 at 23:43
  • While the linked question isn't perfect, I think it fits our format a lot better than many do (and I also don't want to put that poster on the spot too much here). I don't think that code has to be broken to be asked about, but the question does have to be specific. "I'm having trouble with SOQL limits. I see I have a query HERE and HERE that seem to be consuming too much limits, but I can't figure out how to move this one out of the loop" is, to me, specific enough to be on topic. "How do I bulkify this block of code?" is probably not.
    – David Reed Mod
    Commented Jan 21, 2020 at 23:45
  • (I remain open to persuasion that this is not the right framework to apply to this issue).
    – David Reed Mod
    Commented Jan 21, 2020 at 23:46
  • @DavidReed I agree with specificity being a key to on-topicness for our site. I'm struggling with reconciling "does the asker know what's wrong" with my understanding of "code review" (I am working on it, though). Perhaps, at the risk of making things even more nebulous, I can re-frame my thoughts more along the lines of "Does OP present a desire to learn and improve, or are they trying to pawn off their work onto us?"
    – Derek F
    Commented Jan 23, 2020 at 14:50
  • 1
    I was not clear about what I was driving at; apologies. What I meant to propose is that, if the asker shapes their question to target a specific problem and give some indication of what a solution looks like, it is not a code review question - even if the problem is one, like bulkification, that would often come up in code review, and regardless of whether the code works at some level or not. If the asker is generically seeking improvement or review without providing any direction or effort on their part, that would to me be a code review question and off-topic.
    – David Reed Mod
    Commented Jan 23, 2020 at 16:21

1 Answer 1

Reset to default
6

I've always been of the mind that Salesforce-specific questions need to stay here. Users on CRSE and SO may not be aware of governor limits, Locker API, Salesforce-specific optimizations that may exist, etc.

My litmus test for this is simple: if you remove all of the Salesforce-specific language, does the question become nonsensical or otherwise impossible to answer, or would the answer you would provide change under this condition?

The goal of this test is simple. If you can remove all the stuff that only works in Salesforce, and the answer doesn't change, it is not related to Salesforce technology and doesn't belong here. A "yes" to my test means that the question belongs here--it's Salesforce we support, and this is a Salesforce question.

For example, the first question, you considered it a code review. Aside from the fact that this is Apex, and we likely have the highest percentage of Apex developers across all networks, I would argue that CRSE would likely not have provided the following:

FYI, if you changed it to "before insert" you could avoid the recursive update. – sfdcfox

This little comment directly reduced the CPU time and DML statements used, perhaps significantly, reducing future possible problems and giving that developer's users a better user experience. Can you guarantee the OP would have gotten this on CRSE?

Questions about optimizing Apex should always be on-topic here, because we're the most qualified to answer them. JavaScript is a little blurrier, but my test will almost certainly catch all the questions I'd consider off-topic.

As a general rule, the core developers we have on SFSE are more likely well-equipped to answer a Salesforce question on CRSE, than their developers are well-equipped to fully answer a question here.

Of course, I am aware that some of us also go over to CRSE, but it feels like maybe we're "forced" to go visit CRSE just to make sure that people using Salesforce are getting the best possible code reviews... on a different network? Why not keep them here?

Also, CRSE has a different set of rules. I've tried answering questions over there and mostly just gave up. They are even more unfriendly to outside posters than we have been perceived to be.

I realize that in order to participate in any community, you need to abide by its rules, but they seem content to shoot first and ask questions later (and never explain why). There's a reason why I'm still under 600 rep there, even though I've been on there for ages.

I'm aware that not everyone likes code reviews. Many probably won't do anything different if we suddenly "officially" supported code reviews, except perhaps not flag it in to the dirt before someone who's willing to do such a review can answer.

I'm also not saying that we should support 500+ LoC questions, or deal with more than perhaps 5 different issues in a single piece of code (even on CRSE, you're not likely to see more than 3-5 suggestions in an answer).

However, by answering some code review questions, it may help other visitors that have similar problems with bulkification, optimization, etc. To look at any question and immediately dismiss it as a code review is a wasted opportunity.

I agree that we're not here to do any arbitrary code review. That's what CRSE is for. If you're asking about optimizing a C++ program, SFSE can't help. However, our domain-specific knowledge can, and should, be leveraged to provide support to the Salesforce Ohana, even if that support means doing a code review.

3
  • I think this is probably a better answer for my other recent meta. I'm trying to separate the discussion about what a "code review" is from whether or not we want to accept "code reviews" here, since we (as a community) seem to have different ideas about both things. It's the lack of cohesion that prompted me to make these two metas, and I think defining what a "code review" is will aid in the other discussion about if we want "code reviews" here.
    – Derek F
    Commented Jan 22, 2020 at 17:14
  • My previous comment aside...I agree on your main points. Questions where SFDC is integral generally belong here. Our SF experts are primarily here rather than on CRSE. If we do decide that code reviews should be on CRSE, then that would increase friction. It'd force our interested users to trawl CRSE too (which would naturally lead to less involvement than if we kept the question here).
    – Derek F
    Commented Jan 22, 2020 at 17:24
  • Devil's advocate: From my understanding, CRSE is the general place for code reviews on the SE network. An expert in C++ probably isn't going to do a code review for, say, Haskell, but that doesn't mean that Haskell code reviews belong on SO. Similarly, a Java expert probably wouldn't review an Apex question (or, at least, not as well as an Apex expert). The lack of Apex experts on CRSE is a problem that can be fixed, and the need to build ourselves a little exclave on CRSE should be taken into consideration for the approach we settle on.
    – Derek F
    Commented Jan 22, 2020 at 17:35

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .