🚀 go-pugleaf

In article <vNwQa.10678$Yf3.823463036@hestia.telenet-ops.be>, 
slavearc@pandora.be says...

<knip>

>      if(lsChar = 'X')	<- Hier is het probleem
>      {
>           lhCursor = AfxGetApp()->LoadStandardCursor(IDC_ARROW);
>           SetCursor(lhCursor);
>           OnOK();
>      }

Je doet hier een assignatie ipv een vergelijking. Vermits 'X' 
verschillend is van 0 geeft dit true terug en doe je de OnOK() waardoor 
de dialog afsluit.

Om zulke fouten te vermijden draai je best zulke vergelijkingen om :
Voorbeeld : if ('X' == lsChar)

Als je nu 1 = teken zet ipv 2 krijg je een compileerfout, wat 
gemakkelijker te verbeteren is als een logische fout.

Groeten,

Haplo

In article <vNwQa.10678$Yf3.823463036@hestia.telenet-ops.be>, 
slavearc@pandora.be says...

<knip>

> 
> void CMouseDlg::OnKeyDown(UINT nChar, UINT nRepCnt, UINT nFlags)
> {
>      char lsChar;  //ingedrukt key
>      HCURSOR lhCursor; //handle naar af te beelden cursor
> 
>      lsChar = char(nChar); //cast key in char
> 
>      if(lsChar == 'A')
>      {
>           lhCursor = AfxGetApp()->LoadStandardCursor(IDC_ARROW);//get cursor
> handle
>           SetCursor(lhCursor); //zet cursor
>      }
>     if(lsChar == 'B')
>      {
>           lhCursor = AfxGetApp()->LoadStandardCursor(IDC_IBEAM);
>           SetCursor(lhCursor);
>      }
>      if(lsChar == 'C')
>      {
>           lhCursor = AfxGetApp()->LoadStandardCursor(IDC_WAIT);
>           SetCursor(lhCursor);
>      }
>      if(lsChar = 'X')
>      {
>           lhCursor = AfxGetApp()->LoadStandardCursor(IDC_ARROW);
>           SetCursor(lhCursor);
>           OnOK();
>      }
>      else
>      {
>           SetCursor(lhCursor);
>      }
> 
> 
>      CDialog::OnKeyDown(nChar, nRepCnt, nFlags);
> }

Update: Ik heb ook eventjes naar de rest van de code gekeken en er zijn 
nog een paar kleine opmerkingen.

- Gebruik 1 groot "if-else if" blok ipv afzonderlijke if's. Hierdoor 
wordt de code leesbaarder en vermijd je onnodige vergelijkingen
- Vergelijk nChar met de echte Virtual Key Codes (VKEY_A, VKEY_B, ...) 
ipv van de cast naar char. Nu komt dit toevallig uit maar dit is niet 
zeker voor andere keys.
- Verplaats de SetCursor na het "if-else if" blok, omdat dit in elke if 
voorkomt en dus niets met de if te maken heeft. 
- Als je intensief van die paar Cursors gebruik gaat maken is het 
optimaler van ze in static const variabelen te steken en die dan te 
gebruiken. Zo vermijd je dat ze steeds geladen worden bij elke onKey
- In het else-blok doe je een SetCursor van een mogelijk ongedefinieerde 
HCURSOR. Dit zou ook wel eens een crash kunnen veroorzaken.

Groeten,

Haplo

Dankt u allen zeer,
Ik ben gewoon weer zo stom geweest (en te ongeduldig)
om die fout te zien (ik kom het wel meer tegen, maar meestal zie
ik het na een minutje of 10 toch)

Sorry om jullie lastig te vallen met zo'n domme fout,
Jullie mogen nu allemaal beginnen lachen.....

--
Harbinger Of Doom

Haplo wrote:
> In article <vNwQa.10678$Yf3.823463036@hestia.telenet-ops.be>, 
> slavearc@pandora.be says...
> 
><knip>
> 
>>      if(lsChar = 'X')	<- Hier is het probleem
>>      {
> Om zulke fouten te vermijden draai je best zulke vergelijkingen om :
> Voorbeeld : if ('X' == lsChar)
> Als je nu 1 = teken zet ipv 2 krijg je een compileerfout, wat 
> gemakkelijker te verbeteren is als een logische fout.

Dat is iets wat dikwijls aangeraden wordt, maar ik doe dat liever niet.
Voor een stuk omdat ik dat minder leesbaar vind, hoewel dat grotendeels
een kwestie van gewoonte is.
Maar vooral omdat ik het gevoel heb dat dat aanzet tot slordiger
gebruik: je denkt er minder over na, "de compiler zal het toch wel
zien". Veel vergelijkingen zijn echter vergelijkingen tussen twee
variabelen, en dan zal bovenstaand trucje niet helpen. De compiler ziet
niet dat er iets mis is.
Ik denk er liever aan elke keer ik een if-statement schrijf: ah ja,
oppassen, geen = maar ==. Ik zal niet zeggen dat ik de fout nog nooit
gemaakt heb, maar het is toch wel hoogst uitzonderlijk -- pakweg een
keer of twee � drie op vijf jaar tijd.

In dit specifieke geval zou ik trouwens een switch-statement gebruiken
i.p.v. if-else if:

switch (nChar)
{
case 'A':
  /* ... */
  break;
case 'B':
  /* ... */
  break;
/* ... */
default:
  /* ... */
  break;
}

-- 
"Codito ergo sum"
Roel Schroeven

Je kan ook het warning level verhogen naar 4
(project/settings/c-c++)

Dit soort fouten geeft dan netjes de melding:

warning C4706: assignment within conditional expression

Ter info, wij compileren altijd op het hoogste level. Hoogst zelden dat er
eentje over blijft ... na voldoende correct typewerk.

bvb: while(1) ... geeft C4706 op level 4

        for(;;) ... doet net hetzelfde zonder meldingen



Patrick Smout

"Haplo" <Wim.Valgaeren@p.b.invalid> schreef in bericht
news:MPG.197d126aa6eb9dc998968c@news.telenet.be...
> In article <vNwQa.10678$Yf3.823463036@hestia.telenet-ops.be>,
> slavearc@pandora.be says...
>
> <knip>
>
> >      if(lsChar = 'X') <- Hier is het probleem
> >      {
> >           lhCursor = AfxGetApp()->LoadStandardCursor(IDC_ARROW);
> >           SetCursor(lhCursor);
> >           OnOK();
> >      }
>
> Je doet hier een assignatie ipv een vergelijking. Vermits 'X'
> verschillend is van 0 geeft dit true terug en doe je de OnOK() waardoor
> de dialog afsluit.
>
> Om zulke fouten te vermijden draai je best zulke vergelijkingen om :
> Voorbeeld : if ('X' == lsChar)
>
> Als je nu 1 = teken zet ipv 2 krijg je een compileerfout, wat
> gemakkelijker te verbeteren is als een logische fout.
>
> Groeten,
>
> Haplo