This method does so much!

[corgimonster]

I'm trying to debug something that several no-longer-present developers have created. This is just part of the misery.

[code language="C#"]
        public string ValidateSubTypeCommissionCode(int newPayeeId, int[]] selectedLocationIds)
        {  
            string retVal = string.Empty;  
        try
        {
            string locationIds = string.Empty;
            foreach (var locationId in selectedLocationIds)
            {
                locationIds += (locationIds == string.Empty ? locationId.ToString() : "," + locationId.ToString());
            }

            retVal = this.Repository.GetMissingCommissionCodeSubtype(newPayeeId, locationIds);
        }
        catch (Exception)
        {
            throw;
        }

        return retVal;
    }

[/code]

Actual C# developers, and probably others, may notice:

1. Why does GetMissingCommissionCodeSubtype take a joined string instead of the array? There's no reason for it here.
2. I guess String.Join was too hard of a concept. So was not using string concatenation at all, since that is taboo in C#. Don't worry, we don't care about the performance of the application. But, the ternary is nice and creative!
3. Nice try/catch there. At least they didn't use the exception variable name. Omitting the try/catch would be even better.
4. Why did we need the retVal variable again? Oh, we don't, since we can return things that aren't variables.
5. Great method name there. I would definitely expect it to return a comma-delimited string of identifiers. Do you see anything in the method indicating that? Nope, I didn't either.

Not the most serious complaints all told, I suppose, but come on. I'll leave rewriting this as a one-liner as an exercise for the reader.

[Evilweasil]

return this.Repository.GetMissingCommissionCodeSubtype(newPayeeId,String.Join(",", selectedLocationIds.Select(x => x.ToString()).ToArray()));

[TheCPUWizard]

Actual C# developers, and probably others, may notice:

1. The signature may have already been published
2. True
3. Alows for a breakpoint
4. Possibly, there are benefits to having a single return variable for certain types of work (think IL re-writers!)
5. Most likely true

[Renan]

@TheCPUWizard said:

Actual C# developers, and probably others, may notice:

1. The signature may have already been published
2. True
3. Alows for a breakpoint
4. Possibly, there are benefits to having a single return variable for certain types of work (think IL re-writers!)
5. Most likely true

3. Not necessary. You can check the exception on the debugger regardless of having it caught like that.

[Tacroy]

 @TheCPUWizard said:

Actual C# developers, and probably others, may notice:

1. The signature may have already been published
2. True
3. Alows for a breakpoint
4. Possibly, there are benefits to having a single return variable for certain types of work (think IL re-writers!)
5. Most likely true

 #5 actually isn't true at all; god only knows what it actually returns, because we don't know what kind of horrible satanic rituals  <font face="Lucida Console" size="2">Repository.GetMissingCommissionCodeSubtype</font> performs. TRWTF is that for some reason the function that does all the actual work expects a comma separated string of ints, instead of an array of ints; the rest is just naive implementation.

 

[TheCPUWizard]

@Renan said:

3. Not necessary. You can check the exception on the debugger regardless of having it caught like that.

Ok, without the try/catch/throw, have this method called from a dozen locations, have the caling method (up the chain where there is a "real" catch) each call dozens of methods. Then break ONLY when something called by tis method throws an exception. There are many exceptions being thrown, so "break on exception throw" is not viable.

[TheCPUWizard]

@Tacroy said:

 @TheCPUWizard said:

Actual C# developers, and probably others, may notice:

1. The signature may have already been published
2. True
3. Alows for a breakpoint
4. Possibly, there are benefits to having a single return variable for certain types of work (think IL re-writers!)
5. Most likely true

 #5 actually isn't true at all; god only knows what it actually returns, because we don't know what kind of horrible satanic rituals  <font size="2" face="Lucida Console">Repository.GetMissingCommissionCodeSubtype</font> performs. TRWTF is that for some reason the function that does all the actual work expects a comma separated string of ints, instead of an array of ints; the rest is just naive implementation.

My "most likely true" was in response to the sarcastic "great name for this method"  (i.e. I tend to agree that the method name is NOT really a very good choice given the other elements.

[Medinoc]

1. True, at least with these method names.
2. You can't String.Join() integers until .Net 3.5. Using a StringBuilder would have been nice, though.
3. I'm with TheCPUWizard here.
4. True, looks like SESE (single entry, single exit) abuse.
5. True.
   

[corgimonster]

@TheCPUWizard said:

Actual C# developers, and probably others, may notice:

1. The signature may have already been published
2. True
3. Alows for a breakpoint
4. Possibly, there are benefits to having a single return variable for certain types of work (think IL re-writers!)
5. Most likely true

@Medinoc said:

1. True, at least with these method names.
2. You can't String.Join() integers until .Net 3.5. Using a StringBuilder would have been nice, though.
3. I'm with TheCPUWizard here.
4. True, looks like SESE (single entry, single exit) abuse.
5. True.

You're giving my predecessors far too much credit.

1. This is the only method that ever calls GetMissingCommissionCodeSubtype, and it is just in another assembly in the same solution. I understand why the method would need to concatenate a string internally, since it ends up as a parameter to a SQL Server 2005 stored procedure, and table-valued parameters were not yet available. However, there is no reason to soil the rest of the code with this hackery.
2. This code originated in .NET 3.5, so no excuse there.
3. So, take it out when you're done... Break-on-exception is a little clumsy in VS2008, though it would be less so if the code didn't throw stupid exceptions left and right.
4. Code standards? No one ever had those here. No one is doing anything inventive with this code.
5. No argument here...