2010-04-06 6 views
3

Regardons ce code:Comment écrire ceci d'une meilleure manière?

IList<IHouseAnnouncement> list = new List<IHouseAnnouncement>(); 
var table = adapter.GetData(); //get data from repository object -> DataTable 

if (table.Rows.Count >= 1) 
{ 
    for (int i = 0; i < table.Rows.Count; i++) 
    { 
     var anno = new HouseAnnouncement(); 
     anno.Area = float.Parse(table.Rows[i][table.areaColumn].ToString()); 
     anno.City = table.Rows[i][table.cityColumn].ToString(); 
     list.Add(anno); 
    } 
    } 
    return list; 

Est-il meilleure façon d'écrire cela en moins de code et une meilleure façon (doit être :-))? Peut-être en utilisant lambda (mais laissez-moi savoir comment)?

Merci d'avance!

Répondre

9

Juste pour info, vous êtes ne jamais ajouter le nouveau HouseAnnouncement à votre liste, et votre boucle ne s'exécutera jamais pour la dernière ligne, mais je suppose que ce sont des erreurs dans l'exemple plutôt que dans votre code réel.

Vous pouvez faire quelque chose comme ceci:

return adapter.GetData().Rows.Cast<DataRow>().Select(row => 
    new HouseAnnouncement() 
    { 
     Area = Convert.ToSingle(row["powierzchnia"]), 
     City = (string)row["miasto"], 
    }).ToList(); 

Je vais habituellement pour une meilleure lisibilité sur la concision, mais je me sens comme celui-ci est assez lisible. Notez que bien que vous puissiez toujours mettre en cache le DataTable et utiliser table.powierzchniaColumn dans le lambda, j'ai éliminé cela afin que vous n'utilisiez pas de fermeture qui n'était pas vraiment nécessaire (les fermetures introduisent une complexité substantielle à l'implémentation interne du lambda , donc je les évite si possible).

S'il est important pour vous de garder les références de colonne comme ils sont, alors vous pouvez le faire comme ceci:

using (var table = adapter.GetData()) 
{ 
    return table.Rows.Cast<DataRow>().Select(row => 
     new HouseAnnouncement() 
     { 
      Area = Convert.ToSingle(row[table.powierzchniaColumn]), 
      City = (string)row[table.miastoColumn], 
     }).ToList(); 
} 

Cela compliquera l'IL réelle que le compilateur génère, mais devrait faire la tour pour vous.

+0

Ceci est le winnar. En outre, dario, voyez si vous pouvez obtenir votre méthode pour renvoyer un IEnumerable si possible, alors vous pouvez demander aux consommateurs de le lancer dans une liste s'ils le souhaitent. Vous pouvez obtenir qu'il retourne un IEnumerable en convertissant la valeur de retour de lambda en IHouseAnnouncement ou en utilisant 'as'. –

+0

"cast vers liste" = "le convertir en liste" –

+0

La méthode: Rows.Cast () ne s'affiche pas et ne compilera pas. – Dariusz

0

La lisibilité est, pour moi, préférable d'être concis avec votre code - tant que la performance n'est pas une victime. En outre, je suis sûr que toute personne qui doit plus tard maintenir le code l'appréciera également.
Même si je maintiens mon propre code, je ne veux pas le regarder, dire quelques mois plus tard, et penser "que diable essayais-je d'accomplir"

2

Votre "instruction if" n'est pas nécessaire. Votre "boucle for" prend déjà en charge ce cas.

En outre, votre "boucle for" ne s'exécutera pas lorsque le nombre de vos lignes de table est 1. Cela semble être une erreur, et non pas par conception, mais je peux me tromper. Si vous voulez résoudre ce problème, il suffit de prendre le « -1 »:

for (int i = 0; i < table.Rows.Count; i++) 
1

Eh bien, pour une chose, vous semblez avoir un hors par une erreur:

for (int i = 0; i < table.Rows.Count - 1; i++) 
{ 
} 

Si votre La table a trois rangées, celle-ci s'exécutera alors que i est inférieure à 3 - 1, ou 2, ce qui signifie qu'elle fonctionnera pour les rangées 0 et 1 mais pas pour la rangée 2. Ce n'est peut-être pas ce que vous voulez.

0

je pourrais faire quelque chose comme ceci:

var table = adapter.GetData(); //get data from repository object -> DataTable 

return table.Rows.Take(table.Rows.Count-1).Select(row => new HouseAnnouncement() { 
    Area = float.Parse(row[table.powierzchniaColumn].ToString()), 
    City = row[table.miastoColumn].ToString() 
}).ToList(); 
5

Vous pouvez faire quelque chose comme ça dans Linq:

var table = adapter.GetData(); 
var q = from row in table.Rows.Cast<DataRow>() 
     select new HouseAnnouncement() 
      { Area = float.Parse(row[table.areaColumn].ToString()), 
      City = row[table.cityColumn].ToString() 
      }; 
return q.ToList(); 
+0

Ce serait la solution la plus succincte, expressive et élégante. –

+0

Malheureusement, il ne compile pas. 'DataTable.Rows' n'implémente pas' IEnumerable ', vous devez donc appeler' Cast () 'pour lancer une requête dessus. Même ainsi, je ne vois pas comment c'est plus succinct (puisque c'est trois déclarations au lieu de deux). –

+0

En outre, vous récupérez deux fois les données. –

1

ne peuvent pas aller beaucoup plus simple que l'on boucle for et sans instructions if :

var table = adapter.GetData(); //get data from repository object -> DataTable 
IList<IHouseAnnouncement> list = new List<IHouseAnnouncement>(table.Rows.Count); 

for (int i = 0; i < list.Length; i++) 
{ 
    list[i] = new HouseAnnouncement(); 
    list[i].Area = float.Parse(table.Rows[i][table.areaColumn].ToString()); 
    list[i].City = table.Rows[i][table.cityColumn].ToString(); 
} 

return list; 

Il prend plus de caractères que la version linq, mais est analysé plus rapidement par le cerveau du programmeur. :)

Questions connexes