Skip to main content
Code Review

Return to Answer

replaced http://programmers.stackexchange.com/ with https://softwareengineering.stackexchange.com/
Source Link

UsersRepository

private DbEntities db;
public UsersRepository (DbEntities db)
{
 this.db = db;
} 

You should define DbEntities db as readonly to prevent it beeing changed by accident.


The public virtual IEnumerable<tUser> GetSome( Expression<Func<tUser, bool>> filter = null, Func<IQueryable<tUser>, IOrderedQueryable<tUser>> orderBy = null, string includeProperties = "") method should have some parameter validation at least for the includeProperties parameter. This parameter should be checked for null to throw an early exception.


This

public void DeleteUser(object userId)
{
 var user = db.tUser.Find(userId);
 db.tUser.Remove(user);
} 

can be simplified by using the GetById() method like so

public void DeleteUser(object userId)
{
 var user = GetById(userId);
 db.tUser.Remove(user);
} 

You are using this inconsistently. You are reffering to this only with the disposed variable but never with the db.


#region dispose 

Please read Are regions an antipattern or code smell Are regions an antipattern or code smell


UnitOfWork

private IUserRepository userRepository;
public IUserRepository UserRepository
{
 get
 {
 return userRepository ?? new UsersRepository(db);
 }
} 

In this way userRepository will always be null. Either return each time a new UsersRepository(db); or change the getter to

private IUserRepository userRepository;
public IUserRepository UserRepository
{
 get
 {
 return userRepository ?? (userRepository = new UsersRepository(db));
 }
} 

What happens if Dispose() is called twice ? You get a NullReferenceException. So either implement it like you did in the UsersRepository or add a simple null check for db.

UsersRepository

private DbEntities db;
public UsersRepository (DbEntities db)
{
 this.db = db;
} 

You should define DbEntities db as readonly to prevent it beeing changed by accident.


The public virtual IEnumerable<tUser> GetSome( Expression<Func<tUser, bool>> filter = null, Func<IQueryable<tUser>, IOrderedQueryable<tUser>> orderBy = null, string includeProperties = "") method should have some parameter validation at least for the includeProperties parameter. This parameter should be checked for null to throw an early exception.


This

public void DeleteUser(object userId)
{
 var user = db.tUser.Find(userId);
 db.tUser.Remove(user);
} 

can be simplified by using the GetById() method like so

public void DeleteUser(object userId)
{
 var user = GetById(userId);
 db.tUser.Remove(user);
} 

You are using this inconsistently. You are reffering to this only with the disposed variable but never with the db.


#region dispose 

Please read Are regions an antipattern or code smell


UnitOfWork

private IUserRepository userRepository;
public IUserRepository UserRepository
{
 get
 {
 return userRepository ?? new UsersRepository(db);
 }
} 

In this way userRepository will always be null. Either return each time a new UsersRepository(db); or change the getter to

private IUserRepository userRepository;
public IUserRepository UserRepository
{
 get
 {
 return userRepository ?? (userRepository = new UsersRepository(db));
 }
} 

What happens if Dispose() is called twice ? You get a NullReferenceException. So either implement it like you did in the UsersRepository or add a simple null check for db.

UsersRepository

private DbEntities db;
public UsersRepository (DbEntities db)
{
 this.db = db;
} 

You should define DbEntities db as readonly to prevent it beeing changed by accident.


The public virtual IEnumerable<tUser> GetSome( Expression<Func<tUser, bool>> filter = null, Func<IQueryable<tUser>, IOrderedQueryable<tUser>> orderBy = null, string includeProperties = "") method should have some parameter validation at least for the includeProperties parameter. This parameter should be checked for null to throw an early exception.


This

public void DeleteUser(object userId)
{
 var user = db.tUser.Find(userId);
 db.tUser.Remove(user);
} 

can be simplified by using the GetById() method like so

public void DeleteUser(object userId)
{
 var user = GetById(userId);
 db.tUser.Remove(user);
} 

You are using this inconsistently. You are reffering to this only with the disposed variable but never with the db.


#region dispose 

Please read Are regions an antipattern or code smell


UnitOfWork

private IUserRepository userRepository;
public IUserRepository UserRepository
{
 get
 {
 return userRepository ?? new UsersRepository(db);
 }
} 

In this way userRepository will always be null. Either return each time a new UsersRepository(db); or change the getter to

private IUserRepository userRepository;
public IUserRepository UserRepository
{
 get
 {
 return userRepository ?? (userRepository = new UsersRepository(db));
 }
} 

What happens if Dispose() is called twice ? You get a NullReferenceException. So either implement it like you did in the UsersRepository or add a simple null check for db.

Source Link
Heslacher
  • 50.9k
  • 5
  • 83
  • 177

UsersRepository

private DbEntities db;
public UsersRepository (DbEntities db)
{
 this.db = db;
} 

You should define DbEntities db as readonly to prevent it beeing changed by accident.


The public virtual IEnumerable<tUser> GetSome( Expression<Func<tUser, bool>> filter = null, Func<IQueryable<tUser>, IOrderedQueryable<tUser>> orderBy = null, string includeProperties = "") method should have some parameter validation at least for the includeProperties parameter. This parameter should be checked for null to throw an early exception.


This

public void DeleteUser(object userId)
{
 var user = db.tUser.Find(userId);
 db.tUser.Remove(user);
} 

can be simplified by using the GetById() method like so

public void DeleteUser(object userId)
{
 var user = GetById(userId);
 db.tUser.Remove(user);
} 

You are using this inconsistently. You are reffering to this only with the disposed variable but never with the db.


#region dispose 

Please read Are regions an antipattern or code smell


UnitOfWork

private IUserRepository userRepository;
public IUserRepository UserRepository
{
 get
 {
 return userRepository ?? new UsersRepository(db);
 }
} 

In this way userRepository will always be null. Either return each time a new UsersRepository(db); or change the getter to

private IUserRepository userRepository;
public IUserRepository UserRepository
{
 get
 {
 return userRepository ?? (userRepository = new UsersRepository(db));
 }
} 

What happens if Dispose() is called twice ? You get a NullReferenceException. So either implement it like you did in the UsersRepository or add a simple null check for db.

lang-cs

AltStyle によって変換されたページ (->オリジナル) /