Edit method, doing multiple checks for tracking changes vs the one in database

I am working on a Spring-MVC application in which there are around 2-3 methods which are quite big and complex, but only one of them worries me, especially for maintenance or changes. As changes in the method can immediately change some critical functionality in the project. The application is basically a Note-taking application, and takes as input a lot of parameters, and fans out to external services, different views.

The method I am worrying is about editing a Note object. For that, I have to do multiple checks and accordingly take action. I have managed to break down the method into 3 parts, but the core of it still remains tacky at best. How can I proceed with simplification. I am pasting the method for reference sake, cannot go into details of what is actually happening.

Code:

```
@Override
public String editGroupNote(GroupNotes noteObjectFromUser, int msectionId) {

if (noteObjectFromUser.isPrivateNoteFlag()) {
createPrivateNote(noteObjectFromUser, msectionId);
return "privacychange";
}
Person person = this.personService.getCurrentlyAuthenticatedUser();

NoteSelection noteSelection =
this.noteSelectionService.checkIfSelectionValid(noteObjectFromUser.getMnoticesid(),person.getId());
boolean checkIfEvernote = (noteSelection != null);
String latestText = noteObjectFromUser.getMnotetext();
noteObjectFromUser.setMnotetext(noteObjectFromUser.getMnotetext().replaceAll("\\sid=\"cke[^\">]\"",""));

String newText = "";

GroupSection retrievedSection = this.groupSectionService.getGroupSectionById(msectionId);
GroupCanvas ownedCanvas = this.groupCanvasService.getCanvasById(retrievedSection.getCurrentCanvasId());
GroupAccount ownedAccount = this.groupAccountService.getGroupById(ownedCanvas.getGroupAccountId());
GroupNotes databaseNoteObject = this.groupNotesDAO.getGroupNoteById(noteObjectFromUser.getMnoticesid());
Group

if (fromUserDate.after(savedDate)) {
    groupNoteHistory.setWhatHasChanged("generalchange");
    noEdit = true;
}

if (savedDate.after(fromUserDate)) {
    groupNoteHistory.setWhatHasChanged("generalchange");
    noEdit = true;
}

This sort of thing can be merged:

if (savedDate.after(fromUserDate) || fromUserDate.after(savedDate)) {
    groupNoteHistory.setWhatHasChanged("generalchange");
    noEdit = true;
}

You might want to use !savedDate.equals(fromUserDate), if that works the same as checking if either is bigger than the other.

Another example:

if (!noEdit) {
    noEdit = true;
    groupNoteHistory.setWhatHasChanged("zugweised");
    groupNoteHistory.setChangedMessage("Zuweisung von " +
            zugweisedPerson.getFirstName() + " entfernt");
} else {
    groupNoteHistory.setWhatHasChanged("generalchange");
    noEdit = true;
}

If both cases end up with noEdit as true, move it out of the if-statement.

if (!noEdit) {
    groupNoteHistory.setWhatHasChanged("zugweised");
    groupNoteHistory.setChangedMessage("Zuweisung von " +
            zugweisedPerson.getFirstName() + " entfernt");
} else {
    groupNoteHistory.setWhatHasChanged("generalchange");
}
noEdit = true;

Failing that, at the very least remove it from the else case - you KNOW it is true, there is no reason to set it to true again.

if (!noEdit) {
    groupNoteHistory.setWhatHasChanged("zugweised");
    groupNoteHistory.setChangedMessage("Hat die Note " + zugweisedPerson.getFirstName() + " zugewiesen");
    noEdit = true;
} else {
    groupNoteHistory.setWhatHasChanged("generalchange");
    noEdit = true;
}

Same here...

I think if you went through the code and cleaned mini-messes like those, you could chop off a quarter of the complexity.

if ((loggedInMember.isAccesslevel())) {

This check at the start, it wraps most of your code...

For starters, you have one pair of parentheses too many. Let's strip those:

if (loggedInMember.isAccesslevel()) {

Next, what you do when this if statement fails is

return "";

So given the rather large amount of indentation, I'd suggest that you'd negate the if statement and turn it into a guard clause:

if (!loggedInMember.isAccesslevel()) {
    return "";
}

Then the rest of your code can be indented one level less.

Heck, you might even be able to move this guard clause up, thus saving a bit on performance for that case. Performance isn't the main reason to do this though; Identifying guard clauses allows you to move them away from the code doing the actual work.

if ((!(noteObjectFromUser.getStartDateTimestamp() == null)) ||
    (!(databaseNoteObject.getStartDateTimestamp() == null))) {

    if ((noteObjectFromUser.getStartDateTimestamp() != null) &&
        (databaseNoteObject.getStartDateTimestamp() == null)) {
        if (!noEdit) {
            gantt = true;
        }
        noEdit = true;
    } else if ((noteObjectFromUser.getStartDateTimestamp() == null) &&
               (databaseNoteObject.getStartDateTimestamp() != null)) {
        if (!noEdit) {
            gantt = true;
        }
        noEdit = true;
    } else if ((!(noteObjectFromUser.getStartDateTimestamp() == null))) {

        Date fromUserDate = new Date(noteObjectFromUser.getStartDateTimestamp().getTime());
        Date savedDate = new Date(databaseNoteObject.getStartDateTimestamp().getTime());

        if (fromUserDate.after(savedDate)) {
            if (!noEdit) {
                gantt = true;
            }
            noEdit = true;
        }

        if (savedDate.after(fromUserDate)) {

            if (!noEdit) {
                gantt = true;
            }
            noEdit = true;
        }
    }
}

hmmm... looks suspicious.

Let's use a truth table. First, define the values...

noteObjectFromUser.getStartDateTimestamp() = A
databaseNoteObject.getStartDateTimestamp() = B

A == null = Anull
B == null = Bnull

Then simplify the code...

if (!Anull || !Bnull) {

    if (!Anull && Bnull) {
        if (!noEdit) {
            gantt = true;
        }
        noEdit = true;
    } else if (Anull && !Bnull) {
        if (!noEdit) {
            gantt = true;
        }
        noEdit = true;
    } else if (!Anull) {

        Date fromUserDate = new Date(A.getTime());
        Date savedDate = new Date(B.getTime());

        if (fromUserDate.after(savedDate) || savedDate.after(fromUserDate)) {
            if (!noEdit) {
                gantt = true;
            }
            noEdit = true;
        }
    }
}

I took the liberty of applying an earlier made suggestion.

Okay, so... the truth table:

```
Anull | Bnull | !Anull |!Bnull|(!Anull||!Bnull)|(!Anull&&Bnull)| (Anull&&!Bnull)
0 | 0 | 1 | 1 | 1 | 0 | 0
0 | 1 | 1 | 0 | 1 | 1 | 0
1 | 0 | 0 | 1 | 1 | 0 | 1
1 | 1 | 0 | 0 | 0

StackExchange Code Review Q#123104, answer score: 6