Acquiring an ID from a newly inserted entry in an MSSQL database

In an MSSQL database, I have automatically increasing IDs for every entry.
Is it safe to acquire the ID of the newly inserted entry like this? I am worried that, if at the exact same time another entry gets inserted, I could end up with the wrong ID.

string query = "INSERT INTO subscriptions (polygon,...) VALUES (@polygon,...)";
                    SqlDataAdapter cmd = new SqlDataAdapter();  
                    using (SqlCommand querySaveSubscription = new SqlCommand(query))
                    {
                        querySaveSubscription.Connection = openCon;
                        // Create Poly out of given geodata values
                        var point = SqlGeography.Point(Double.Parse(lat), Double.Parse(lon), 4326);
                        SqlGeography poly = point.BufferWithTolerance(Double.Parse(radius) * 1000, 0.01, true);

                        var param = new SqlParameter(@"polygon", poly);
                        param.UdtTypeName = "Geography";

                        querySaveSubscription.Parameters.Add(param);
        ...

                        openCon.Open();
                        querySaveSubscription.ExecuteNonQuery();

I have the following c# code directly after my INSERT INTO - query getting executed:

query = "SELECT ID FROM subscriptions ORDER BY ID DESC";
                    SqlCommand command = new SqlCommand(query, openCon);
                    SqlDataReader reader = command.ExecuteReader();
                    reader.Read();

                    int id = reader.GetInt32(0);

                    result.Add("success", "true");

                    result.Add("id", id.ToString());

You can use scope_identity as @RobH hinted in a comment, but the sexiest way IMO is to use the OUTPUT clause of the INSERT syntax:

DECLARE @result TABLE (Id INT NOT NULL);

INSERT INTO subscriptions (polygon,...)
VALUES (@polygon,...)
OUTPUT INSERTED.Id INTO @result;

SELECT Id FROM @result;


Now, having a whole multi-statement CommandText like this, even if it's parameterized, isn't desirable at all, and makes for pretty ugly C# code - why not put that T-SQL in a stored procedure, and call that instead?

Speaking of parameters, your code snippet, as small as it is, is showing us that your method is breaking SRP (Single Responsibility Principle):

// Create Poly out of given geodata values
    var point = SqlGeography.Point(Double.Parse(lat), Double.Parse(lon), 4326);
    SqlGeography poly = point.BufferWithTolerance(Double.Parse(radius) * 1000, 0.01, true);

That logic doesn't belong there at all: you need a SqlGeography-type parameter, fine. That doesn't mean the logic for translating lat and lon values into one belongs in that method.

Side note, I get what lat and lon stand for in the context of creating a SqlGeography value, but these are bad variable names nonetheless: there's no reason to shorten the names like this - not even "it's faster to type"; with IntelliSense you type "lat" and it autocompletes "latitude" for you.

I'm going to make a little assumption here. This makes me think your method is returning something like a Dictionary:

result.Add("success", "true");
result.Add("id", id.ToString());

That's a very error-prone way of working. The "success" value is useless, because if you have a value, you have a successful insert. If you don't have a value, you're dealing with an exception - and I see no try/catch block in your code, so I'm hoping the caller is handling such an exception...

You could have a class somewhere, with properties for each column of a database table / entity you're working with, and you should return that instead... but the simplest return type to use would be a Nullable, so that the method either succeeded and returns the Id for the inserted record, or failed and returns null - for that to happen reliably, you'll need to wrap your logic in a try/catch block, and handle any SqlException that might be thrown in the process.

Another thing is that you're not properly disposing of your connection there. SqlConnection implements IDisposable, and the instance should be wrapped in a using block, too. Also I would try to open the connection before I start creating the command, so that I could pass the connection into the command's constructor (and not even bother instantiating it if the connection can't be opened). Keeps things cleaner I find:

try
{
    using (var connection = new SqlConnection(...))
    {
        connection.Open();
        using (var command = new SqlCommand(sql, connection))
        {
            //...
        }
    }
}
catch (SqlException)
{
    return null;
}

Lastly, I find that you're using confusing identifiers: in the top snippet your adapter is called cmd, and your command is called querySaveSubscription, as if it were a string containing the SQL statement (and the SQL string in question is called query... but doesn't query anything). ...but then in the bottom snippet a command is called a command and a reader is called a reader, and a connection that might never have successfully opened is called openCon. Would you ever work with a closedCon? Why not just call it connection then?

StackExchange Code Review Q#87336, answer score: 6