Thread: transformExpr() refactor
This patch refactors transformExpr(): rather than being a monsterous 900 line function, it breaks it up into numerous sub-functions that are invoked by transformExpr() for individual expression types, in the style of transformStmt(). I think this patch is reasonably safe for HEAD (it mostly just rearranges code), but I don't mind holding on to it for 8.1. Comments? -Neil
Attachment
Neil Conway <neilc@samurai.com> writes:
> This patch refactors transformExpr(): rather than being a monsterous 900
> line function, it breaks it up into numerous sub-functions that are
> invoked by transformExpr() for individual expression types, in the style
> of transformStmt().
I don't actually find this to be an improvement. What's the point?
Since all the switch arms are independent, you haven't really done
anything at all to improve the comprehensibility of the code...
regards, tom lane
On Fri, 2004-10-29 at 00:17, Tom Lane wrote: > I don't actually find this to be an improvement. What's the point? > Since all the switch arms are independent, you haven't really done > anything at all to improve the comprehensibility of the code... I think the code is more readable this way. The very fact that the switch branches are completely independent is good enough reason to make them distinct functions, IMHO. Breaking 900 lines of code into smaller chunks of code, each of which does a single conceptual task, just makes the whole enterprise easier to understand. As for sharing code between the functions, I agree that isn't done today -- but it will be easier to do in the future with this refactoring. -Neil
On Thu, 2004-10-28 at 18:00, Neil Conway wrote:
> I think the code is more readable this way.
FWIW, I'm +1 on the patch for the above reason.
--
Regards,
James William Pye
Attachment
James William Pye wrote: -- Start of PGP signed section. > On Thu, 2004-10-28 at 18:00, Neil Conway wrote: > > I think the code is more readable this way. > > FWIW, I'm +1 on the patch for the above reason. I liked the large case statement myself. I don't like breaking things into pieces when the pieces are just splits out of a switch structure. You are basically giving names to switch actions and pushing into functions with names. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
On Thu, 2004-10-28 at 16:49 +1000, Neil Conway wrote: > This patch refactors transformExpr(): rather than being a monsterous 900 > line function, it breaks it up into numerous sub-functions that are > invoked by transformExpr() for individual expression types, in the style > of transformStmt(). I still think this patch is worth applying. Sadly, I will need to basically rewrite it due to code drift. I'm happy to do that, although I'd like to resolve whether we want to accept it or not. Tom and Bruce objected when I posted it originally, although I don't think we reached a conclusion. Why I think the patch is a good idea: 900 line functions are almost universally bad (in fact, I'd be tempted to remove the "almost"). A 900 line function that divides distinct functionality into different branches of a "switch" statement isn't _that_ evil, but it is still a large hunk of code for someone to understand as a single, monolithic unit. Because each branch of the "switch" statement is independent, we can trivially split each branch off into a function -- each branch does something distinct, so it ought to be a distinct function. That means the independence of each branch of the "switch" statement is guaranteed (the function can't modify a local variable in the calling function, for example), and it means that the code is conceptually easier to read. With the present layout, It does mean that you'll need to jump to the function definition if you want to see what a particular branch of the "switch" statement does. But (a) use tags, it's not tough (b) this is a _good_ thing. If I want to understand what the parent function does (transformExpr(), the one with the "switch"), I don't want to have to grok through a 700 line "switch" statement. If each branch of the switch just invokes a function, the intent of transformExpr() is perfectly clear. For refresh everyone's memory, I've attached the original version of the patch. It won't apply cleanly to current sources, but it should apply to HEAD as of approximately Oct. 28, 2004. -Neil
Attachment
Neil Conway <neilc@samurai.com> writes:
> Why I think the patch is a good idea: 900 line functions are almost
> universally bad (in fact, I'd be tempted to remove the "almost").
[ shrug... ] 900 line functions that consist of absolutely independent
case arms are not any harder to read than the alternative. I won't
stand in the way of you doing this, but I think you could find more
profitable uses for your time.
regards, tom lane
On Tue, 18 Jan 2005 16:15:57 +1100, Neil Conway <neilc@samurai.com> wrote: >900 line functions are almost >universally bad Amen. So you might be interested in reviewing http://archives.postgresql.org/pgsql-hackers/2004-06/msg00398.php ;-) Servus Manfred
On Tue, Jan 18, 2005 at 04:08:01PM +0100, Manfred Koizar wrote: > On Tue, 18 Jan 2005 16:15:57 +1100, Neil Conway <neilc@samurai.com> > wrote: > >900 line functions are almost > >universally bad > > Amen. So you might be interested in reviewing > http://archives.postgresql.org/pgsql-hackers/2004-06/msg00398.php ;-) Hmm. I think this is a good idea on principle, but what happens in case a previous vacuum was interrupted? Is there a possibility that tuples belonging to that vacuum are still marked MOVED_OFF but are not in vacpage->offsets, for example? -- Alvaro Herrera (<alvherre[@]dcc.uchile.cl>) "La fuerza no está en los medios físicos sino que reside en una voluntad indomable" (Gandhi)
On Tue, 2005-01-18 at 00:54 -0500, Tom Lane wrote: > I won't stand in the way of you doing this Attached is a revised patch. Barring any objections, I intend to apply this sometime tomorrow. -Neil
Attachment
On Wed, 2005-01-19 at 18:39 +1100, Neil Conway wrote: > Attached is a revised patch. Barring any objections, I intend to apply > this sometime tomorrow. Applied. -Neil