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.

I have developed an application that interacts with IBM ClearQuest. The problem is that when I run everything locally, such as, run the webservice local and then ASP page local, everything is at the speed I expect. When I post the webservice (precompiled) to the server and run the web page through the server, the call to the webmethod takes at least 10x the amount of time it should. I don't know why this is happening. I made a console application that has the function in question and executes it on the server and locally, and they both return the same amounts of time (roughly). It's just when I move to executing via the webmethod that everything grinds to a snails pace.

Any ideas? This happens every time not just on the first call.

Maybe a code review might help here?

WebMethod (VB):

Public Function RetrieveQueryResults(ByRef cqSession As ClearQuestOleServer.Session, _
                                     ByVal sqlStmt As String) As List(Of SearchResultsSingleIssue)

Dim numCols As Integer, status As Integer, columnIdx As Integer
Dim numRows As Integer
Dim rowContents As String = ""
Dim colValue As New Object
Dim colLabel As New Object
Dim allitems As New List(Of SearchResultsSingleIssue)
Dim results As New SearchResultsSingleIssue
Dim cqResultSet As ClearQuestOleServer.OAdResultset

cqResultSet = cqSession.BuildSQLQuery(sqlStmt)
cqResultSet.Execute()

' Get the number of columns returned by the query.
numRows = 0
numCols = cqResultSet.GetNumberOfColumns
status = cqResultSet.MoveNext


' Collect query results.
Do While status = AD_SUCCESS
    results = New SearchResultsSingleIssue
    numRows = numRows + 1

    For columnIdx = 1 To numCols

        colLabel = cqResultSet.GetColumnLabel(columnIdx)
        colValue = cqResultSet.GetColumnValue(columnIdx)

        'Make sure that we dont pass along a null reference
        If colValue = Nothing Then
            colValue = ""
        End If

        Select Case colLabel
            Case "ID"
                results.IssueID = colValue
            Case "HEADLINE"
                results.Headline = colValue
            Case "NAME"
                results.State = colValue
            Case "OE_CONTACT"
                results.OEContact = colValue
            Case "DESCRIPTION"
                results.Further_Description = colValue
            Case "PRODUCT_NAME"
                results.Product_Name = colValue
            Case "FUNCTIONAL_AREA"
                results.Functional_Area = colValue
            Case "SUBTOPIC"
                results.Subtopic = colValue
            Case "FOUND_VERSION"
                results.Found_In = colValue
            Case "SCHEDULED_VERSION"
                results.Scheduled_For = colValue
            Case "SYMPTOMS"
                results.Symptoms = colValue
            Case "AFFECTED_SYSTEMS"
                results.Affected_System_Types = colValue
            Case "ISSUE_TYPE"
                results.Issue_Type = colValue
            Case "ASSIGNED_TO"
                results.Assigned_Developer = colValue
            Case "TESTED_BY"
                results.Assigned_Tester = colValue
            Case "BUILT_VERSION"
                results.Built_In = colValue
            Case "TESTED_VERSION"
                results.Tested_In = colValue
            Case "NOTES_LOG"
                results.Notes_Log = colValue
            Case "CUSTOMER_SEVERITY"
                results.Severity = colValue
            Case "PRIORITY"
                results.Priority = colValue

        End Select

    Next columnIdx

    ' Add the query row result to the compiled list of all rows.
    allitems.Add(results)
    status = cqResultSet.MoveNext

Loop

Return allitems

End Function

Local Windows Application Method (C#):

private void button2_Click(object sender, EventArgs e)
{
    start = DateTime.Now.TimeOfDay.Seconds;

    int numCols = 0;
    int status = 0;
    int columnIdx = 0;
    int numRows = 0;
    string rowContents = "";
    string colValue;
    string colLabel;
    List<SearchResultsSingleIssue> allitems = new List<SearchResultsSingleIssue>();
    SearchResultsSingleIssue results = new SearchResultsSingleIssue();
    ClearQuestOleServer.OAdResultset cqResultSet = default(ClearQuestOleServer.OAdResultset);

    cqResultSet = (ClearQuestOleServer.OAdResultset)ClearQuestSession.BuildSQLQuery(sqlStatement); 
    cqResultSet.Execute();

    // Get the number of columns returned by the query.
    numRows = 0;
    numCols = cqResultSet.GetNumberOfColumns();
    status = cqResultSet.MoveNext();


    // Collect query results.
    while (status == 1)
    {
        results = new SearchResultsSingleIssue();
        numRows = numRows + 1;


        for (columnIdx = 1; columnIdx <= numCols; columnIdx++)
        {
            colLabel = (string)cqResultSet.GetColumnLabel(columnIdx);
            colValue = (string)cqResultSet.GetColumnValue(columnIdx);
            //Make sure that we dont pass along a null reference
            if (colValue == null)
            {
                colValue = "";
            }

            switch (colLabel)
            {
                case "ID":
                    results.IssueID = colValue;
                    break;
                case "HEADLINE":
                    results.Headline = colValue;
                    break;
                case "NAME":
                    results.State = colValue;
                    break;
                case "OE_CONTACT":
                    results.OEContact = colValue;
                    break;
                case "DESCRIPTION":
                    results.Further_Description = colValue;
                    break;
                case "PRODUCT_NAME":
                    results.Product_Name = colValue;
                    break;
                case "FUNCTIONAL_AREA":
                    results.Functional_Area = colValue;
                    break;
                case "SUBTOPIC":
                    results.Subtopic = colValue;
                    break;
                case "FOUND_VERSION":
                    results.Found_In = colValue;
                    break;
                case "SCHEDULED_VERSION":
                    results.Scheduled_For = colValue;
                    break;
                case "SYMPTOMS":
                    results.Symptoms = colValue;
                    break;
                case "AFFECTED_SYSTEMS":
                    results.Affected_System_Types = colValue;
                    break;
                case "ISSUE_TYPE":
                    results.Issue_Type = colValue;
                    break;
                case "ASSIGNED_TO":
                    results.Assigned_Developer = colValue;
                    break;
                case "TESTED_BY":
                    results.Assigned_Tester = colValue;
                    break;
                case "BUILT_VERSION":
                    results.Built_In = colValue;
                    break;
                case "TESTED_VERSION":
                    results.Tested_In = colValue;
                    break;
                case "NOTES_LOG":
                    results.Notes_Log = colValue;
                    break;
                case "CUSTOMER_SEVERITY":
                    results.Severity = colValue;
                    break;
                case "PRIORITY":
                    results.Priority = colValue;

                    break;
            }

        }

        // Add the query row result to the compiled list of all rows.
        allitems.Add(results);
        status = cqResultSet.MoveNext();


    }

    seconds = (DateTime.Now.TimeOfDay.Seconds - start);
    label3.Text = seconds.ToString();



}

The code executes in about...6 seconds.

share|improve this question
3  
Try profiling your application on server. –  Snowbear Mar 12 '11 at 19:52
1  
Code Review can only help when you have zeroed down upon the source of slowness. Please set up a profiler (such as jetbrains.com/profiler) and get the results. –  NRS Nov 6 '11 at 11:16
    
the first block of code is VB not C# –  Malachi Nov 28 '13 at 23:03
    
To eliminate the obvious - are you trying the web method multiple times? Is the web method 10x slower upon every execution when you do? It can take some time to spool up a worker process in IIS, so your results would be skewed if you do not take this into account. –  Dan Lyons Dec 31 '13 at 18:11
    
@Sean P: Also try using a logging framework like NLog or log4net and write output at each point. These frameworks can be configured to output at a millisecond level. –  Dominic Zukiewicz Feb 13 '14 at 13:12

1 Answer 1

This being the oldest zombie on the site, I thought the code could use a bit of reviewing. I don't think I can answer the question about the weird performance issue though.

VB

Declarations

You're not consistent with your declaration style:

Dim numCols As Integer, status As Integer, columnIdx As Integer

While this is correct and avoids the common mistake of only declaring a type to the last variable declared, I don't like multiple declarations on the same line. It's more readable when they're separated. These three being declared on the same line just seems arbitrary.

The variables should be declared as close as possible to their usage, this block of declarations at the top of your method isn't helping readability either.

This line serves no purpose and can be eliminated:

numRows = 0

Loop

You're fetching the first result before iterating the result set:

status = cqResultSet.MoveNext

' Collect query results.
Do While status = AD_SUCCESS

And then your loop ends like this:

    status = cqResultSet.MoveNext
Loop

I think you can eliminate the status identifier along with the first check, and do this instead:

Do While cqResultSet.MoveNext = AD_SUCCESS

The loop doesn't need to enter the Select Case block if colValue is Nothing - I'm more of a C# guy so this could be wrong, but I think the Continue keyword could be used here to skip that iteration right there:

'Make sure that we dont pass along a null reference
If colValue = Nothing Then Continue

Not sure I like the Select Case here, and I'm not sure the looping over returned columns is buying you anything either. I don't know this API, but if this were ADO.NET I'd recommend you get the value for each column name, and assign it to result if that column name exists, like this (syntax is probably wrong, and C#-like, but you get the idea):

if (cqResultSet["ID"] != null) results.IssueId = cqResultSet["ID"].Value;

The point, is that you're naming all columns and all properties anyway, so the loop+switch isn't buying you anything except the need to now track what column index you're at.

Naming

I don't like the underscore in identifiers. Stick to PascalCase convention, be consistent. Also results would be a better name for allItems (Return allItems becomes Return result), and result would be a better name for results (allItems.Add(results) becomes results.Add(result)).

If AD_SUCCESS is a status code, this naming makes me think it's a constant. However status codes are a suite of closely related constants, usually mutually exclusive - this looks like a job for an Enum.

C#

Most of the above also applies to the C# code. Both snippets do the same thing and the code roughly seems pretty equivalent, so I'm going to keep this review DRY and won't mention again to declare variables as close as possible to their usage ;)

while (status == 1)

What happened to the constant? Are magic numbers any less magical in C# than in VB?

Timing

You're timing your code the VB6 way, in both snippets. Take a look at the StopWatch class, which is much better at this kind of task (be it just for the ElapsedMilliseconds property!).

share|improve this answer
    
+1 ! Also, possibly refactor the switch to a method like ApplyValueToResult(SearchResultsSingleIssue result, string label, object value) so the for loop is short and sweet. –  Dominic Zukiewicz Feb 13 '14 at 13:16
    
@DominicZukiewicz thanks! I would actually eliminate the entire loop + switch by calling the columns by their name, if that's at all possible - like if (cqResultSet["ID"] != null) results.IssueId = cqResultSet["ID"].Value;... I don't like looping over columns, we're already looping over returned rows. –  Mat's Mug Feb 13 '14 at 13:30
    
Looping over rows and columns is fine. Its just a necessary evil with DB interaction at this lower level; it bloats code, but as long as its refactored with a clearer purpose, its good to do. Ideally, you'd use a dedicated class to do this, which abstracts the DB layer, but this question is about why the times are not correct, not how can the OP improve the code structure :-( –  Dominic Zukiewicz Feb 13 '14 at 13:41

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.