Code Review Needed

jkane · December 17, 2024, 4:56pm

I am building a Function that creates an array of values that will be used as the key in a Dictionary. I am passing a string that is delimited and also splitting that into an array. I then create the Dictionary with the delimited string as the values. Testing it out returns a 500 error with “We apologize, but an unexpected internal problem occurred. Please provide your System Admin or Technical Support this Correlation ID for further details.”

Can anyone see anything that might be wrong? I do not get any errors when checking in the Function.

Dictionary<string, string> pkColl = new Dictionary<string, string>();

string[] arrayCol = {"ASME", "ASMETbl", "BeamSize", "ChnSize", "Class", "Color", "Cond", "Dash", "Degree", "Dia", "Drive", "Duro", "Elast", "Finish", "Fit", "Flange", "Gauge", "Grade", "Head", "HW", "Inlet", "Length", "LengthE", "Manufact", 
    "Material", "Metal", "NPS", "OD", "Outlet", "Sched", "Seal", "Shape", "Standard", "Style", "Thick", "ThickE", "Thread", "Valve", "Wall", "AFlat", "Height", "Leg1", "Leg2", "Width"};

string[] delPartKey = partKey.Split('~');

foreach (string element in arrayCol)
{
  int x = 0;
  
  pkColl.Add(arrayCol[x], delPartKey[x]);
  
  x++;
}

string udPartDesc = "";

if (!string.IsNullOrEmpty(pkColl["Fit"]))
{
  udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "Fit" && ud.CodeID == pkColl["Fit"]).ToString();
  partDesc = udPartDesc;
}

if (!string.IsNullOrEmpty(pkColl["Flange"]))
{
  udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "Flange" && ud.CodeID == pkColl["Flange"]).ToString();
  partDesc += ", " + udPartDesc;
}

if (!string.IsNullOrEmpty(pkColl["HW"]))
{
  udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "HW" && ud.CodeID == pkColl["HW"]).ToString();
  partDesc += ", " + udPartDesc;
}

if (!string.IsNullOrEmpty(pkColl["Metal"]))
{
  udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "Metal" && ud.CodeID == pkColl["Metal"]).ToString();
  partDesc += ", " + udPartDesc;
}

if (!string.IsNullOrEmpty(pkColl["Seal"]))
{
  udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "Seal" && ud.CodeID == pkColl["Seal"]).ToString();
  partDesc += ", " + udPartDesc;
}

if (!string.IsNullOrEmpty(pkColl["Valve"]))
{
  udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "Valve" && ud.CodeID == pkColl["Valve"]).ToString();
  partDesc += ", " + udPartDesc;
}

if (!string.IsNullOrEmpty(pkColl["Shape"]))
{
  udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "Shape" && ud.CodeID == pkColl["Shape"]).ToString();
  partDesc += ", " + udPartDesc;
}

if (!string.IsNullOrEmpty(pkColl["Manufact"]))
{
  udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "Manufact" && ud.CodeID == pkColl["Manufact"]).ToString();
  partDesc += ", " + udPartDesc;
}

if (!string.IsNullOrEmpty(pkColl["Style"]))
{
  udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "Style" && ud.CodeID == pkColl["Style"]).ToString();
  partDesc += ", " + udPartDesc;
}

if (!string.IsNullOrEmpty(pkColl["Drive"]))
{
  udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "Drive" && ud.CodeID == pkColl["Drive"]).ToString();
  partDesc += ", " + udPartDesc;
}

if (!string.IsNullOrEmpty(pkColl["Head"]))
{
  udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "Head" && ud.CodeID == pkColl["Head"]).ToString();
  partDesc += ", " + udPartDesc;
}

if (!string.IsNullOrEmpty(pkColl["Thread"]))
{
  udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "Thread" && ud.CodeID == pkColl["Thread"]).ToString();
  partDesc += ", " + udPartDesc;
}

if (!string.IsNullOrEmpty(pkColl["Degree"]))
{
  udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "Degree" && ud.CodeID == pkColl["Degree"]).ToString();
  partDesc += ", " + udPartDesc;
}

if (!string.IsNullOrEmpty(pkColl["Dia"]))
{
  udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "Dia" && ud.CodeID == pkColl["Dia"]).ToString();
  partDesc += ", " + udPartDesc;
}

if (!string.IsNullOrEmpty(pkColl["Gauge"]))
{
  udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "Gauge" && ud.CodeID == pkColl["Gauge"]).ToString();
  partDesc += ", " + udPartDesc;
}

if (!string.IsNullOrEmpty(pkColl["Thick"]))
{
  if (pkColl["Thick"] == "24")
  {
    partDesc += ", " + pkColl["ThickE"] + "\" THKNS";
  }
  else
  {
    udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "Thick" && ud.CodeID == pkColl["Thick"]).ToString();
    partDesc += ", " + udPartDesc;
  }
}

if (!string.IsNullOrEmpty(pkColl["OD"]))
{
  udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "OD" && ud.CodeID == pkColl["OD"]).ToString();
  partDesc += ", " + udPartDesc;
}

if (!string.IsNullOrEmpty(pkColl["NPS"]))
{
  udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "NPS" && ud.CodeID == pkColl["NPS"]).ToString();
  partDesc += ", " + udPartDesc;
}

if (!string.IsNullOrEmpty(pkColl["Wall"]))
{
  udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "Wall" && ud.CodeID == pkColl["Wall"]).ToString();
  partDesc += ", " + udPartDesc;
}

if (!string.IsNullOrEmpty(pkColl["BeamSize"]))
{
  udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "BeamSize" && ud.CodeID == pkColl["BeamSize"]).ToString();
  partDesc += ", " + udPartDesc;
}

if (!string.IsNullOrEmpty(pkColl["ChnSize"]))
{
  udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "ChnSize" && ud.CodeID == pkColl["ChnSize"]).ToString();
  partDesc += ", " + udPartDesc;
}

if (!string.IsNullOrEmpty(pkColl["Length"]))
{
  if (pkColl["Length"] == "20")
  {
    partDesc += ", " + pkColl["LengthE"] + "\" LG";
  }
  else
  {
    udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "Length" && ud.CodeID == pkColl["Length"]).ToString();
    partDesc += ", " + udPartDesc;
  }
}

if (!string.IsNullOrEmpty(pkColl["Width"]))
{
  partDesc += ", " + pkColl["Width"] + "\" WD";
}

if (!string.IsNullOrEmpty(pkColl["Height"]))
{
  partDesc += ", " + pkColl["Height"] + "\" HGT";
}

if (!string.IsNullOrEmpty(pkColl["AFlat"]))
{
  partDesc += ", " + pkColl["AFlat"] + "\" ACROSS FLAT";
}

if (!string.IsNullOrEmpty(pkColl["Leg1"]))
{
  partDesc += ", " + pkColl["Leg1"] + "\" LEG 1";
}

if (!string.IsNullOrEmpty(pkColl["Leg2"]))
{
  partDesc += ", " + pkColl["Leg2"] + "\" LEG 2";
}

if (!string.IsNullOrEmpty(pkColl["Sched"]))
{
  udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "Sched" && ud.CodeID == pkColl["Sched"]).ToString();
  partDesc += ", " + udPartDesc;
}

if (!string.IsNullOrEmpty(pkColl["Material"]))
{
  udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "Material" && ud.CodeID == pkColl["Material"]).ToString();
  partDesc += ", " + udPartDesc;
}

if (!string.IsNullOrEmpty(pkColl["Grade"]))
{
  udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "Grade" && ud.CodeID == pkColl["Grade"]).ToString();
  partDesc += ", " + udPartDesc;
}

if (!string.IsNullOrEmpty(pkColl["Cond"]))
{
  udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "Cond" && ud.CodeID == pkColl["Cond"]).ToString();
  partDesc += ", " + udPartDesc;
}

if (!string.IsNullOrEmpty(pkColl["Finish"]))
{
  udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "Finish" && ud.CodeID == pkColl["Finish"]).ToString();
  partDesc += ", " + udPartDesc;
}

if (!string.IsNullOrEmpty(pkColl["Dash"]))
{
  udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "Dash" && ud.CodeID == pkColl["Dash"]).ToString();
  partDesc += ", " + udPartDesc;
}

if (!string.IsNullOrEmpty(pkColl["Duro"]))
{
  udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "Duro" && ud.CodeID == pkColl["Duro"]).ToString();
  partDesc += ", " + udPartDesc;
}

if (!string.IsNullOrEmpty(pkColl["Elast"]))
{
  udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "Elast" && ud.CodeID == pkColl["Elast"]).ToString();
  partDesc += ", " + udPartDesc;
}

if (!string.IsNullOrEmpty(pkColl["Color"]))
{
  udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "Color" && ud.CodeID == pkColl["Color"]).ToString();
  partDesc += ", " + udPartDesc;
}

if (!string.IsNullOrEmpty(pkColl["Inlet"]))
{
  udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "Inlet" && ud.CodeID == pkColl["Inlet"]).ToString();
  partDesc += ", " + udPartDesc;
}

if (!string.IsNullOrEmpty(pkColl["Outlet"]))
{
  udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "Outlet" && ud.CodeID == pkColl["Outlet"]).ToString();
  partDesc += ", " + udPartDesc;
}

if (!string.IsNullOrEmpty(pkColl["Class"]))
{
  udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "Class" && ud.CodeID == pkColl["Class"]).ToString();
  partDesc += ", " + udPartDesc;
}

if (!string.IsNullOrEmpty(pkColl["ASME"]))
{
  udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "ASME" && ud.CodeID == pkColl["ASME"]).ToString();
  partDesc += ", " + udPartDesc;
}

if (!string.IsNullOrEmpty(pkColl["ASMETbl"]))
{
  udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "ASMETbl" && ud.CodeID == pkColl["ASMETbl"]).ToString();
  partDesc += ", " + udPartDesc;
}

if (!string.IsNullOrEmpty(pkColl["Standard"]))
{
  udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "Standard" && ud.CodeID == pkColl["Standard"]).ToString();
  partDesc += ", " + udPartDesc;
}

klincecum · December 17, 2024, 5:05pm

I don’t have time to really review the code, but I would add another output variable to return an exception you can see.


try
{
    //code here
}
catch (Exception ex)
{
   ErrorOut = ex.ToString();
}

NateS · December 17, 2024, 5:20pm

chat gpt helped with this one. it found that you are declaring x=0 inside a for each loop. Here is gpt’s complete review of your code:

CHAT GPT

Here’s an evaluation of your code with potential issues identified and suggestions for improvement:

1. `x` Declaration and Usage Inside the Loop

Problem:
The int x = 0; is declared inside the foreach loop. This means x is reset to 0 every iteration of the loop, so pkColl.Add(arrayCol[x], delPartKey[x]); will always attempt to use x = 0.

Error:
You will end up repeatedly trying to use the first elements of arrayCol and delPartKey. This will cause a “Key already exists” exception when adding to pkColl more than once.

Fix: Move the x declaration outside the foreach loop and replace the foreach loop with a for loop since you need indexing:

csharp

Copy code

for (int x = 0; x < arrayCol.Length; x++)
{
    if (x < delPartKey.Length) // Safety check for index out of bounds
    {
        pkColl.Add(arrayCol[x], delPartKey[x]);
    }
}

2. Accessing `partKey`

Problem:
The line string[] delPartKey = partKey.Split('~'); assumes that partKey exists and has been initialized. If partKey is null or empty, it will throw a NullReferenceException or an empty string split may cause index issues later.

Fix: Add a null or empty check for partKey:

csharp

Copy code

if (string.IsNullOrEmpty(partKey))
{
    throw new ArgumentException("partKey cannot be null or empty");
}

string[] delPartKey = partKey.Split('~');

3. `Db.UDCodes.Select` Returns Invalid Output

Problem:
This line:

csharp

Copy code

udPartDesc = Db.UDCodes.Select(ud => ud.CodeTypeID == "Fit" && ud.CodeID == pkColl["Fit"]).ToString();

The Select method in LINQ does not filter items; it just projects them. Instead, you likely meant to use Where to filter and then extract the value you need.

Fix:
Use FirstOrDefault() or another method to retrieve a single matching result:

csharp

Copy code

udPartDesc = Db.UDCodes
                .Where(ud => ud.CodeTypeID == "Fit" && ud.CodeID == pkColl["Fit"])
                .Select(ud => ud.SomeProperty) // Replace with actual property name to fetch
                .FirstOrDefault();

SomeProperty should correspond to the column or field you want from UDCodes.

4. Potential Key Missing Exception

Problem:
If any key from arrayCol does not exist in pkColl, accessing it using pkColl["Key"] will throw a KeyNotFoundException.

Fix: Check if the key exists before accessing it:

csharp

Copy code

if (pkColl.ContainsKey("Fit") && !string.IsNullOrEmpty(pkColl["Fit"]))
{
    // Perform action
}

5. String Concatenation for `partDesc`

Problem:
Repeatedly concatenating strings using += inside loops or conditionals can cause performance issues due to creating multiple string instances.

Fix:
Use a StringBuilder to efficiently build partDesc:

csharp

Copy code

StringBuilder partDescBuilder = new StringBuilder();

if (pkColl.ContainsKey("Fit") && !string.IsNullOrEmpty(pkColl["Fit"]))
{
    udPartDesc = Db.UDCodes
                    .Where(ud => ud.CodeTypeID == "Fit" && ud.CodeID == pkColl["Fit"])
                    .Select(ud => ud.SomeProperty)
                    .FirstOrDefault();
    partDescBuilder.Append(udPartDesc);
}

// Append other conditions similarly...
partDesc = partDescBuilder.ToString();

6. Index Out of Bounds for `delPartKey`

Problem:
The delPartKey array may not have as many elements as arrayCol. If partKey does not have enough ~-separated values, delPartKey[x] will throw an IndexOutOfRangeException.

Fix: Add a length check before accessing delPartKey[x]:

csharp

Copy code

for (int x = 0; x < arrayCol.Length; x++)
{
    if (x < delPartKey.Length)
    {
        pkColl.Add(arrayCol[x], delPartKey[x]);
    }
    else
    {
        pkColl.Add(arrayCol[x], string.Empty); // Default to empty string if out of range
    }
}

Summary of Fixes

Replace the foreach loop with a for loop to properly manage the x index.
Validate partKey to avoid null or empty errors.
Replace Select with Where and FirstOrDefault() to retrieve filtered values.
Add key existence checks (ContainsKey) before accessing dictionary values.
Use StringBuilder for concatenating partDesc.
Add bounds checking when accessing delPartKey.

Let me know if you’d like me to rewrite the entire corrected code!

NateS · December 17, 2024, 5:21pm

I know the for each loop thing is an easy one, but I am not sure about the other suggestions. Take them with a grain of salt.

jkane · December 17, 2024, 5:37pm

I keep on forgetting about ChatGPT!

I’m sure this will get it working, just going to take a while to make all of the edits. I’ll be back in an hour or so.

Olga · December 17, 2024, 6:22pm

Check server log or event viewer for real exception text, it will help you to understand where the error is

jkane · December 17, 2024, 7:07pm

If only I had asked for them to be turned on.

klincecum · December 17, 2024, 7:09pm

They are on…

klincecum · December 17, 2024, 7:11pm

jkane · December 17, 2024, 7:14pm

Security setting?

klincecum · December 17, 2024, 7:16pm

Not sure what you’re asking. Cloud server logs are on by default now, and available where I showed.

Are yours not there?

jkane · December 17, 2024, 7:19pm

klincecum · December 17, 2024, 7:20pm

You chose the admin folder?

jkane · December 17, 2024, 7:21pm

Yes

klincecum · December 17, 2024, 7:21pm

I guess they forgot you. Time for a ticket.

jkane · December 17, 2024, 7:22pm

Already open.

jkane · December 17, 2024, 7:52pm

I guess I was too impatient? Took minutes to actually load

spaceage · December 17, 2024, 7:58pm

Oh it does, and we have tickets and PRBs for it. Literally takes 5-10 minutes to load for us.

jkane · December 17, 2024, 7:59pm

Thanks for letting me know I’m not alone!

kve · December 17, 2024, 8:40pm

I think you’d probably be better off looping through the dictionary pkColl instead of adding a code block for each entry. If you handle the edge cases where the string you add has more than just the dictionary value, it would be much more maintainable. I also recommend stringbuilder which is more efficient than adding to a string.

I think this script and handles the edge cases:

Dictionary<string, string> pkColl = new Dictionary<string, string>();

string[] arrayCol = {"ASME", "ASMETbl", "BeamSize", "ChnSize", "Class", "Color", "Cond", "Dash", "Degree", "Dia", "Drive", "Duro", "Elast", "Finish", "Fit", "Flange", "Gauge", "Grade", "Head", "HW", "Inlet", "Length", "LengthE", "Manufact", 
        "Material", "Metal", "NPS", "OD", "Outlet", "Sched", "Seal", "Shape", "Standard", "Style", "Thick", "ThickE", "Thread", "Valve", "Wall", "AFlat", "Height", "Leg1", "Leg2", "Width"};

string[] delPartKey = partKey.Split('~');

var EValues = new Dictionary<string,string> { {"ThickE", "24"}, {"LengthE", "20"} };

var suffixes = new Dictionary<string,string> {

    { "Thick" , "THNKNS"      },
    { "Width" , "WD"          },
    { "Height", "HGT"         },
    { "Length", "LG"          },
    { "AFlat" , "ACROSS FLAT" },
    { "Leg1"  , "LEG 1"       },
    { "Leg2"  , "LEG 2"       }
};


for ( int i = 0; i < arrayCol.Length; i++ )
{
    string dictValue = i < delPartKey.Length? delPartKey[i]: string.Empty;
    plColl.Add( arrayCol[i], dictValue );
}

var sbDesc = new StringBuilder();


foreach ( var entry in pkColl )
{
    if ( string.IsNullOrEmpty(entry.Key) ) continue;

    if ( EValues.ContainsKey(entry.Key) ) continue;


    if ( sbDesc.Length != 0 ) sbDesc.Append(", ");


    if ( EValues.ContainsKey(entry.Key + "E") && entry.Value == EValues[entry.Key + "E"] )
    {
        sbDesc.Append(pkColl[$"{entry.Key}E"])
              .Append(suffixes[entry.Key]);
        continue;
    }


    string udPartDesc = Db.UDCodes.Where(ud => ud.CodeTypeID == entry.Key && ud.CodeID == entry.Value)
                           .Select( s => s.CodeDesc ) // If you aren't after CodeDesc, change this field.
                           .FirstOrDefault()
                           .ToString();

    sbDesc.Append( udPartDesc );


    if ( suffixes.ContainsKey(entry.Key) )
    {
        sbDesc.Append( "\" " )
              .Append( suffixes[entry.Key] );
    }
}

partDesc = sbDesc.ToString();

Code Review Needed

CHAT GPT

1. x Declaration and Usage Inside the Loop

2. Accessing partKey

3. Db.UDCodes.Select Returns Invalid Output

4. Potential Key Missing Exception

5. String Concatenation for partDesc

6. Index Out of Bounds for delPartKey

Summary of Fixes

1. `x` Declaration and Usage Inside the Loop

2. Accessing `partKey`

3. `Db.UDCodes.Select` Returns Invalid Output

5. String Concatenation for `partDesc`

6. Index Out of Bounds for `delPartKey`