Re: Code review

[Steve Baker <sjbaker1@airmail.net>]

"Philipp Gühring" wrote:
> 
> Hi!
> 
> What do you think about the following code snippet, which checks for
> rectangular buttons?
> 
> typedef struct
> {
>   int active;
>   int x; int x1;
>   int y; int y1;
> } Buttontyp;
> 
> Buttontyp Buttons[]={
> {1,10,20,10,20},
> {1,20,120,20,120},
> {1,530,630,10,30},
> //...
> };
> int nButtons=sizeof(Buttons)/sizeof(Buttontyp);
> 
> int CheckButtons()
> {
>   int i=0;
>   // printf("nButtons: %d\n",nButtons);
>   for(i=0;i<nButtons;i++)
>     if ( Buttons[i].active && (mx>=Buttons[i].x) && (mx<=Buttons[i].x1) && (my>=Buttons[i].y) && (my<=Buttons[i].y1) )
>       return i;
>   return -1;
> };
> 
> Is there a better, "right" solution?

Well, if you were *REALLY* pushed for time and had a *HUGE* number of "Buttons"
then you could try storing the buttons according to (say) their Y coordinates
and do a binary search instead of a linear one...or perhaps grouping the
buttons into some kind of hierarchical structure with bounding boxes around
each group of buttons.  These technigues could dramatically speed up the
search if there were many hundreds of buttons.

However, if you are talking about a GUI with a couple of dozen buttons, it'll
be faster to do exactly what you are doing now.

If you are looking for points for style - and if I were being particularly
picky, I'd say that:

* 'i' is redundantly doubly-initialised to zero.
* Your 'if' statement has a lot of redundant brackets in it.
* You are using C++ style comments in what *appears* to be a C program...
* ...and if you are really using C++ then:

    * Using a typedef for 'Buttontyp' is an ugly 'C-ism'.
    * 'i' could be declared inside the 'for' clause.
    * The test for inside/outside the button would be better
      made as a member function of the Buttontyp class.

...but that would be overly picky. What you have is fine.

-- 
Steve Baker                  http://web2.airmail.net/sjbaker1
sjbaker1@airmail.net (home)  http://www.woodsoup.org/~sbaker
sjbaker@hti.com      (work)