won't accept a null object in C#

Question

When I run my code it tells me that the object of adr is null, and thats true, but why wont it work when it works in a duplicate of the same method, with the exeption of insert instead of select.

the code looks like this:

public City doesExist(string postnr, string navn, City city, SqlConnection con)
{
    DatabaseConnection.openConnection(con);
    using (var command = new SqlCommand("select Id from [By] where Postnummer='" + postnr + "' and Navn='" + navn + "'", con))
    {
        command.Connection = con;
        SqlDataReader reader = command.ExecuteReader();
        if (reader.Read())
        {
            city.id = reader.GetInt32(0);
            city.postnr = postnr;
            city.navn = navn;
            reader.Close();

            return city;
        }

        reader.Close();
        return null;
    }
}

public City create(string postnr, string navn, City city, SqlConnection con)
{
    DatabaseConnection.openConnection(con);
    using (var command = new SqlCommand("insert into [By] (Postnummer, Navn) values ('" + postnr + "', '" + navn + "'); select @@identity as 'identity';", con))
    {
        object ID = command.ExecuteScalar();

        city.id = Convert.ToInt32(ID);
        city.postnr = postnr;
        city.navn = navn;
        return city;
    }
}

the call looks like this:

City city = new City();
city = city.doesExist(zip, by, city, connection); // this works fine
if (city == null)
{
     // I know that city is null
     // tried inserting City city = new City(); same error
     city = city.create(zip, by, city, connection); // this is where the null error occours
}

Why is the create method an instance method of the City class? Of course it will throw an exception. After all, city is null, isn't it? — vgru
– vgru, Commented Mar 6, 2013 at 13:28
Note that your doesExist method also takes an input city parameter which is unused, and should be removed. Actually, both of these methods look like repository methods, i.e. they should not be instance methods of a city class (because they don't belong to a single city instance). — vgru
– vgru, Commented Mar 6, 2013 at 13:31
warning your code may be vulnerable to sql injection attacks! — Daniel A. White
– Daniel A. White, Commented Mar 6, 2013 at 13:31

Parimal Raj · Accepted Answer · 2013-03-06 15:16:18Z

8

Well yes, look:

if (city == null)
{
    // If we've got in here, we know that city is a null reference, but...
    city = city.create(...);
}

You're calling a method on a reference which is definitely null. That's guaranteed to throw a NullReferenceException.

You almost certainly want to make your create method static (and rename it to comply with normal .NET naming conventions), and call it as

city = City.Create(...);

You'll also need to remove the city parameter from the method call, and instead create a new City object inside your method. For example:

public static City Create(string postnr, string navn, SqlConnection con)
{
    DatabaseConnection.openConnection(con);
    using (var command = new SqlCommand
         ("insert into [By] (Postnummer, Navn) values (@postnr, @navn); "+
          "select @@identity as 'identity';", con))
    {
        command.Parameters.Add("@postnr", SqlDbType.NVarChar).Value = postnr;
        command.Parameters.Add("@navn", SqlDbType.NVarChar).Value = navn;
        object ID = command.ExecuteScalar();

        City = new City();
        city.id = Convert.ToInt32(ID);
        city.postnr = postnr;
        city.navn = navn;
        return city;
    }
}

Note how I've changed your code to use parameterized SQL too. You really, really, shouldn't put values directly into your SQL statement like that - it opens up your system to SQL injection attacks and makes various conversions messy.

Additionally, I would recommend creating a new SqlConnection (and closing it) for each database operation.

Frankly it's a bit odd for doesExist to be an instance method, too... and again, for it to take a city parameter.

I would suggest changing the design of this so that you have a CityRepository (or something similar) which knows the connection string, and exposes:

// I'd rename these parameters to be more meaningful, but as I can't work out what they're
// meant to mean now, it's hard to suggest alternatives.
public City Lookup(string postnr, string nav)

public City Create(string postnr, string nav)

The repository would know the relevant connection string, and would be responsible for all the database operations. The City type would know nothing about databases.

edited Mar 6, 2013 at 15:16

Parimal Raj

20.6k9 gold badges76 silver badges113 bronze badges

answered Mar 6, 2013 at 13:29

Jon Skeet

1.5m893 gold badges9.3k silver badges9.3k bronze badges

Sign up to request clarification or add additional context in comments.

4 Comments

vgru Over a year ago

...and probably remove the city parameter from the method as well.

RBarryYoung Over a year ago

+1: for removing the Sql Injection vulnerability at the same time.

Michael Tot Korsgaard Over a year ago

Wont closing and reopening a database connection in every method slow the loading process, instead of opening one connectione, do all the call, and then closing it?

Jon Skeet Over a year ago

@MichaelTotKorsgaard: No - the connection pool will handle the "real" connections for you, and it will make sure you don't end up trying to do two things on the same connection at the same time.

Parimal Raj · Accepted Answer · 2013-03-06 13:31:15Z

0

You are trying to call a method on object class that has not been initialized/is null.

You need to make City.create a static member.

public static City create(string postnr, string navn, City city, SqlConnection con)
{
    DatabaseConnection.openConnection(con);
    using (var command = new SqlCommand("insert into [By] (Postnummer, Navn) values ('" + postnr + "', '" + navn + "'); select @@identity as 'identity';", con))
    {
        object ID = command.ExecuteScalar();

        city.id = Convert.ToInt32(ID);
        city.postnr = postnr;
        city.navn = navn;
        return city;
    }
}

And use it like :

if(city==null) 
{
    City city = City.create(... );
}

answered Mar 6, 2013 at 13:31

Parimal Raj

20.6k9 gold badges76 silver badges113 bronze badges

Comments

Xaruth · Accepted Answer · 2013-03-06 13:35:49Z

0

the better way :

 static public City create(string postnr, string navn, SqlConnection con)
 {
     DatabaseConnection.openConnection(con);
     using (var command = new SqlCommand("insert into [By] (Postnummer, Navn) values ('" + postnr + "', '" + navn + "'); select @@identity as 'identity';", con))
     {
         object ID = command.ExecuteScalar();

         City city = new City();
         city.id = Convert.ToInt32(ID);
         city.postnr = postnr;
         city.navn = navn;
         return city;
     }

     return null;
 }

Create method must be static, and no need to have city in argument. Call it with :

 if (city == null)
 {
      city = City.Create(.....);
 }

answered Mar 6, 2013 at 13:35

Xaruth

4,1083 gold badges22 silver badges27 bronze badges

Collectives™ on Stack Overflow

won't accept a null object in C#

3 Answers 3

4 Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

4 Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related