# Refactoring and creating separate methods

The below code works fine with no errors and no problems.

However, I think that it is too long, and I want to refactor it by separating the three parts (as commented on the code) by creating a new method for each of them, so that I can reuse them later.

I tried to separate Part 2 and created a method which takes an IQuerayble<anonymous> as its parameter, and called it with makbuzQuery as that parameter. However, when it is in a separate method, foreach does not see what is inside the IQueryable<anonymous>.

How can I refactor this code by creating separate methods for Part 1, Part 2 and Part 3?

public static IEnumerable MakbuzOlustur(int islemId)
{
//id si verilen tahsilat işleminin bilgilerini çekiyorm
//Part 1
var makbuzQuery = from islem in db.TBLP1ISLEMs
where islem.ID == islemId
select new
{
islem.ID,
MAKBUZTARIHI = DateTime.Now.ToShortDateString(),
islem.BELGENO,
islem.GENELTOPLAM,
CARININPARABIRIMI = islem.TBLP1CARI.LISTEPARABIRIMI,
islem.PARABIRIMI,
islem.ACIKLAMA,
};
//Part 2
List<Makbuz> makbuzListesi = new List<Makbuz>();

foreach (var makbuz in makbuzQuery)
{
Makbuz makbuzObjesi = new Makbuz();
//eğer işlemin kuru varsa çekiyorum
decimal isleminKuru = DovizTuruVerilenBirIsleminDovizKurunuCek(makbuz.ID, makbuz.PARABIRIMI);

makbuzObjesi.IslemId = makbuz.ID;
makbuzObjesi.MakbuzTarihi = makbuz.MAKBUZTARIHI;
makbuzObjesi.BelgeNo = makbuz.BELGENO;
//carinin para birimi TL ise kur ile çarpmaya gerek yok
if (makbuz.CARININPARABIRIMI == "TL")
{
makbuzObjesi.GenelToplam = makbuz.GENELTOPLAM.Value;
}
else
{
makbuzObjesi.GenelToplam = (makbuz.GENELTOPLAM * isleminKuru).Value;
}
makbuzObjesi.ParaBirimi = "TL";
makbuzObjesi.Yaziyla =
makbuzObjesi.Aciklama = makbuz.ACIKLAMA;

}
//Part 3
var toReport = from makbuzSon in makbuzListesi
select new
{
ID = makbuzSon.IslemId,
MAKBUZTARIHI = makbuzSon.MakbuzTarihi,
BELGENO = makbuzSon.BelgeNo,
GENELTOPLAM = DAL.Format.ParaDuzenle.ParaFormatDuzenle(makbuzSon.GenelToplam.ToString()) + " TL",
PARABIRIMI =makbuzSon.ParaBirimi,
YAZIYLA = makbuzSon.Yaziyla,
ACIKLAMA = makbuzSon.Aciklama,
};

}


This is the referred class

public class Makbuz
{
public int IslemId { get; set; }
public string FirmaBireyAdi { get; set; }
public string Adres { get; set; }
public string MakbuzTarihi { get; set; }
public string BelgeNo { get; set; }
public decimal GenelToplam { get; set; }
public string ParaBirimi { get; set; }
public string Yaziyla { get; set; }
public string Aciklama { get; set; }
public string PersonelAdi { get; set; }
}

• @M. Tibbits I didn't know that site Mr.Tibbits thank you. – JohnnyCageWins Jul 25 '11 at 21:08
• islem.TBLP1CARI.K_FIRMAADI == null || islem.TBLP1CARI.K_FIRMAADI == "" looks like it should be able to be replaced with string.IsNullOrEmpty(islem.TBLP1CARI.K_FIRMAADI). – Jesse C. Slicer Jul 28 '14 at 14:26

I think you are running into a fundamental problem I often have with anonymous types: they are anonymous. C++0x has some interesting extensions for getting types back from anonymous types, though I am not sure they would help here.

I suggest creating an explicit type to replace the anonymous type. This may cause early evaluation of your query.

public class MakbuzQT
{
public int ID;
public string MAKBUZTARIHI;
public string BELGENO;
public decimal GENELTOPLAM;
public string CARININPARABIRIMI;
public string PARABIRIMI;
public string YAZIYLA;
public string ACIKLAMA;
}


Now you can use the explicit type to refactor the query out:

    public static IEnumerable<MakbuzQT> Part1(int islemId) {
//id si verilen tahsilat işleminin bilgilerini çekiyorm
var makbuzQuery = from islem in db.TBLP1ISLEMs
where islem.ID == islemId
select new MakbuzQT {
ID = islem.ID,
MAKBUZTARIHI = DateTime.Now.ToShortDateString(),
BELGENO = islem.BELGENO,
GENELTOPLAM = islem.GENELTOPLAM,
CARININPARABIRIMI = islem.TBLP1CARI.LISTEPARABIRIMI,
PARABIRIMI = islem.PARABIRIMI,
ACIKLAMA = islem.ACIKLAMA,
};

return makbuzQuery;
}

public static List<Makbuz> Part2(IEnumerable<MakbuzQT> q) {
var ans = new List<Makbuz>();

foreach (var makbuz in q) {
Makbuz makbuzObjesi = new Makbuz();
//eğer işlemin kuru varsa çekiyorum
decimal isleminKuru = DovizTuruVerilenBirIsleminDovizKurunuCek(makbuz.ID, makbuz.PARABIRIMI);

makbuzObjesi.IslemId = makbuz.ID;
makbuzObjesi.MakbuzTarihi = makbuz.MAKBUZTARIHI;
makbuzObjesi.BelgeNo = makbuz.BELGENO;
//carinin para birimi TL ise kur ile çarpmaya gerek yok
if (makbuz.CARININPARABIRIMI == "TL") {
makbuzObjesi.GenelToplam = makbuz.GENELTOPLAM;
}
else {
makbuzObjesi.GenelToplam = (makbuz.GENELTOPLAM * isleminKuru);
}
makbuzObjesi.ParaBirimi = "TL";
makbuzObjesi.Aciklama = makbuz.ACIKLAMA;

}

return ans;
}

public static IEnumerable MakbuzOlustur(int islemId) {
List<Makbuz> makbuzListesi = Part2(Part1(islemId));

//Part 3
var toReport = from makbuzSon in makbuzListesi
select new {
ID = makbuzSon.IslemId,
MAKBUZTARIHI = makbuzSon.MakbuzTarihi,
BELGENO = makbuzSon.BelgeNo,
GENELTOPLAM = DAL.Format.ParaDuzenle.ParaFormatDuzenle(makbuzSon.GenelToplam.ToString()) + " TL",
PARABIRIMI = makbuzSon.ParaBirimi,
YAZIYLA = makbuzSon.Yaziyla,
ACIKLAMA = makbuzSon.Aciklama,
};

}


Part3 will have the same issue and you can create an explicit type if you want to refactor out the query into another method in the same way as part 2.

I run into this quite a bit and have yet to find what I think is a satisfactory solution, so I end up creating explicit types instead.

• PS I also find it annoying that C# requires you name the fields explicitly when you using an explicit type in a query, rather than matching the field names for you when it will create the same field names for anonymous types. – NetMage Jul 26 '11 at 1:10

Well this jumped out at me as a relatively easy thing to break out. EDIT: not sure what the data type is on the makbuzQuery type -- hence the data type of the parameter of the new function

//Part 2 -- change to
List<Makbuz> makbuzListesi = getMakbuzList(makbuzQuery);

//new function
List<Makbuz> getMakbuzList(makbuzQueryType makbuzQuery)
{
List<Makbuz> makbuzListesi = new List<Makbuz>();
foreach (var makbuz in makbuzQuery)
{
Makbuz makbuzObjesi = new Makbuz();
//eğer işlemin kuru varsa çekiyorum
decimal isleminKuru = DovizTuruVerilenBirIsleminDovizKurunuCek(makbuz.ID, makbuz.PARABIRIMI);

makbuzObjesi.IslemId = makbuz.ID;
makbuzObjesi.MakbuzTarihi = makbuz.MAKBUZTARIHI;
makbuzObjesi.BelgeNo = makbuz.BELGENO;
//carinin para birimi TL ise kur ile çarpmaya gerek yok
if (makbuz.CARININPARABIRIMI == "TL")
{
makbuzObjesi.GenelToplam = makbuz.GENELTOPLAM.Value;
}
else
{
makbuzObjesi.GenelToplam = (makbuz.GENELTOPLAM * isleminKuru).Value;
}
makbuzObjesi.ParaBirimi = "TL";
makbuzObjesi.Yaziyla =
makbuzObjesi.Aciklama = makbuz.ACIKLAMA;

}
return makbuzListesi;
}

• Thank you Mr.Cortright that was the first thing I also tried but as I mentioned in my question because the query type is IQueryable<anonymous> I think that's why in the foreach loop makbuz. does not give the inner properties of makbuzQuery, it gives error. – JohnnyCageWins Jul 7 '11 at 7:12

Here you go with your division of 1 big method into 3 parts as you wanted :

public static IEnumerable MakbuzOlustur(int islemId)
{
var makbuzQuery = CreateMakbuzQuery(islemId);  // You got your makbuzQuery created and stored
List<Makbuz> makbuzListesi = CreateList(makbuzQuery);
var toReport = CreateReport(makbuzListesi);
}

// Part 1
private static CreateMakbuzQuery(int islemId) {
var makbuzQuery = from islem in db.TBLP1ISLEMs where islem.ID == islemId
select new  {
DateTime.Now.ToShortDateString(),  islem.BELGENO,  islem.GENELTOPLAM, CARININPARABIRIMI
= islem.TBLP1CARI.LISTEPARABIRIMI,  islem.PARABIRIMI,  YAZIYLA =
(islem.ID)
};

return makbuzQuery;
}

// Part 2
private static CreateList(var makbuzQuery) {
List<Makbuz> makbuzListesi = new List<Makbuz>();
foreach (var makbuz in makbuzQuery)            {
Makbuz makbuzObjesi = new Makbuz();
//e?er i?lemin kuru varsa çekiyorum
decimal isleminKuru = DovizTuruVerilenBirIsleminDovizKurunuCek(makbuz.ID,
makbuz.PARABIRIMI);
makbuzObjesi.IslemId = makbuz.ID;
makbuzObjesi.MakbuzTarihi = makbuz.MAKBUZTARIHI;
makbuzObjesi.BelgeNo = makbuz.BELGENO;
//carinin para birimi TL ise kur ile çarpmaya gerek yok
if (makbuz.CARININPARABIRIMI == "TL")                {
makbuzObjesi.GenelToplam = makbuz.GENELTOPLAM.Value;
} else  {
makbuzObjesi.GenelToplam = (makbuz.GENELTOPLAM * isleminKuru).Value;
}
makbuzObjesi.ParaBirimi = "TL";
makbuzObjesi.Aciklama = makbuz.ACIKLAMA;
}
return makbuzListesi;
}

// PArt 3
private static CreateReport(List<Makbuz> makbuzListesi) {
var toReport = from makbuzSon in makbuzListesi
select new
{
ID = makbuzSon.IslemId,
MAKBUZTARIHI = makbuzSon.MakbuzTarihi,
BELGENO = makbuzSon.BelgeNo,
(makbuzSon.GenelToplam.ToString()) + " TL",  PARABIRIMI
=makbuzSon.ParaBirimi, YAZIYLA = makbuzSon.Yaziyla,  ACIKLAMA =
};

}


Hopefully this does your work. Kindly check your query, to make sure that while formatting nothing is lost.

• Thanks I will try this and feedback as soon as possible, I hope I don't get that anonymous IQueryable problem with this code too. – JohnnyCageWins Jul 7 '11 at 9:11
• Errors I had with your code are like this var cannot be used as a parameter type, and some method needed a return type.I handled those but the same problem I mentioned in my question remains with your CreateList method.Thanks for your time. – JohnnyCageWins Jul 7 '11 at 10:54

Why not using Makbuz class as

select new Makbuz()
{
}


instead of using that anonymous type?

And you could make all those queries into a single one if you really want.

• I will try it and feedback as soon as possible, thanks for the idea. – JohnnyCageWins Jul 22 '11 at 7:45
• I see that what you are basically doing there is pulling some data from a source, formatting a bit some properties, and then returning the data via IEnumerable. If I were you and if you afford it, I'd give more power to that Makbuz class. I'd do one method to return IEnumerable<Makbuz> from db.TBLP1ISLEMs in one query based on that ID parameter. The all that extra bits of processing, I would sepparate them in different small methods, and I will call those methods inside "select new Makbuz() { // Assign properties and call those methods here }. – Vladimir Jul 22 '11 at 7:54
• I used to call the methods in select part however, I got could not translate expression into sql error a couple times like this one could not translate expression into sql; that is why I started to use more queries.Also I think not calling the methods in the select part is much readable and never causes error, even if they do tracking them is easier. – JohnnyCageWins Jul 22 '11 at 8:12
• Could you post which method that you are calling inside the selector produces that exception? That error of yours generally happens when you try processing data using the variables pulled directly from the DB (like your islem there from the first line of code). You should do a simple select in first place, then call AsEnumerable() on that query, and then do the processing that you need (by your new methods) client side. – Vladimir Jul 22 '11 at 8:34
• The method is in another question I asked and solved.I linked it on my previous comment, thanks for your interest. – JohnnyCageWins Jul 22 '11 at 8:52

Not a direct answer, but check out the Resharper refactor stuff. Pretty neat. Highlight a section of code and select the menu ReSharper / Refactor / Move.. (Ctrl+RO) It will copy the code to a method of your choice and wire in the correct parameters to keep it working.

I find that when I ecounter these situations that they are less of a headache now as a result of this little option. Like you can just try stuff to see what it looks like and if you don't like it, just hit Ctrl+Z to undo.

• thanks sgtz, what reshaper says is create a method chain which is my idea also, however when I click create chan method I get those errors, too. – JohnnyCageWins Jul 24 '11 at 16:38

Simplest thing will be, using Visual Studio, and let VS do the magic. Select part of the code, right click and select Extract Method, this will give you dialog box where you can give appropriate name of the Method. Passing parameter and return value all will be taken care by visual studio only. No, need to do it manually.

But yes, if you have time then go through site http://www.dofactory.com and check out which design pattern is suitable to your code. Restructure your code accordingly.

I always go with some what faster way, whenever I feel that I have to copy paste some logic will extract method from that, and if I need it in different class, I make that method public, this thing works in small projects and obviously if you are running out of time. Otherwise I personally prefer structural coding for greater re-useability.

Please let me know, if you need further help or information.

• nice link, nice information thank you, I will try those and feedback. – JohnnyCageWins Jul 25 '11 at 13:41