From: Johannes Sixt <j6t@kdbg.org>
Date: Mon, 15 Feb 2010 21:11:56 GMT
Subject: Re: [PATCH 2/4] Refactoring: connect.c: move duplicated code to get_host_and_port
Message-ID: <4B79B89C.1050603@kdbg.org>
In-Reply-To: <1266182863-5048-2-git-send-email-michael.lukashov@gmail.com>

Michael Lukashov schrieb:
> +static void get_host_and_port(char **host, const char **port, int set_port_none)

Minor nit: The last parameter, set_port_none, is a rather prominent sign 
that this function mixes policy and functionality. And indeed, this 
implementation:

> +	if (colon) {
> +		*colon = 0;
> +		*port = colon + 1;
> +		if (set_port_none && !**port)
> +			*port = "<none>";
> +	}

proves it. The _functionality_ is to find host and port from a string. The 
_policy_ is to set the port to "<none>" if it would otherwise be empty. 
The callers take care of the _policy_, this function should only care 
about _functionality_. There's only one call site that wants "<none>"; 
don't move this detail into this function.

Other than that: nice catch.

-- Hannes