[Qt] TScrollBar patch

Felipe Monteiro de Carvalho felipemonteiro.carvalho at gmail.com
Mon May 28 23:19:36 CEST 2007


Hi, the patch looks good, but I would like that some things be changed:

* DoTheJob is the typical name that a function shouldn't have (i.e. it
doesn't mean anything).

* Also, I think that internal functions should be avoided whenever
possible, and particularly in this case I see that it could be
avoided. at the point where it is called we only have:

+                   DoTheJob(SB_HORZ);
+                   Result := FScrollInfo.nPos;

for all cases (only in 1 we don't have, but then we could call Exit on
it) this is the last thing done, so it could be moved out of the case
statement. We can also use a variable to see later if SB_HORZ or
something else was selected.

The only problem is that the function would become too big. If another
function is really needed I would rather have a private function on
TQtWidgetSet class. This will force the function to not use the
private variables of the main one, and thus improve it's design.

* Identation:

  case VariableX of
    CONST_A:
    begin
    end;

    CONST_B:
    begin
    end;
  end;

each identation step takes 2 spaces and the begin is on separate line
as the constant

Carefully written code means it will be easier to maintain later =)

thanks,
-- 
Felipe Monteiro de Carvalho



More information about the Qt mailing list