Re: [PATCH 2/4] Refactoring: connect.c: move duplicated code to get_host_and_port

From: Johannes Sixt <j6t@kdbg.org>
Date: Feb 15, 2010, 21:11 UTC
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:

Show 6 quoted lines

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