views:

71

answers:

2

Hi,

I have replicated a stripped-down version of my code that has recently been re-written to use linq to access the database.

However, in my opinion, the linq is really simple and could probably be optimized quite a bit, especially around line 90 where there is a linq statement inside a foreach loop.

It'd be really helpful to see how someone else would go about writing this simple task using linq. Thanks in advance! Below is a snippet of my source code.

    // Model objects - these are to be populated from the database,
// which has identical fields and table names.
public class Element
{
    public Element()
    {
        Translations = new Collection<Translation>();
    }

    public int Id { get; set; }
    public string Name { get; set; }
    public Collection<Translation> Translations { get; set; }

    public class Translation
    {
        public int Id { get; set; }
        public string Title { get; set; }
        public string Content { get; set; }
        public Language Lang { get; set; }
    }
}
public class Language
{
    public int Id { get; set; }
    public string Name { get; set; }
    public string Code { get; set; }
}

// Stripped-down functions for adding and loading Element
// objects to/from the database:

public static class DatabaseLoader
{
    // Add method isn't too bulky, but I'm sure it could be optimised somewhere.
    public static void Add(string name, Collection<Translation> translations)
    {
        using (var db = DataContextFactory.Create<ElementContext>())
        {
            var dbElement = new Database.Element()
            {
                Name = name
            };
            db.Elements.InsertOnSubmit(dbElement);
            // Must be submit so the dbElement gets it's Id set.
            db.SubmitChanges();

            foreach (var translation in translations)
            {
                db.Translations.InsertOnSubmit(
                    new Database.Translation()
                        {
                            FK_Element_Id = dbElement.Id,
                            FK_Language_Id = translation.Lang.Id,
                            Title = translation.Title,
                            Content = translation.Content
                        });
            }

            // Submit all the changes outside the loop.
            db.SubmitChanges();
        }
    }

    // This method is really bulky, and I'd like to see the many separate linq
    // calls merged into one clever statement if possible (?).
    public static Element Load(int id)
    {
        using (var db = DataContextFactory.Create<ElementContext>())
        {
            // Get the database object of the relavent element.
            var dbElement =
                (from e in db.Elements
                 where e.Id == id
                 select e).Single();

            // Find all the translations for the current element.
            var dbTranslations =
                from t in db.Translations
                where t.Fk_Element_Id == id
                select t;

            // This object will be used to buld the model object.
            var trans = new Collection<Translation>();

            foreach (var translation in dbTranslations)
            {
                // Build up the 'trans' variable for passing to model object.
                // Here there is a linq statement for EVERY itteration of the
                // foreach loop... not good (?).
                var dbLanguage =
                    (from l in db.Languages
                     where l.Id == translation.FK_Language_Id
                     select l).Single();

                trans.Add(new Translation()
                    {
                        Id = translation.Id,
                        Title = translation.Title,
                        Content = translation.Content,
                        Language = new Language()
                            {
                                Id = dbLanguage.Id,
                                Name = dbLanguage.Name, 
                                Code = dbLanguage.Code
                            }
                    });
            }

            // The model object is now build up from the database (finally).
            return new Element()
                {
                    Id = id,
                    Name = dbElement.Name,
                    Translations = trans
                };
        }
    }
}
A: 

First thing I don't like in here is all the "new Translation() { bla = bla } because they're big blocks of code, I would put them in a method where you hand them the objects and they return the new for you.

Translations.InsertOnSubmit(CreateDatabaseTranslation(dbElement, translation));

and

trans.Add(CreateTranslationWithLanguage(translation, dbLanguage));

etc, wherever you have code like this, it just muddles the readability of what you're doing in here.

Jimmy Hoffa
+1  A: 

Using some made-up constructors to oversimplify:

public static Element Load(int id)
{
    using (var db = DataContextFactory.Create<ElementContext>())
    {
        var translations = from t in db.Translations
                           where t.Fk_Element_Id == id
                           join l in db.Languages on t.FK_Language_Id equals l.Id
                           select new Translation(t, l);

        return new Element(db.Elements.Where(x => x.Id == id).Single(), translations);
    }
}
Ocelot20
That's the kind of thing I was hoping for - I'm not too sure how lambda expressions work, but with your example I hope I'll learn faster. Thanks!
Greg