Disclaimer
From the comments:
the problem is that I don't have enough experience nor knowledge to improve it
I will stop you right there. If you don't have enough experience nor knowledge then I suggest you don't do refactoring on this specific code, not because you can't but because you endanger yourself of making undesired behaviour or making actually harder to read code.
The sole purpose of refactoring is making a code easier to understand. Now this can be tricky because sometimes some sections of your application have to be very performant, other sections need to be very readable as most of the business logic is there, some parts need to be very flexible since they are changing often there are many cases. The key to refactoring is first realizing what are you refactoring for - performance (very hard, if you don't have experience I would suggest not to do it), readability, extensibility and so on.
Back to your case
Playing with tasks if you are inexperienced often leads to some code running either slower or harder to reason about.
First (and probably the last one given the fact I don't know the business logic) change I would do to this code is postponing Task's awaiter.
// instead of this
public async Task UnpublishPersonAsync(string id)
{
await QueueAndUpdateStatusAsync(id, PersonStatus.Unpublishing, QueueNames.Unpublish);
}
// strive to do this
public Task UnpublishPersonAsync(string id)
{
return QueueAndUpdateStatusAsync(id, PersonStatus.Unpublishing, QueueNames.Unpublish);
}
// or arrow notation if your team likes it
public Task UnpublishPersonAsync(string id)
=> QueueAndUpdateStatusAsync(id, PersonStatus.Unpublishing, QueueNames.Unpublish);
Why is this?
I am always trying to explain the guys in the team that for a change that is so insignificant can give us little tiny bit of improvement. What I mean is that
it does not create async state machine class for your method
it does not introduce unnecessary code that has to be run (goto statements) really really small improvement but if the change is so little.
it leaves the consumer of your method to decide when he wants to await or if he wants to await.
Both methods(QueueAndUpdateStatusAsync and DeletePersonAsync) look the same, alas, they use different overloads of PublishToStorageQueueAsync methods and it will make the code harder to read if you try to abstract them.
What I don't like is throwing exceptions - I use them as a last resort really really last resort. In your case I will refactor both of the methods to follow the same pattern (not that they don't now) like so:
public async Task<bool> DeletePersonAsync(string id, bool deleteAssociatedData)
{
var personDto = await _cosmosDbService.GetItemAsync<PersonDto>(id, id);
var operationIsValid = ValidateOperation(PersonStatus.Deleting, personDto);
// personal preference you can do it with brackets and newlines
if (!operationIsValid) return false;
personDto.Status = PersonStatus.Deleting;
personDto = await _cosmosDbService.UpdateItemAsync(id, id, personDto);
var person = _mappingService.MapToPerson(personDto);
var deleteCommand = new DeletePersonCommand()
{
Person = person,
DeleteAssociatedData = deleteAssociatedData
};
await _queue.PublishToStorageQueueAsync(deleteCommand, QueueNames.Delete);
return true;
}
As you can see now you return if the whole transaction is successful or not simply by changing return type to Task<bool> let consumers handle errors the way they want don't force them to handle your exception especially for something so easy to fix.
If you have decided to go with exceptions nevertheless, then I strongly suggest you encode some information inside it. Say some message like "Validation of X failed." anything that can help you give meaning to it later when you happen to debug it. Also if this is some kind of library code, creating custom specific exception is also an option if you like exceptions so much, but as a rule of thumb I strongly suggest you to stay away from exceptions at all cost no matter the language or framework.
P.S. They also incur performance drawback for unwinding the stack later. Your clients need to catch it and probably re-throw it also if necessary, overall it gets messy IMO.
Conclusion
Other than that I think its relatively easy code to follow, don't refactor it unless you find problems with it - whether that will be performance problems, readability problems and etc.
