views:

157

answers:

4

Say I have a number of usercontrols, each usercontrol inside a tabitem, inside a window.

For example, let say this is a food collection application. Then we have tabs Fruit, Vegetables and Snacks. Each tab will show a list of food of that subject, and allow the user to add, delete, modify the food in each section. The food is stored in seperate textfiles, i.e. Fruit.txt, Vegetable.txt, Snack.txt

The actual text files might look something like this (vegetable.txt):

Name        Carbs    Fat
Eggplant    2        1.1
Cucumber    3        0.5
etc

Now this is a large list and there is a load method which pulls all the vegetables out into a List

The question I have is this loadVegetables method is in the code behind file, and I end up repeating this load method all over the place, because I have another of other screens like ReviewAllFood, AddVegetable, etc. along with all the other load methods for fruit and snacks.

This is more of a design question, I'm wondering how I set this up to not repeat this code. I could have a VegetableManager (or something) class where the load method is, but does this actually mean less repeated code? Then in each screen I have to create object of VegetableManager and call its load method anyway. So I guess efficiency wise its no better, but I do achieve a better design.

I think I'm missing something here. It's been a while since I studied cohesion and coupling and I think i'm confusing myself with these concepts at the moment. Appreciate if someone could suggest a design for this situation and explain why they chose it and why its better than how i'm doing it at the moment.

Thanks for reading.

+2  A: 

I would suggest pulling the vegetables (or whatever it is you're loading) out once when you read the file. Then you store them in some underlying data model. You can bind the list, and whatever other controls you need to, to the underlying data model. The data gets loaded once, but various views can display it.

EDIT: Adding code

List<T> loadObjects(File file, ILineConversionStrategy strategy) {
   // read eaqch line of the file
   // for each line
   T object = strategy.readLine(line);
   list.add(object);
   return listOfObjects;
}

EDIT 2: Data model

class FoodModel {
   List<Vegetable> getVegetables();
   List<Fruit> getFruit();
   // etc
}
Jeff Storey
what would the underlying data model be (suggestions)? I guess the other issue is that because as part of the app users can add vegetables etc... at the moment these are written straight back to file, so I continuously reload as potential changes have been made.
baron
Your model what store whatever data your application needs. It could be as simple as an object that has a bunch of bound properties. I would advise not writing back to the file on each change but writing to the model since the I/O is much slower. You can flush the model to a file when the app closes (or periodically if you need).
Jeff Storey
I'm not sure I quite get you. At the moment they're stored in a number of List<Vegetable>, List<Fruit>, List<Snack> - but this is repeated in code behind a lot of places. I could make a seperate class like I mentioned, but then i'm just creating an object of that class everywhere. What would it look like how your explaining? Can you show some example/sample code?
baron
So I think we're talking about two problems here. I've posted some sample code that shows how to avoid repeating the load code everywhere for each type of object. It uses the strategy pattern to determine how to convert the line in the file to the appropriate object - vegetable, fruit, etc. This way the same code can be used for loading every type of file, and just the conversion of the text line to the object is different (the code is java like though since I'm a java developer).For the other issue of calling the code everywhere, it should only be once, when you load it from the file.
Jeff Storey
so how do you access that collection from other classes?
baron
When the load method is called it loads the foods into the model. The the other classes would have a reference to the model.
Jeff Storey
A: 
    public interface IEatable {}

    class Vegitable : IEatable 
    { string Name { get; set; } }
    class Fruit : IEatable 
    { string Name { get; set; } }

    public interface IEatableManager
    {
        List<Vegitables> LoadEatables(string filePath);
    }
    public class VetabaleManager : IEatableManager
    {
        #region IEatableManagerMembers    
        public List<Vegitable> LoadVegs(string filePath)
        {
            throw new NotImplementedException();
        }    
        #endregion
    }
    .
    .
    .

There are several things you need to consider for using a design like above

and a must read:

Asad Butt
+2  A: 

I could have a VegetableManager (or something) class where the load method is, but does this actually mean less repeated code? Then in each screen I have to create object of VegetableManager and call its load method anyway.

The point of doing this is not efficiency (i.e. performance). The point is to encapsulate the details of loading that data into a single isolated object. Say for example that your site gets really big and you decide to move the data storage to a database for scalability and performance. In the existing code as you described, you'll have to go through each user control or page and change the logic of the load method. At the best this is a pain, and at the worst you miss some or copy-paste incorrectly. If the logic is encapsulated into a dedicated object, whose only responsibility is to know how to load the data from somewhere, then you only have to make the change once.

codebehind of user control:

protected void Page_Load(object sender, EventArgs e) {
  var veggieManager = new VegetableManager();
  VeggieListControl.DataSource = veggieManager.GetAll();
  VeggieListControl.DataBind();
}

VegetableManager.cs:

public class VegetableManager {
  private static Collection<Vegetable> _veggies;
  private static object _veggieLock;

  public ReadOnlyCollection<Vegetable> GetAll() {
    if (_veggies == null) {
      lock(_veggieLock) { //synchronize access to shared data
        if (_veggies == null) { // double-checked lock
          // logic to load the data into _veggies
        }
      }
    }

    return new ReadOnlyCollection(_veggies);
  }

  public void Add(Vegetable veggie) {
    GetAll(); // call this to ensure that the data is loaded into _veggies
    lock(_veggieLock) { //synchronize access to shared data
      _veggies.Add(veggie);
      // logic to write out the updated list of _veggies to the file
    }
  }
}

Because _veggies is static, there is only one collection of veggies in memory, despite the fact that multiple callers will instantiate VegetableManager. But because it's static, if you have a multi-threaded application (e.g. a website) you must synchronize access to that field across all threads (hence the locks).

This is the tip of the iceberg in terms of good object-orientation. I recommend perusing UncleBob's SOLID principles, and Domain-Driven Design (free e-book).

So, yes you are repeating something, but all you're repeating is a method call, and that is ok to repeat. DRY means to mitigate the duplication of "logical" code, i.e. decision-making and algorithms; simple method calls do not fall under this. However, if you want, you can consolidate logic into a base class do this, effectively isolating the user controls from having to know about VegetableManager, though I think this is object-orientation overkill, or OOO :-)

public abstract class FoodUserControl : UserControl {
  protected List<Vegetable> GetVeggies() {
    return new VegetableManager().GetAll();
  }
}

Then your actual controls would derive from this instead of from UserControl.

Update

Eager-loading VegetableManager.cs:

public class VegetableManager {
  private static Collection<Vegetable> _veggies;
  private static object _veggieLock;

  static VegetableManager() {
    // logic to load veggies from file
  }

  public ReadOnlyCollection<Vegetable> GetAll() {
    return new ReadOnlyCollection(_veggies);
  }

  public void Add(Vegetable veggie) {
    lock(_veggieLock) { //synchronize access to shared data
      _veggies.Add(veggie);
      // logic to write out the updated list of _veggies to the file
    }
  }
}

Notice this eager-loading version doesn't have to do double-checked locking around the load code in the constructor. Also notice that the load code is in a static constructor, since this code initializes a static field (otherwise, you'd be reloading the data from the file on every construction into the same shared static field). Because veggies are eager-loaded, you don't need to load in GetAll or Add.

gWiz
I understand there are multiple ways to model the persistence. What i was struggling with is the following. Using your example. What I was originally thinking was inside the FoodUserControl, lets say I have ui with text fields and add button. I add a vegetable. What I was doing previously is adding the vegetable to that local list. But actually I should have a method in VegetableManager addVeggie. Then we have something like var veggieManager = new VegetableManager(); veggieManager.AddVeggie(txt.text, txt2.text); adding it to veggie managers list,
baron
not the local copy of the list I have in the class. Then the only problem is updating the UI when the veggiemangers list has changed. AS oposed to having var veggieManager = new VegetableManager(); List<Vegetable> veggies = veggieManager.GetVeggies(); veggies.Add(txt.text, txt2.text);
baron
Why have a separate manager for each type of food? You're going to end up with a whole lot of various managers. Then if something wants to access say fruits and vegetables, it will need both a vegetable and fruit manager.
Jeff Storey
Well, when handling an Add event, first you call VegetableManager.Add to add the vegetable to the system so that it's accessible to all other requests. Second you call VegetableManager.GetAll and re-bind your listing user control to this updated veggie list.
gWiz
@Jeff, because I'm keeping my answer simple and focused on why persistence modeling is useful at all. Adding a generic aspect to my answer would complicate it further. Also, generic repositories aren't without their problems. To write to different files, a generic repo would require type-based switch statements, which bury couplings and can violate the Open-Closed Principle. http://www.objectmentor.com/resources/articles/ocp.pdf and http://refactoring.com/catalog/replaceConditionalWithPolymorphism.html
gWiz
Thank you both, makes a lot more sense now, I see how I should be structuring things quite differently.
baron
@gWiz, makes sense for simplification, though you wouldn't need to have switch statements for different files. The strategy pattern makes a nice way to choose the file writer depending on the object type (by injecting a map of object types to the file writer type) to the generic repository. But, it really depends how many types of different objects are being written out (obviously the pattern scales better than the other solutions but for smaller problems it could be overkill).
Jeff Storey
should the constructor for VegetableManager call GetAll()? I think it looks like this Public VegetableManager() { GetAll(); }
baron
@Jeff +1 true that about injecting a map. good point.
gWiz
well the constructor could call it. for the best DRYness, that loading logic would be pulled out into a new method, and GetAll, Add, and the constructor would call that new method. i'll update my answer.
gWiz
i've updated my answer. rather than pull out the logic into a new method as my previous comment mentions, i put the load logic into a static constructor. this is because when eager-loading in a constructor, you don't need to do the load routine anywhere else (so you don't necessarily need to refactor the load logic into a method for DRYness--though it could help for readability).
gWiz
A: 

I would use the repository pattern for this. As a start, create one class containing methods to retrieve the objects from each text file:

public class FoodRepository
{
    public IList<Vegetable> GetVegetables() { ... }
    public IList<Fruit> GetFruit() { ... }
    // etc.
}

This class should be the only class in your application that is aware that foods are actually stored in text files.

Once you get that working you might want to consider caching frequently used data to improve performance.

Jamie Ide
where would a AddVegetable method go? and so I can access this from other classes like - just say I wanted to iterate through vegetables - foreach(Vegetable in FoodRepository.Vegetables). Actually I guess this method would go in that class. I suppose what I'm struggling with is how I have one main collection, not creating instances of FoodRepository everywhere - each with different collections etc. Like say we have foodviewer and addfood usercontrols. In addfood I add a food to that instances collection, but in the foodviewer, no food has been added to that instances collection.
baron
Unless I tell it to reload the foods and tell the addfood method to write back to the file. (Hope this makes sense)
baron
There are multiple ways to model the persistence, and this is just one of them. But it appears to me the asker needs an explanation that cuts to the core of why *any* modeling of the persistence is beneficial.
gWiz
@gWiz -- I have the opposite take on it and after re-reading still do. This site suffers from complex answers to simple questions. The question is about taking a first step to centralizing data access in a small web app, and the accepted answer mentions SOLID, DRY, multi-threading, and that torturous DDD book.
Jamie Ide
@baron -- As a first step, I would put all data access, including AddVegetable, into one repository class unless that becomes unwieldy. I'm not sure I understand the rest of the question, but you can make the class static and use caching so that you only have one instance.
Jamie Ide
@Jamie, I see where you're coming from: providing a known-good solution to a requirement. But IMHO the question is not just about a requirement, but about how fundamentals (SOLID and DRY) are applied practically. In comments, the OP asks how a single manager class can be re-used without instantiating/loading the list for each request. Thus shared state is explained, which *requires* mention of concurrency. IMHO, the OP wants the details of the big picture, so he can properly bridge the gap between theory and practice for this problem and *future ones*.
gWiz