Ask user to Save data code review

When user clicks to exit form, and data are unsaved, I would like to ask user does he wants to save data. Can below code be more simplified, to be easier to read this method SaveOrRejectChanges().

private bool SaveOrRejectChanges()
{
if (MsgBox("do you want to save data") == true)
{
if (ValidateIsEmptyOrNullValue()) return false; //here I check is required field not entered

SaveChanges(); // if validation passes (data are entered), save it
}
else
RejectChanges(); // cancel changes by setting EntityState.Unchanged on DBContext for each entity

return true;
}

private void iExit_ItemClick(object sender, ItemClickEventArgs e)
{
_isFormClosing = true;

// close form if data are not modified (no saving required). Or close form when data are saved or changes are canceled
if (_isDataModified == false || SaveOrRejectChanges())
this.Close();
}


I am still sort of beginner, so please share your thoughts. I kind of dont like nested if, but I had to put it inside of other if, as if it is before asking user, then it would simply cancel closing which I dont want of course. Please share your solution if you think of some. Thank you.

So what is your question, whats not working
– TheGeneral
Jun 30 at 2:01

TheGeneral, question is can code in method SaveOrRejectChanges() be more simplified, to be easier to read in that method. it has IF for asking user with MsgBox, then we had another if, its kind of more difficult to read
– ExpertLoser
Jun 30 at 2:02

This RejectChanges(); // cancel changes by setting EntityState.Unchanged on DBContext for each entity worries me. Just dispose the context, you don't need any of that manual work. You can remove that else clause completely
– Camilo Terevinto
Jun 30 at 2:05

RejectChanges(); // cancel changes by setting EntityState.Unchanged on DBContext for each entity

else

To be honest, there is nothing really wrong with that method. well apart from the whole entity thing. we sometimes have to live with ifs
– TheGeneral
Jun 30 at 2:06

if

thanks Camilo. In form closing event I have this code : private void FormClosing(object sender, FormClosingEventArgs e) { e.Cancel = false; DbContext.Dispose(); } so DBContext is disposed when form is closed
– ExpertLoser
Jun 30 at 2:07

2 Answers
2

This is more of a Code Review question.

The way you're coding is mixing logic code with GUI code. A much better approach is binding the Form Controls to a class.

This way you can easily unit test the business logic, even if it's just validating & saving the state of the controls.

The other benefit is if all the Form Controls state is saved in a class you can simply Serialise the class to Json, XML, dB, etc.

Update:

Here is a huge tutorial on using BindingSource Controls - DataGridView, TextBox & ListBox examples: https://www.codeproject.com/Articles/24656/A-Detailed-Data-Binding-Tutorial.

So your logic is already in a business logic class, but you have a dependancy still on a Message Box. To decouple have an Action method or an EventHandler hooked up to call the GUI Form class, please note psuedo code written on my phone:

public class GUI {

public BusLogic BL = new BusLogic();

public GUI () {
BL.ShouldSaveData += Should_Save_Data;
}

public bool Should_Save_Data() {
DialogResult res = MessageBox.Show("do you want to save data", "", MessageBoxButtons.OkCancel);
return res.Ok == DialogResult.Ok;
}

private void iExit_ItemClick(object sender, ItemClickEventArgs e) {
_isFormClosing = true;

if (_isDataModified) BL.SaveOrRejectChanges();
}
}


In your business logic class you can invoke the ShouldSaveData() event.

public class BusLogic {

public event Action<bool> ShouldSaveData;

public void SaveOrRejectChanges() {
bool confirmSave = false;

if (ShouldSaveData != null) confirmSave = ShouldSaveData;

if (confirmSave) //DO YOUR SAVING HERE
}
}


This keeps the presentation and logic tiers decoupled.

Thanks Jeremy. Well, my database code logic is in Persistence layer. I have also UI layer. So, all regarding database is in database layer. And since some code is still dependent on UI layer, it has to remain in UI layer. I have extracted it in other class methods. However, yes, I do not know Serialise class to other types, its a bit advanced for me. I have barely 2 years of experience. Binding Form controls to a class, hmm, how to do it?
– ExpertLoser
Jun 30 at 13:47

please check my refactored version reply, Saving and Rejecting changes code is in Persistence layer. However, I do not understand serialise concept and "Binding Form controls to a class"? But, my form is actually binded to a class with BindindSource component which uses class from Persistence layer. However, I do not understand other parts which you mentioned.
– ExpertLoser
Jun 30 at 13:56

ok, thank you. I have not used delegates yet, I have to learn it.
– ExpertLoser
18 hours ago

I have improved code a bit, here is refactored version:

private void AskToSaveChanges()
{
// note: MyMessageBox is a class in UI layer, because it uses UI component
// of Windows forms
if (MyMessageBox.Show(MyMessageBoxActions.Save, ""))
SaveChanges(); // this is done in Persistence layer
else
RejectChanges(); // this is done in Persistence layer
}

private void iExit_ItemClick(object sender, ItemClickEventArgs e)
{
_isFormClosing = true;

if (_isDataModified == false || AskToSaveChanges())
this.Close();
}


By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.