IMO Create should provision a system unique ID and return it - the user/client of this API probably doesn't have complete knowledge of all other entries. Happy for you to argue against it (leave it).

resolving the comment is not exactly arguing against it ;) .
please put forward your case why you think the user/client of your system would have knowledge of (available) ID or change implementation to provision on ID (e.g. by incrementing a counter).

I agree with @juliaogris on this one. The user should not be expected to understand the complete set of entries. All they want to do is create a puppy, and then have some method of retrieving that puppy. Having the ID returned means the API handles that logic and the user doesn't need to worry about what else exists. They have a means of accessing their object through the id returned from a successful create.

@hsy3418 if you don't agree or have some other view / ideas, please speak up and let us know. I would be happy to discuss and hear your thoughts. Alternatively, please update this one accordingly.

@juliaogris @rokane I totally agree with you the client who is going to create the puppy wouldn't know the ID, they will just enter the puppy info and ID should be handled in the back-end logic, didn't think about that deeply before.

By returning the ID as a sign for creating the puppy successfully make sense to the client definitely, I think I will change the method accordingly, but my question is if there is no error return, would it work similarly to the client? Cause my understanding is if the client just wants to know if the puppy been created or not, they don't really need the ID, unless the front-end logic needs to show the exact ID. Correct me if I am wrong.

I am going to take that back :), just rethink about it, since the ID is created in backend, there is no error could be returned since there wouldn't be duplicate puppys.

ok instead of exists and invert logic so that error is inside if: if !ok return err

I need the p within the if block, so a puppy could be created if ok, otherwise I need to have an else block for create the puppy if ok, but if doing so, I couldn't pass lint..So it seems like I need to keep this logic.

If you flip the logic, you won't need the p inside the if statement, and can go about it that way.

func (m *SyncStore) ReadPuppy(id int32) (Puppy, error) {
  p, ok := m.Load(id)
  if !ok {
    return Puppy{}, fmt.Errorf("puppy with %d ID does not exist", id)
  }
  puppy, _ := p.(Puppy)
  return puppy, nil
}

I think what Julia is suggesting here is to have your structure following the convention of: if err -> fail early

Normally if there are multiple error cases, I think this is a good approach. When there is a single error case, I am not as fussed either way. I will leave this one up to you.

Got it!

resolving the comment is not exactly arguing against it ;) .
please put forward your case why you think the user/client of your system would have knowledge of (available) ID or change implementation to provision on ID (e.g. by incrementing a counter).

adds a new puppy to the puppy store

You have done really well documenting your interface methods, but you forgot these as well. Since the SyncStore and NewSyncStore are also included as part of the exported package, these should also have documentation around them.

I agree with @juliaogris on this one. The user should not be expected to understand the complete set of entries. All they want to do is create a puppy, and then have some method of retrieving that puppy. Having the ID returned means the API handles that logic and the user doesn't need to worry about what else exists. They have a means of accessing their object through the id returned from a successful create.

@hsy3418 if you don't agree or have some other view / ideas, please speak up and let us know. I would be happy to discuss and hear your thoughts. Alternatively, please update this one accordingly.

Again here, this is also a part of the exported package. Please document

Little nit. Another new line not required

New line here can be removed.

If you flip the logic, you won't need the p inside the if statement, and can go about it that way.

func (m *SyncStore) ReadPuppy(id int32) (Puppy, error) {
  p, ok := m.Load(id)
  if !ok {
    return Puppy{}, fmt.Errorf("puppy with %d ID does not exist", id)
  }
  puppy, _ := p.(Puppy)
  return puppy, nil
}

I think what Julia is suggesting here is to have your structure following the convention of: if err -> fail early

Normally if there are multiple error cases, I think this is a good approach. When there is a single error case, I am not as fussed either way. I will leave this one up to you.

you should save the id from CreatePuppy and use that to do the ReadPuppy and not just make the assumption that the puppy ID will be zero. As main() is a user of the store, it should not make assumptions about the internal workings of the store. The store could start IDs at 1, not 0. Since there is no need to assume because CreatePuppy returns the ID, you should just use that ID.

Put a \n at the end of this output and the next. This should output a complete line to be user-friendly.

nit: Put a space after // like you have for the doc strings above. Do the same for the next comment and also for the comments in syncStore.go

Instead of looking up the map again, when you did it already before, you should save the value from from the first lookup. It means you can't put it all inside the if, but that doesn't matter:

puppy, ok := m.puppies[id]
if !ok {
    ...
}
return puppy, nil

I think it is more readable if this function is placed at the top of the file before SetupTest. This is where the tests in this file start, so it reads better to start here, and then move onto SetupTest, and then finally the tests themselves.