CODEWITHSUNDEEP
c++ooppointersnullpointerexceptionnull
Photon Light
Photon Light

Reputation: 777

Dealing with accessing NULL pointer

In my class, I've got - inter alia - a pointer:

Class GSM
{
//...
private:
    char *Pin;
//...
}

My constructor initialize it as:

GSM::GSM()
{
//...
    Pin = NULL; 
//...
}

Now, I want to set default value ("1234") to my PIN. I tried very simple way:

bool GSM::setDefaultValue()
{
    lock();
    Pin = "0";
    for (uint8 i =0; i < 4; ++i)
    {
        Pin[i] = i+1;
    }
    unlock();
    return true;
}

But it didn't work. When I run my program (I use Visual Studio 2010) there is an error:

Access violation writing location 0x005011d8

I tried to remove line

Pin = "0";

But it didn't help. I have to initialize it as NULL in constructor. It's part of a larger project, but I think, the code above is what makes me trouble. I'm still pretty new in C++/OOP and sometimes I still get confused by pointers.

What should I do to improve my code and the way I think?
EDIT: As requested, I have to add that I can't use std::string. I'm trainee at company, project is pretty big (like thousands of files) and I did not see any std here and I'm not allowed to use it.

Upvotes: 0

Views: 286

Answers (3)

Baldrickk
Baldrickk

Reputation: 4409

Simple answer:

Instead of using the heap (new delete) just allocate space in your class for the four character pin:

Class GSM
{
//...
private:
    char Pin[5];
//...
}

The length is fixed at 5 (to allow space for 4 characters and terminating null ('\0'), but as long as you only need to store a maximum of 4 characters, you are fine.

Of course, if you want to make it easy to change in the future:

Class GSM
{
//...
private:
    const int pin_length = 4;
    char Pin[pin_length+1];
//...
}

Your function to set the value will then look like:

bool GSM::setDefaultValue()
{
    lock();
    for (char i = 0; i < pin_length; ++i)
    {
        Pin[i] = i+1;
    }
    Pin[pin_length]=0;
    unlock();
    return true;
}

or even:

bool GSM::setDefaultValue()
{
    lock();
    strcpy(Pin,"1234");  //though you would have to change this if the pin-length changes.
    unlock();
    return true;
}

Upvotes: 0

Mats Petersson
Mats Petersson

Reputation: 129464

You need to give the Pin some memory. Something like this:

Pin = new char[5];  // To make space for terminating `\0`;
for(...)
{
   Pin[i] = '0' + i + 1;
}
Pin[4] = '\0';      // End of the string so we can use it as a string.

...

You should then use delete [] Pin; somewhere too (Typically in the destructor of the class, but depending on how it's used, it may be needed elsewhere, such as assignment operator, and you need to also write a copy-constructor, see Rule Of Three).

In proper C++, you should use std::string instead, and you could then do:

Class GSM
{
//...
private:
    std::string Pin;
....

Pin = "0000";
for (uint8 i =0; i < 4; ++i)
{
    Pin[i] += i+1;
}

Using std::string avoids most of the problems of allocating/deallocating memory, and "just works" when you copy, assign or destroy the class - because the std::string implementation and the compiler does the work for you.

Upvotes: 4

GHugo
GHugo

Reputation: 2654

You need to allocate a block of memory to store "1234". This memory block will be pointed by your Pin pointer. You can try:

bool GSM::setDefaultValue()
{
    lock();
    Pin = new char[4];
    for (uint8 i =0; i < 4; ++i)
    {
        Pin[i] = '0' + (i + 1);
    }
    unlock();
    return true;
}

As you have allocated dynamicaly a memory block, you should always release it when you don't need it anymore. To do so, you should add a destructor to your class:

GSM::~GSM()
{
    delete [] Pin;
}

Upvotes: 1

Related Questions