Help refactoring a method  
Author Message
NoEgo





PostPosted: Visual C# General, Help refactoring a method Top

I want to get rid of repetative code in the attached method I created (like in my If statements as you can obviously see, most is repeated but just for different product types) but not sure the best way to restrucure it:

http://www.webfound.net/forum_posts/repetative.txt



Visual C#20  
 
 
Harsimrat





PostPosted: Visual C# General, Help refactoring a method Top

You can use switch case, IMHO your if else is not that many....


 
 
guy kolbis





PostPosted: Visual C# General, Help refactoring a method Top

Hi,

I would use the Simple factory design pattern to create instance of Product.

And then I will create an interface named IProductLoader (for example).

I will create another abstract class named BaseProductLoader that implements IProductLoader and give basic implementation to the Load operation.

Then I will add 3 classes that inherit the BaseProductLoader:

SampleProductLoader, TrackProductLoader and AlbumProductLoader.

I hope this helps.

guy kolbis



 
 
FavorFlave





PostPosted: Visual C# General, Help refactoring a method Top

sorry, can I get some help with syntax for each....example maybe learning.

 
 
FavorFlave





PostPosted: Visual C# General, Help refactoring a method Top

guy kolbis
, for this particular program, we arleady have an abstract class and Interface being used. I don't want to add all that. This is gonna be reused but not in the sense that people can just reuse my classes like this as this is a data load and the incoming params change every time. We don't have a set template of data to use each time from Excel when our partners send us this sh&&.

 
 
James Curran





PostPosted: Visual C# General, Help refactoring a method Top

Well, as far as I can see, everything in each of the if (ProductType == ".....") block is identical, (except for the variable name, which is irrelevant), so keep the first one, delete the other two, and remove the if() at the start of it.

Note, that your current code has two bugs in it already. You need to assign a prAudioID value for Samples in the switch at the top, and put curly braces around the verbose logging block.

Actually, I spotted a couple more glaring errors, so I'm beginning to think that this is a homework assignment.




 
 
FavorFlave





PostPosted: Visual C# General, Help refactoring a method Top

Dude, first off...this is code in progress.  As you or any other developer knows, this may not be "working code" yet....DUH!

homework assignement...kiss my a** as far as that comment goes.

Stick to my quesiton...not BS.

This is for a huge public e-commerce site so again, kiss my a**, I've been in IT for 8 years.

Yes, I'm taking that personally as you can tell because comments like yours is not necessary.  It's a stupid remark...and I'd advise not to get so****y.  People who say this kind of horsesh** have a huge ego and I get tired of it.

A few more things.  Yes, I do need my If statement because it's product specific...and I do not want to insert a product 3 times if it's only one type of product.

You dont' know the process.  I asked how I can refactor.  You are half trying and assumeing this or that.  If you hae specific quesitons on my process, ask.

Now, I need to check whether it's an album, track, etc. but do not want to repeat the values as I have stated..they're the same values mostly but dependent on the prAudioID and depedent on Type to determine if to call the Load function.

>>>except for the variable name, which is irrelevant

How do you know this.  It is relevant as I have prAudioID being set to a different value depending on the ProductType if you would have looked at it closer.



 
 
FavorFlave





PostPosted: Visual C# General, Help refactoring a method Top

the ID is irrelevant   What is this then:

   switch (ProductType.Trim() + "|" + FileExtension.Trim())
{
case "Album|mp3":
prAudioID = "AL" + track_Id;
FileName = FileName + " Album (MP3)";
break;
case "Album|wav":
prAudioID = "AL" + track_Id;
FileName = FileName + " Album (WAV)";
break;
case "Track|mp3":
prAudioID = "TM" + track_Id;
FileName = FileName + " (MP3)";
break;
case "Track:wav":
prAudioID = "TW" + track_Id;
FileName = FileName + " (WAV)";
break;
case "Sample:mp3":
FileName = FileName + " (MP3)";
break;

what the hell does this do...hmm, it changes its value based on type....so it IS relevant.  Therefore what I'm saying is, the if statement is needed and the variable DOES matter.



 
 
FavorFlave





PostPosted: Visual C# General, Help refactoring a method Top

Conclusion, don't assume this or that, just stick to the help the person has asked and keep the side comments neutral. Not everyone has working code, we're all at different stages in our code as you well should know.

 
 
FavorFlave





PostPosted: Visual C# General, Help refactoring a method Top

I'll go with a switch case...good idea, adios everyone. thanks

 
 
James Curran





PostPosted: Visual C# General, Help refactoring a method Top

>>>except for the variable name, which is irrelevant

How do you know this. It is relevant as I have prAudioID being set to a different value depending on the ProductType if you would have looked at it closer.

I know the variable name is irrelevant because all variable names are irrelevant. ie. You have code written like this:

Product JAA = null;
JAA = ProductAbstraction.Load(prAudioID, sstore.SellerStoreID, out created, TransactionAbstraction.GetTransaction());

But it would not matter a whit if you had written:

Product aaJ = null;
aaJ = ProductAbstraction.Load(prAudioID, sstore.SellerStoreID, out created, TransactionAbstraction.GetTransaction());

or

Product XXX = null;
XXX = ProductAbstraction.Load(prAudioID, sstore.SellerStoreID, out created, TransactionAbstraction.GetTransaction());

because varaible names are irrelevant.

So, in you code you have:

Product JAA = null;
Product JAT = null;
Product JAS = null;

But you will only create one of those at a time, so you only need one. So, let's get rid of two of them:
Product JA = null;

Now rename all "JAA", "JAT" and "JAS" to "JA". You'll now see the EVERYTHING inside each of the if() block is exactly identical. In other words, you code is essentially:

if (ProductType == "Album")
DoStuff();
else if (ProductType == "Track")
DoStuff();
else if (ProductType == "Sample")
DoStuff();

which is the same as

if ((ProductType == "Album") || (ProductType == "Track") || (ProductType == "Sample"))
DoStuff();

Or, since ProductType can only ever be "Album", "Track" or "Sample", simply:
DoStuff();

Now, in you example, "DoStuff()" is actually 50 lines of code, but the principle is the same.

Now, you say the Load functions are different --- but they are not. All three take exactly the same parameters. The values of those parameters may be different, but that's irrelevant. (That's pretty much the purpose of parameters)

Finally, as for my "Homework" comment, let's examine the evidence:

  1. Everyday, students post their homework assignments on forums like this, hoping to get someone to do their work for them.
  2. You posted a block of code which could clearly be refactored, which you had no idea how to refactor, but which you nevertheless knew could be refactored. How would you knew that the code could be refactor, when you clearly have no idea how to refactor it Reasonable assumption: Someone told you "Refactor this code".
  3. The code you presented contains serveral glaring errors, things someone who's "been in IT for 8 years" should have spotted immedately --- Including writing a logging block, with indents, but forgetting the braces around it --- THREE TIMES! Reasonable assumption: Someone took a functioning block of code, and delibarately added bugs, as an exercise for the student.