conceptual problem

[psullsack]

I had a conceptual problem concerning stability of the following code. I would appreciate if someone could help me with it. I believe that the following code could be made more stable.

const int iNumCamera = 5;
const char *szCameraName[iNumCamera] = { "classic", "broadcast", "classic2", "classic3", "action" };

char *SelectCamera( int low, int high )
{
const char *s = NULL;
static char szCamera[100];
strcpy( szCamera, "First camera in range is " );
if( high > iNumCamera ) high = iNumCamera;
for( int i = low; i < high; i++ )
{
if( !s || strcmp(szCameraName [i],s) < 0 ) s = szCameraName [i];
}
strcat( szCamera, s ); return szCamera;
}


void DisplayCamera(int low, int high) {
print (“%s\n”, SelectCamera(low, high));
}

I would appreciate if one could help me with this one

cheers
pf

[torfil]

Presumably if you think it can be made more stable, you know what the problems with it are? If not.... sounds like homework to me...

The error checking is incorrect/incomplete, and there is something wrong with the value returned.

[Paul McKenzie]

I had a conceptual problem concerning stability of the following code. I would appreciate if someone could help me with it. I believe that the following code could be made more stable.

First, please use code tags when posting code.

Second, if this is a C++ program, you should be using C++ techniques such as std::string, and not resort to strcpy(), strcat(), char arrays, etc. (see my example below)

Third, you don't check if low is out of bounds, or if it is greater than "high".

Fourth, there is no such C or C++ function as "print".

Fifth, what exactly is the code supposed to accomplish? I don't understand exactly what you're supposed to be doing. Is it to select the first camera? Then why do you need a loop for that? Just return the first camera. Is it that you must return the first camera in alphabetical order? If that is the case, then the following code works:

#include <string>
#include <algorithm>
#include <cstring>
#include <iostream>

using namespace std;

const int iNumCamera = 5;
const char *szCameraName[] = { "classic", "broadcast", "classic2", "classic3", "action" };

bool CompareStrings(const char *s1, const char *s2)
{
   return strcmp(s1, s2) < 0;
}

string SelectCamera( int low, int high )
{
    high = max(0, min(high, iNumCamera-1));
    low =  max(0, min(low, high));
    return *min_element(szCameraName + low, szCameraName + high + 1, CompareStrings); 
}

int main()
{
    cout << "First camera in range is " << SelectCamera(0,4);
}

Output:
First camera in range is action

This returns the camera that has a name less than any of the cameras in the given range. Also note the check for low and high. The high value must be >= low, and must be < iNumCameras. The low value must be >= 0 and <= to the high value.

The std::min_element is an algorithm function that checks for the smallest value in a range. The range goes from (szCameraName + low) to (szCameraName + high + 1) (you need to go 1 past the ending element in the fange). The function used to compare is called CompareStrings() which takes two of the elements and compares whether the first element is < the second element.

If you are using the (broken) Visual C++ 6.0 compiler, you will need to #define min and max in the code:

#define min(x,y) ((x) < (y) ? (x):(y))
#define max(x,y) ((x) > (y) ? (x):(y))

If you are not that advanced, then at the very least get the low and high values correct, as my code did in the beginning of the function. It should be self-explanatory how min and max work.

Regards,

Paul McKenzie

[exterminator]

An addition to the list Paul mentioned.

const char *s = NULL;

There can be no use of 's' now. It cannot be changed and if you however attempt changing it using const_cast it would result in an undefined behaviour. What is the use of it? You already have the NULL macro pre-defined. Any further usage of the variable 's' in your code is not correct. I also suggest you to use std::string. Please use it. You can learn about it from here:
C++ string
Hope this helps. Regards.

[HighCommander4]

If you are using the (broken) Visual C++ 6.0 compiler, you will need to #define min and max in the code:

#define min(x,y) ((x) < (y) ? (x):(y))
#define max(x,y) ((x) > (y) ? (x):(y))

If you are using C++, a better idea would be to use template functions which are less bug-prone than using #define:

template <typename T>
const T& min(const T& a, const T& b)
{
    return (a < b) ? a : b;
}

template <typename T>
const T& max(const T& a, const T& b)
{
    return (a > b) ? a : b;
}