Re: svn commit: r1803899 - in /subversion/trunk/subversion: libsvn_ra_serf/ libsvn_subr/ mod_dav_svn/

Stefan Sperling wrote:
> On Wed, Aug 02, 2017 at 06:49:55PM -0000, kotkov_at_apache.org wrote:
>> +svn_boolean_t
>> +svn_ra_serf__is_local_network(svn_ra_serf__session_t *session)
>> +{
>> + return session->conn_latency >= 0 &&
>> + session->conn_latency < apr_time_from_msec(5);
>> +}
>
> "local network" is a rather blurry concept.
> [...] why not name this function after the question it answers,
> e.g. svn_ra_serf__is_low_latency_connection()?

Am I right in thinking what the caller really wants to know is whether
this is a high-bandwidth connection? If so, this is a case of the
following pattern:

  * the caller wants to know one thing (is it high bandwidth),
  * the available information is something else (is it low latency),
  * the caller uses the available information to make a guess.

Naming the helper function 'is_local' splits the guesswork to two
places, the caller and the callee. It really requires comments at both
places, otherwise code that does not do what it says can become
confusing later.

Naming the helper function 'is_low_latency' would make this function do
exactly what its name says; self-documenting is a good thing. A comment
at the point of call should then explain that it is using 'low latency'
to make a guess about 'high bandwidth'.

That is probably better, so long as there is only one such caller.

A self-documenting option that scales to multiple callers is to put all
the guesswork in the helper function, and name it so as to recognize that:

is_probably_high_bandwidth() {
  /* the best we can do for now is to guess from the latency */
  return (latency < X);
}

That is probably best.

- Julian
Received on 2017-08-03 10:33:55 CEST