Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Following code is supposed generate a new ID (PID) for a new client based on the client name's first letter and the existing IDs in the range which are stored in a database.

For ex lets say if the new client's name is "Abc" and if last ID in the database starting from letter A, is A200. Then the new ID should be A201.

This code compiles and runs giving the desired out put but I hope this could be optimized further.

Expecting your suggestions on improving this...

private void GeneratePIDforNewPoint(string pointName)
{
    string firstLetterOfPointName; // variable to hold first letter of "point name" being passed to this method
    string lastExistingPID; // variable to hold the last PID (most recent hence largest) from the list of PIDs retrieved from the database
    int lastDigitOfExistingPID; // variable to hold last digit of the "lastPID" (ex- digit '2' from PID 'F002')
    string finalPID; // variable to hold out put (the generated new PID for the new point)...


    firstLetterOfPointName = pointName[0]; // Get the fist letter of the point name...

    List<string> pidList=_ds.GetPIDs(firstLetterOfPointName); // Calling method to get the pid list from database

    if (pidList.Count >=1) //  At the end of this if block, numeric part of the new PID will be desided
    {
        pidList.Sort();
        lastExistingPID = pidList[pidList.Count - 1];
        lastDigitOfExistingPID = int.Parse(lastExistingPID[lastExistingPID.Length - 1].ToString());
    }
    else
    {
        lastDigitOfExistingPID = 0;
    }

    lastDigitOfExistingPID += 1;

    // Found digit will be converted to the pid format by attaching the starting letter and zeros to make the length.(format --> A001/ B099 / C100 etc.)


    finalPID= firstLetterOfPointName + String.Format("{0:000}", lastDigitOfExistingPID );
    _view.PID = finalPID;
}


 public List<string> GetPIDs(string firstLetterOfPointName)
        {
            string selectStatement = @"SELECT PID FROM point WHERE PID like @PID";
            List<string> pidList = new List<string>(); // List to store all retrived PIDs from the database...

            // Retrieve all existing PIDs starting with "letter" from the database
            using (SqlConnection sqlConnection = new SqlConnection(db.GetConnectionString))
            {
                using (SqlCommand sqlCommand = new SqlCommand(selectStatement, sqlConnection))
                {
                    sqlCommand.Parameters.Add("@PID", SqlDbType.VarChar).Value = firstLetterOfPointName + '%';
                    sqlConnection.Open();
                    using (SqlDataReader dataReader = sqlCommand.ExecuteReader())
                    {                        
                        // If reader has data, are added to the list
                        while (dataReader.Read())
                        {
                            pidList.Add(dataReader.GetString(0).Trim());
                        }
                        return pidList;
                    }
                }
            }
        }
share|improve this question
1  
Create a table create table charid { id char, count long }. Search for pointName[0] as id in the table, increment count and write it back while locking the row. If pointName[0] is not in the table, add it and set the count to 1 while locking the table. –  Bob Dalgleish Apr 30 at 0:43
add comment

1 Answer

up vote 3 down vote accepted

First, I would expect from a function named GeneratePID to return the generated ID and not to set it. (Nobody likes side-effects)

private string GeneratePIDforNewPoint(string pointName)
{
    if(pointName == null)
        return "error";         

    var storedPIDs = _ds.GetPIDs(pointName[0]);
    var newPID = 0;        
    if(storedPIDs.Count > 0) {
        var maximumStoredPID = _ds.GetPIDs(pointName[0]).Max();
        newPID = Int32.Parse(maximumStoredPID.substring(1)) + 1;
    }
    return pointName[0] + String.Format("{0:000}", newPID); 
}
share|improve this answer
    
+1 for avoiding side-effects! –  Mat's Mug Apr 29 at 19:56
    
Lack of side effects destroys the utility. Unless this code is single-threaded and non-reentrant, somehow you have to increment a count and/or store the new ID. –  Bob Dalgleish Apr 30 at 0:38
    
I think when using the count, problem might be, if a record deleted accidentally in the table? Ex: Lets say it had A001,A002 and A003. Now A002 was deleted. The above code will generate A003 as new PID and would raise an error( violation of key constrain). How to avoid this? –  Chathuranga Apr 30 at 3:26
1  
Okay, in that case you have to find the maximum PID. I've edited the code above. –  ceec Apr 30 at 7:02
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.