New to TDD, am I doing everything right? How could I improve?

I'm new to TDD, never used it ever. I understand the basic concepts but I'm now working on a small project which will be the first time I've ever actually used TDD.

The code is pretty self explanatory, it's a simple user data storage class. I've written an interface, a UserData class, and then implemented the interface with SimpleUserStore which uses lists.

I first wrote the tests, and then wrote the code so that they'd fail, and then "filled in" the code so that they would pass. When I realised I needed another method, I wrote the tests, then generated the stubs, then wrote the code so that they would pass.

My question is basically, am I doing this right? Are there things I'm missing out? Big gaping holes that make you go woah, wait a second!?

All responses appreciated.

```
public interface IUserStore
{
bool AddUser(UserData user);
bool DeleteUser(UserData user);
bool UpdateUser(UserData user);

UserData GetUserById(int id);
UserData GetUserByName(string name);
}

public class UserData
{
public UserData() { }

public virtual int UserId { get; set; }
public virtual string Username { get; set; }
public virtual string PasswordHash { get; set; }
public virtual string PasswordSalt { get; set; }
}

public class SimpleUserStore : IUserStore, IDisposable
{
private List userdata;

public SimpleUserStore()
{
userdata = new List();
}

public bool AddUser(UserData user)
{
if (user == null) throw new ArgumentNullException("user");

if (userdata.Contains(user)) return false;

userdata.Add(user);
return userdata.Contains(user);
}

public bool DeleteUser(UserData user)
{
if (user == null) throw new ArgumentNullException("user");
return userdata.Remove(user);
}

public bool UpdateUser(UserData user)
{
if (user == null) throw new ArgumentNullException("user");
if (!userdata.Contains(user)) return false;
U

Your tests in general suffer from trusting your code too much:

[TestMethod]
public void DeleteUserTest()
{                        
    simpleUserStore.AddUser(userA);
    Assert.IsTrue(simpleUserStore.DeleteUser(userA));            
}

You verify the return value, but you don't verify that the user has been removed from the store. The object could very well return success without actually removing the object. You should have a call to GetUser() or similiar to verify that the user isn't there anymore.

Similar with update:

[TestMethod]
public void UpdateUserTest()
{                        
    simpleUserStore.AddUser(userA);
    userA.Username = "Testing";
    Assert.IsTrue(simpleUserStore.UpdateUser(userA));
}

You don't do anything to verify that update actually did the update. You trust the return value. But you shouldn't do that. Let's actually look at the implementation.

public bool UpdateUser(UserData user)
{
    if (user == null) throw new ArgumentNullException("user");
    if (!userdata.Contains(user)) return false;
    UserData _user = userdata.FirstOrDefault(x => x.UserId == user.UserId);

You don't verify that this works. You never pass a different object then the one you originally inserted in the Store. You need to pass another object to see if that works.

_user = user;

You don't verify that the user in the store gets updated, so you aren't testing this line either.
return true;
}

Truth be told, I can see two bugs in this implementation. I'll leave it to you to fix your tests and actually find them yourselves. However, its bad enough that if you wrote this in an interview I probably wouldn't hire you. One of the bugs makes it look like you are very confused about the semantics of the language you are using. Maybe its just a careless error (they happen to the best of us), but it would give me grave doubts.

StackExchange Code Review Q#21297, answer score: 5