StackOverflow2013

Note that there are some explanatory texts on larger screens.

plurals

PO
text
Body
copied!<p>Hmm, there is definitely a couple of things that I would do to improve this.</p> <p><strong>First:</strong> I would define an interface for your Repository to implement. This allows you greater control over dependencies, and when coupled with a Inversion Of Control/Dependency Injection (IOC/DI) framework, this is hugely improved. IOC/DI frameworks include StructureMap or NInjet. Have a read of <a href="http://www.hanselman.com/blog/ListOfNETDependencyInjectionContainersIOC.aspx" rel="noreferrer">this</a> from Scott Hanselman, it's a pretty comprehensive list.</p> <p>Your interface may look like:</p> <pre><code>public interface IClientRepository { IQueryable<ClientBO> FindAllClients(int userId); ClientBO FindClientById(int userId, int clientId); ClientInsert(ClientBO clientBO); ClientBO updateClient(ClientBO clientBO); bool deleteClients(string Ids, int userId); } </code></pre> <p><strong>Second:</strong> don't do your Business Object (<code>ClientBO</code>) to persistent object (<code>Client</code>) conversion inside of your repository. This means that if you make any changes to your BO, then you'll need to go through and change your entire repository.</p> <p>I notice you have a lot of left-right assignment code, eg. </p> <pre><code>cli.ClientName = clientBO.ClientName; </code></pre> <p>I would seriously investigate the use of <a href="http://automapper.codeplex.com/" rel="noreferrer">AutoMapper</a>. It makes this "monkey code" a hell of a lot easier.</p> <p>EDIT: <a href="http://www.bengtbe.com/blog/post/2009/04/14/Using-AutoMapper-to-map-view-models-in-ASPNET-MVC.aspx" rel="noreferrer">Here is a blog post</a> that describes how to use AutoMapper to remove the left-right assignment code.</p> <p><strong>Third:</strong> Your naming structure is all over the shop. We have: <code>FindAllClients()</code>, <code>ClientInsert()</code>, <code>updateClient()</code> all in the one class. Very very poor naming. For your repository, try to model your methods on what will be happening on the DB side. Try <code>Add</code> or <code>Insert</code>, <code>Delete</code>, <code>FindAll</code> or <code>GetAll</code>, <code>Find</code> or <code>GetAll</code>, <code>SaveChanges</code>, method names.</p> <p>Don't append/prepend the type to the method name, as your are in the <code>ClientRepository</code>, it's implied that you'll be adding or getting <code>Client</code>'s.</p> <p><strong>Fourth:</strong> Your mixing your LINQ syntax. In some places your using the declarative query syntax and other places your using the method syntax. Pick 1 and use it everywhere.</p> <p><strong>Fifth:</strong> This line worries me:</p> <pre><code> if (!taxidb.Clients.Where(c => c.ClientMobNo == cli.ClientMobNo).Any()) </code></pre> <p>This looks suspiciously like business logic to me. Not something that should be in the repository. Either declare the column to be <code>UNIQUE</code> in the DB, or move that logic into another validation layer.</p> <p>These were the main things that jumped out at me. A couple of these are personal preference, but I hope this helps. </p>

Querying!

Guidance

An individual column

Larger individual text columns get their own page to allow for proper reading.

SQuiL has stopped working due to an internal error.

If you are curious you may find further information in the browser console, which is accessible through the devtools (F12).

Reload