Click here to Skip to main content
15,851,514 members
Home / Discussions / C / C++ / MFC
   

C / C++ / MFC

 
QuestionRe: LoadString failed in Windows7 Operating System. Pin
David Crow7-Jun-11 3:52
David Crow7-Jun-11 3:52 
GeneralRe: LoadString failed in Windows7 Operating System. Pin
Richard MacCutchan7-Jun-11 1:57
mveRichard MacCutchan7-Jun-11 1:57 
GeneralRe: LoadString failed in Windows7 Operating System. Pin
Richard MacCutchan7-Jun-11 1:59
mveRichard MacCutchan7-Jun-11 1:59 
QuestionC++ Question Pin
Software20076-Jun-11 18:18
Software20076-Jun-11 18:18 
AnswerRe: C++ Question Pin
Niklas L7-Jun-11 3:00
Niklas L7-Jun-11 3:00 
GeneralRe: C++ Question Pin
Stefan_Lang7-Jun-11 3:05
Stefan_Lang7-Jun-11 3:05 
GeneralRe: C++ Question Pin
Niklas L7-Jun-11 3:26
Niklas L7-Jun-11 3:26 
AnswerRe: C++ Question Pin
Stefan_Lang7-Jun-11 3:00
Stefan_Lang7-Jun-11 3:00 
Go, figure:

Apparently the intent of the function is to set a value which is bound to some key, but this functionality may have side-effects or may be varied depending on some flags which are passed with the last parameter.

The flags being handled are No_UpdateType, Private_UpdateType, and Public_UpdateType.

The function returns true or false, depending on whether or not certain tests, depending on these flags, are successful. Apparently the result should be true only if the key exists and the value could be successfully updated, but the implementation does not fully back up this assumption.

Problems and shortcomings:

The (member?) variable m_Something is being dereferenced without testing for 0. (Might be ascertained elsewhere, but the code doesn't indicate any of that) (style, possible error)

There is no (visible) test whether an element exist for the given key value before calling SetInt(). Although that code might be hidden in SetInt(), there should be an indication in case of failure, but apparently there is none. (style, possible error)

retval is not initialized, and not all execution paths assign a value to it. It may be undefined on return. (error)

The first if statement can lead to a premature return. Apart from the fact this is bad style, it will cause a memory leak, due to the fact that buffer will not be released. (style, memory leak)

Also, the flag No_UpdateType which this if statement tests for seems to hint at the possible meaning that no update should actually be performed, so the call to SetInt() right at the start of the function may have been premature. (possible error)

Moreover, the test is wrong: '==' has a higher precedence than '&', so the line
if ( update & No_UpdateType == No_UpdateType )
will be interpreted as
if ( update & (No_UpdateType == No_UpdateType) )
which in turn is equivalent to
if ( update )
. The test requires additional brackets like this:
if ( (update & No_UpdateType) == No_UpdateType )
(error)

The same error is repeated twice below. (error * 2)

The following two if blocks contain code that is partially identical. It would be a good idea to extract this code into a separate function, or at least to pull it out of the individual if blocks to prevent redundancy. (style)

The for loop in these blocks suffer from the following three problems:
1. retval is needlessly assigned once for every iteration (performance)
2. once an element equal to key is found, the loop could be abandoned (performance)
3. After this element has been found, retval is being overwritten with false again (error)

There is one more curiosity revealed in these loops: apparently there is some structure which is supposed to hold all key values, but at the same time, this key is being used to set a particular value within the object m_something. This hints at possibly redundant storage, or at least an inefficient way to store a mapping between some key and value. It would probably be a good idea to use a map or hash_map - this would also eliminate the need to implement your own key-searching and value-setting functions. (stlye, possible waste of resources)

The SendXY() functions use buffer before it is being initialized. (likely error * 2)

The two if blocks may both be executed, in which case the return value might be overwritten yet again. Not to mention that the same for loop would be executed twice. (likely error)

Wrong call to delete (error: should be delete []).

On a sidenote, if the size of the buffer should always be the same, it would be better to define it on the stack in stead of the heap! (style, performance)

Conclusion:

As indicated above, the implementation probably does not set the return value according to its intended meaning. I assume the functionality should be something like this:
// test for the existance of a key in my data structure
bool Item::key_exists(int key) {
   // do the for loop or whatever more suitable test you can think of
}
// test if a particular flag is set
bool is_set(UpdateType update, UpdateType flag) {
   return ( update & flag ); // the '== flag' part will give the same result --> redundant
}
bool Item::SetInt(/*...*/) {
   bool retval = key_exists(key); // some new function replacing the for loop that searches for the key
   if (!is_set(update, No_UpdateType)) { // even though the test is trivial, extract to function
      retval = false; // do not set value (?)
   }
   if (retval) {
      byte buffer[256];
      m_something->SetInt(key, val);
      if (is_set(update, Public_UpdateType)) {
         //ToDo: initialize buffer here!
         SendPublic(/*...*/);
      }
      if (is_set(update, Private_UpdateType)) {
         //ToDo: initialize buffer here!
         SendPrivate(/*...*/);
      }
   }
   return retval;
}

GeneralRe: C++ Question Pin
Software20077-Jun-11 6:03
Software20077-Jun-11 6:03 
QuestionMiddleware/CORBA Data Field Inheritance Pin
rkeeler786-Jun-11 14:11
rkeeler786-Jun-11 14:11 
QuestionSuddenly Exe is not running Pin
pix_programmer6-Jun-11 3:16
pix_programmer6-Jun-11 3:16 
AnswerRe: Suddenly Exe is not running Pin
Alan Balkany6-Jun-11 10:21
Alan Balkany6-Jun-11 10:21 
AnswerRe: Suddenly Exe is not running Pin
Stefan_Lang7-Jun-11 3:17
Stefan_Lang7-Jun-11 3:17 
GeneralRe: Suddenly Exe is not running Pin
pix_programmer7-Jun-11 3:49
pix_programmer7-Jun-11 3:49 
GeneralRe: Suddenly Exe is not running Pin
Stefan_Lang7-Jun-11 3:55
Stefan_Lang7-Jun-11 3:55 
GeneralRe: Suddenly Exe is not running Pin
pix_programmer7-Jun-11 4:00
pix_programmer7-Jun-11 4:00 
GeneralRe: Suddenly Exe is not running Pin
Stefan_Lang7-Jun-11 4:26
Stefan_Lang7-Jun-11 4:26 
QuestionDisplay CBitmap into CStatic [modified] Pin
_Flaviu6-Jun-11 2:54
_Flaviu6-Jun-11 2:54 
AnswerRe: Display CBitmap into CStatic Pin
Roger Allen6-Jun-11 3:24
Roger Allen6-Jun-11 3:24 
GeneralRe: Display CBitmap into CStatic Pin
_Flaviu6-Jun-11 3:39
_Flaviu6-Jun-11 3:39 
QuestionCStatic ModifyStyle Pin
_Flaviu6-Jun-11 2:00
_Flaviu6-Jun-11 2:00 
AnswerRe: CStatic ModifyStyle Pin
The_Inventor6-Jun-11 21:41
The_Inventor6-Jun-11 21:41 
AnswerRe: CStatic ModifyStyle Pin
Code-o-mat7-Jun-11 23:59
Code-o-mat7-Jun-11 23:59 
QuestionHow to use tcp keepalive to check the client is alive? Pin
wangningyu5-Jun-11 0:23
wangningyu5-Jun-11 0:23 
AnswerRe: How to use tcp keepalive to check the client is alive? Pin
Code-o-mat5-Jun-11 23:55
Code-o-mat5-Jun-11 23:55 

General General    News News    Suggestion Suggestion    Question Question    Bug Bug    Answer Answer    Joke Joke    Praise Praise    Rant Rant    Admin Admin   

Use Ctrl+Left/Right to switch messages, Ctrl+Up/Down to switch threads, Ctrl+Shift+Left/Right to switch pages.