Return to Answer

Leaves us with this..

@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("\\s*id=\"cke[^\">]*\"", ""));
 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());
 GroupMembers loggedInMember = this.groupMembersService.returnMembersMatchingUsernameAccountId(person.getUsername(), ownedAccount.getGroupId());
 if (!(loggedInMember.isAccesslevel())) {
 return "";
 }
 String oldText = databaseNoteObject.getMnotetext();
 String oldTag = databaseNoteObject.getMnotetag();
 databaseNoteObject.setMnotetext(databaseNoteObject.getMnotetext().replaceAll("\\s*id=\"cke[^\">]*\"", ""));
 noteObjectFromUser.setCreatorId(databaseNoteObject.getCreatorId());
 noteObjectFromUser.setMnotetext(noteObjectFromUser.getMnotetext().replaceAll("\\r?\\n", "<br/>"));
 noteObjectFromUser.setMnotetag(noteObjectFromUser.getMnotetag().replaceAll("\\r?\\n", "<br/>"));
 noteObjectFromUser.setNoteCreatorEmail(databaseNoteObject.getNoteCreatorEmail());
 boolean noEdit = false;
 GroupNoteHistory groupNoteHistory = new GroupNoteHistory();
 if (!(noteObjectFromUser.getMnotetext().equals(databaseNoteObject.getMnotetext()))) {
 String newText = "";
 if (isLineDifferent(databaseNoteObject.getMnotetext(), noteObjectFromUser.getMnotetext())) {
 diff_match_patch diffMatchPatch = new diff_match_patch();
 LinkedList<diff_match_patch.Diff> deltas = diffMatchPatch.diff_main(databaseNoteObject.getMnotetext(), noteObjectFromUser.getMnotetext());
 diffMatchPatch.diff_cleanupSemantic(deltas);
 newText += diffMatchPatch.diff_prettyHtml(deltas);
 groupNoteHistory.setWhatHasChanged("textchange");
 noEdit = true;
 }
 newText = newText.replaceAll("<li>", "<div class=‘list‘>");
 newText = newText.replaceAll("</li>", "</div>");
 groupNoteHistory.setNewNoteText(newText);
 groupNoteHistory.setWhatHasChanged("textchange");
 }
 if (!(noteObjectFromUser.getMnotecolor().equals(databaseNoteObject.getMnotecolor()))) {
 if (noEdit) {
 groupNoteHistory.setWhatHasChanged("generalchange");
 } else {
 groupNoteHistory.setWhatHasChanged("colorchange");
 groupNoteHistory.setChangedMessage(databaseNoteObject.getMnotecolor());
 }
 groupNoteHistory.setChangedMessage(part1 + noteColorSubject + part2 + noteObjectFromUser.getMnotecolor() + part3);
 noEdit = true;
 }
 if (!(noteObjectFromUser.getMnotetag().equals(databaseNoteObject.getMnotetag()))) {
 groupNoteHistory.setNewNoteHeadline(noteObjectFromUser.getMnotetag());
 if (noEdit) {
 groupNoteHistory.setWhatHasChanged("generalchange");
 } else {
 groupNoteHistory.setWhatHasChanged("tagchange");
 groupNoteHistory.setNewNoteHeadline(noteObjectFromUser.getMnotetag());
 groupNoteHistory.setOldHeadLine(databaseNoteObject.getMnotetag());
 }
 noEdit = true;
 }
 if (!(noteObjectFromUser.getZugwisenPersonId() == databaseNoteObject.getZugwisenPersonId())) {
 if ((databaseNoteObject.getZugwisenPersonId() != 0) && (noteObjectFromUser.getZugwisenPersonId() == 0)) {
 Person zugweisedPerson = this.personService.getPersonById(databaseNoteObject.getZugwisenPersonId());
 if (!noEdit) {
 groupNoteHistory.setWhatHasChanged("zugweised");
 groupNoteHistory.setChangedMessage("Zuweisung von " + zugweisedPerson.getFirstName() + " entfernt");
 } else {
 groupNoteHistory.setWhatHasChanged("generalchange");
 }
 noEdit = true;
 updateStatisticsForZugweisen(zugweisedPerson.getId(), false, ownedAccount.getGroupId(),
 ownedCanvas.getMcanvasid(), noteObjectFromUser.getMnoticesid());
 } else {
 Person zugweisedPerson = this.personService.getPersonById(noteObjectFromUser.getZugwisenPersonId());
 if (!(zugweisedPerson == null)) {
 noteObjectFromUser.setPersonWhoZugweised(person.getId());
 if (!noEdit) {
 groupNoteHistory.setWhatHasChanged("zugweised");
 groupNoteHistory.setChangedMessage("Hat die Note " + zugweisedPerson.getFirstName() + " zugewiesen");
 } else {
 groupNoteHistory.setWhatHasChanged("generalchange");
 }
 noEdit = true;
 updateStatisticsForZugweisen(zugweisedPerson.getId(), true, ownedAccount.getGroupId(), ownedCanvas.getMcanvasid(), noteObjectFromUser.getMnoticesid());
 }
 }
 }
 boolean gantt = false;
 boolean noteTargetTimeChanged = checkNoteTargetTime(noteObjectFromUser.getNoteTarget(), databaseNoteObject.getNoteTarget());
 boolean startDateChanged = checkStartDateChange(noteObjectFromUser.getStartDateTimestamp(), databaseNoteObject.getStartDateTimestamp());
 if(noteTargetTimeChanged || startDateChanged){
 if (!noEdit) {
 gantt = true;
 }
 noEdit = true;
 }
 if (noEdit) {
 noteObjectFromUser.setNoteCreationTime(databaseNoteObject.getNoteCreationTime());
 noteObjectFromUser.setNoteCreatorEmail(databaseNoteObject.getNoteCreatorEmail());
 noteObjectFromUser.setMnotetext(latestText);
 databaseNoteObject.setMnotetext(oldText);
 this.groupNotesDAO.editGroupNote(noteObjectFromUser, msectionId);
 if (checkIfEvernote) {
 this.evernoteService.pushNoteToEvernote(noteObjectFromUser.getMnoticesid());
 }
 groupNoteHistory.setMnoteEditDate(new Timestamp(System.currentTimeMillis()));
 groupNoteHistory.setEditorId(person.getId());
 if (!gantt) {
 this.groupNoteHistoryService.addNoteHistory(groupNoteHistory, noteObjectFromUser.getMnoticesid());
 this.noteActivityService.saveGroupNotActivity(noteObjectFromUser.getMnoticesid());
 }
 noteObjectFromUser.setActiveEdit(false);
 noteObjectFromUser.setNoteDate(noteObjectFromUser.getNoteDate());
 int canvasId = retrievedSection.getCurrentCanvasId();
 GroupCanvas groupCanvas = this.groupCanvasService.getCanvasById(canvasId);
 Long groupAccountId = groupCanvas.getGroupAccountId();
 this.unreadNotesService.deleteEntriesForAnote(databaseNoteObject.getMnoticesid());
 this.notificationService.deleteNotificationsForNote(databaseNoteObject.getMnoticesid());
 sendOutNotifications(groupAccountId, groupCanvas, retrievedSection, noteObjectFromUser, groupNoteHistory, person.getId(), person.getUsername(), person.getFirstName(),
 groupNoteHistory.getWhatHasChanged(), groupNoteHistory.getChangedMessage(), gantt, oldTag);
 return "edit";
 } else {
 return "nochange";
 }
}

Of interest, again, this region:

 boolean gantt = false;
 boolean noteTargetTimeChanged = checkNoteTargetTime(noteObjectFromUser.getNoteTarget(), databaseNoteObject.getNoteTarget());
 boolean startDateChanged = checkStartDateChange(noteObjectFromUser.getStartDateTimestamp(), databaseNoteObject.getStartDateTimestamp());
 if(noteTargetTimeChanged || startDateChanged){
 if (!noEdit) {
 gantt = true;
 }
 noEdit = true;
 }
 if (noEdit) {

There's no real need for all those checks if noEdit is already true. After all, it's not going to change the gantt variable. So relocate the !noEdit check:

 boolean gantt = false;
 if (!noEdit) {
 boolean noteTargetTimeChanged = checkNoteTargetTime(noteObjectFromUser.getNoteTarget(), databaseNoteObject.getNoteTarget());
 boolean startDateChanged = checkStartDateChange(noteObjectFromUser.getStartDateTimestamp(), databaseNoteObject.getStartDateTimestamp());
 
 if(noteTargetTimeChanged || startDateChanged){
 gantt = true;
 noEdit = true;
 }
 }
 if (noEdit) {

But... you know, if noEdit is false, and it's still false, then you

 } else {
 return "nochange";
 }

return "nochange". So we can take that return variable and move it to the else case of

 if(noteTargetTimeChanged || startDateChanged){
 gantt = true;
 noEdit = true;
 }

Because then it's already clear. It does make things more messy, but we're gonna clean that up too, promise.

 boolean gantt = false;
 if (!noEdit) {
 boolean noteTargetTimeChanged = checkNoteTargetTime(noteObjectFromUser.getNoteTarget(), databaseNoteObject.getNoteTarget());
 boolean startDateChanged = checkStartDateChange(noteObjectFromUser.getStartDateTimestamp(), databaseNoteObject.getStartDateTimestamp());
 
 if(noteTargetTimeChanged || startDateChanged){
 gantt = true;
 } else {
 return "nochange";
 }
 }

The if check of noEdit afterwards can go, because we just relocated the else case. Incidentally, this means that the setting of noEdit = true can go away from the noteTargetTimeChanged || startDateChanged check, because it's never used again afterward.

But the flow is still weird, because...

If you go into that block, either gantt turns true or you return.

So we could do this:

 boolean gantt = !noEdit;
 if (gantt) {
 boolean noteTargetTimeChanged = checkNoteTargetTime(noteObjectFromUser.getNoteTarget(), databaseNoteObject.getNoteTarget());
 boolean startDateChanged = checkStartDateChange(noteObjectFromUser.getStartDateTimestamp(), databaseNoteObject.getStartDateTimestamp());
 
 if(!noteTargetTimeChanged && !startDateChanged){
 return "nochange";
 }
 }

It's your choice whether you put gantt or !noEdit in the if statement for the checks - pick one that makes the most semantic sense.

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("\\s*id=\"cke[^\">]*\"", ""));
 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());
 GroupMembers loggedInMember = this.groupMembersService.returnMembersMatchingUsernameAccountId(person.getUsername(), ownedAccount.getGroupId());
 if (!(loggedInMember.isAccesslevel())) {
 return "";
 }

That accesslevel check looks awfully late.

Explaining the chain of usages would be very lengthy and much easier in person, but basically, you only need these:

GroupSection retrievedSection = this.groupSectionService.getGroupSectionById(msectionId);
GroupCanvas ownedCanvas = this.groupCanvasService.getCanvasById(retrievedSection.getCurrentCanvasId());
GroupAccount ownedAccount = this.groupAccountService.getGroupById(ownedCanvas.getGroupAccountId());
GroupMembers loggedInMember = this.groupMembersService.returnMembersMatchingUsernameAccountId(person.getUsername(), ownedAccount.getGroupId());
if (!(loggedInMember.isAccesslevel())) {
 return "";
}

The rest of all the declarations should be moved to be after this check. I'd even be inclined to make the last check, this part

GroupMembers loggedInMember = this.groupMembersService.returnMembersMatchingUsernameAccountId(person.getUsername(), ownedAccount.getGroupId());
if (!(loggedInMember.isAccesslevel())) {
 return "";
}

Into a separate function, taking a username and groupid. Something like "isAuthorizedMemberOf" or something like that.

There's still some duplication here:

 if (!(noteObjectFromUser.getZugwisenPersonId() == databaseNoteObject.getZugwisenPersonId())) {
 if ((databaseNoteObject.getZugwisenPersonId() != 0) && (noteObjectFromUser.getZugwisenPersonId() == 0)) {
 Person zugweisedPerson = this.personService.getPersonById(databaseNoteObject.getZugwisenPersonId());
 if (!noEdit) {
 groupNoteHistory.setWhatHasChanged("zugweised");
 groupNoteHistory.setChangedMessage("Zuweisung von " + zugweisedPerson.getFirstName() + " entfernt");
 } else {
 groupNoteHistory.setWhatHasChanged("generalchange");
 }
 noEdit = true;
 updateStatisticsForZugweisen(zugweisedPerson.getId(), false, ownedAccount.getGroupId(),
 ownedCanvas.getMcanvasid(), noteObjectFromUser.getMnoticesid());
 } else {
 Person zugweisedPerson = this.personService.getPersonById(noteObjectFromUser.getZugwisenPersonId());
 if (!(zugweisedPerson == null)) {
 noteObjectFromUser.setPersonWhoZugweised(person.getId());
 if (!noEdit) {
 groupNoteHistory.setWhatHasChanged("zugweised");
 groupNoteHistory.setChangedMessage("Hat die Note " + zugweisedPerson.getFirstName() + " zugewiesen");
 } else {
 groupNoteHistory.setWhatHasChanged("generalchange");
 }
 noEdit = true;
 updateStatisticsForZugweisen(zugweisedPerson.getId(), true, ownedAccount.getGroupId(), ownedCanvas.getMcanvasid(), noteObjectFromUser.getMnoticesid());
 }
 }
 }

I wish I could do something about it, but as you'll see, I get stuck.

The original check, I get it. Something like "if ticket reassigned".

But after that, you're pretty much interested in only one task: get a person object, change note status, maybe give it a message depending on situation...

So maybe we can do something about that...

First, extract case into boolean.

boolean fromDatabase = (databaseNoteObject.getZugwisenPersonId() != 0) && (noteObjectFromUser.getZugwisenPersonId() == 0);
if (fromDatabase) {
 Person zugweisedPerson = this.personService.getPersonById(databaseNoteObject.getZugwisenPersonId());
 if (!noEdit) {
 groupNoteHistory.setWhatHasChanged("zugweised");
 groupNoteHistory.setChangedMessage("Zuweisung von " + zugweisedPerson.getFirstName() + " entfernt");
 } else {
 groupNoteHistory.setWhatHasChanged("generalchange");
 }
 noEdit = true;
 updateStatisticsForZugweisen(zugweisedPerson.getId(), false, ownedAccount.getGroupId(),
 ownedCanvas.getMcanvasid(), noteObjectFromUser.getMnoticesid());
} else {
 Person zugweisedPerson = this.personService.getPersonById(noteObjectFromUser.getZugwisenPersonId());
 if (!(zugweisedPerson == null)) {
 noteObjectFromUser.setPersonWhoZugweised(person.getId());
 if (!noEdit) {
 groupNoteHistory.setWhatHasChanged("zugweised");
 groupNoteHistory.setChangedMessage("Hat die Note " + zugweisedPerson.getFirstName() + " zugewiesen");
 } else {
 groupNoteHistory.setWhatHasChanged("generalchange");
 }
 noEdit = true;
 updateStatisticsForZugweisen(zugweisedPerson.getId(), true, ownedAccount.getGroupId(), ownedCanvas.getMcanvasid(), noteObjectFromUser.getMnoticesid());
 }
}

Next, retrieve person using boolean and move the rest of the code out of the switched case... then apply the boolean where there's any changes.

boolean fromDatabase = (databaseNoteObject.getZugwisenPersonId() != 0) && (noteObjectFromUser.getZugwisenPersonId() == 0);
Person zugweisedPerson = null;
if (fromDatabase) {
 zugweisedPerson = this.personService.getPersonById(databaseNoteObject.getZugwisenPersonId());
} else {
 zugweisedPerson = this.personService.getPersonById(noteObjectFromUser.getZugwisenPersonId());
}
if (!(zugweisedPerson == null)) {
 if(!fromDatabase){
 noteObjectFromUser.setPersonWhoZugweised(person.getId());
 }
 if (!noEdit) {
 groupNoteHistory.setWhatHasChanged("zugweised");
 if(fromDatabase){
 groupNoteHistory.setChangedMessage("Zuweisung von " + zugweisedPerson.getFirstName() + " entfernt");
 } else {
 groupNoteHistory.setChangedMessage("Hat die Note " + zugweisedPerson.getFirstName() + " zugewiesen");
 }
 } else {
 groupNoteHistory.setWhatHasChanged("generalchange");
 }
 noEdit = true;
 updateStatisticsForZugweisen(zugweisedPerson.getId(), !fromDatabase, ownedAccount.getGroupId(), ownedCanvas.getMcanvasid(), noteObjectFromUser.getMnoticesid());
}

It still looks messy and it's not cleaned up all that much. I'd roll this one back; the duplication is gone but the code is twice as messy. It's harder to follow, now. I left it in the answer so that you might get ideas on how to clean it up; I think if you could somehow supply the person and remove the noteObjectFromUser change, it could be easily wrapped in a function.

boolean checkIfEvernote = (noteSelection != null);

Move this to a place where it's used, because it has only 1 usage.

The only reason I can do all this is because your code used to work. If a bug happens, it could be in the refactoring we did, but far more likely that the bug ALREADY existed. So if a bug happens, don't panic; either the bug was already a bug or the refactoring is wrong. And the refactoring has step-by-step documentation, so you can test it step-by-step.

More to come

So, we've significantly reduced the length of your code now, it looks a lot more managable. The next step in cleaning complex code like this is identifying subfunctions.

And, actually, the area I have been shooting at all this time is a prime candidate for this!

Take another look at them.

boolean hasNoteTargetFromUser = noteObjectFromUser.getNoteTarget() != null;
boolean hasNoteTargetFromDB = databaseNoteObject.getNoteTarget() != null;
if (hasNoteTargetFromUser != hasNoteTargetFromDB ) {
 if (!noEdit) {
 gantt = true;
 }
 noEdit = true;
} else if(hasNoteTargetFromUser){
 Date fromUserDate = new Date(noteObjectFromUser.getNoteTarget().getTime());
 Date savedDate = new Date(databaseNoteObject.getNoteTarget().getTime());
 if (fromUserDate.after(savedDate) || savedDate.after(fromUserDate)) {
 if (!noEdit) {
 gantt = true;
 }
 noEdit = true;
 }
}

and

boolean hasStartFromUser = noteObjectFromUser.getStartDateTimestamp() != null;
boolean hasStartFromDB = databaseNoteObject.getStartDateTimestamp() != null;
if (hasStartFromUser != hasStartFromDB) {
 if (!noEdit) {
 gantt = true;
 }
 noEdit = true;
} else if(hasStartFromUser && hasStartFromDB){
 Date fromUserDate = new Date(noteObjectFromUser.getStartDateTimestamp().getTime());
 Date savedDate = new Date(databaseNoteObject.getStartDateTimestamp().getTime());
 if (fromUserDate.after(savedDate) || savedDate.after(fromUserDate)) {
 if (!noEdit) {
 gantt = true;
 }
 noEdit = true;
 }
}

You've got 1 result here, that you use to decide whether you should or shouldn't execute

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

That bit of code.

So what if we made a function whose job it is to tell you whether you should?

Something that could give us this:

boolean noteTargetTimeChanged = checkNoteTargetTime(noteObjectFromUser.getNoteTarget(), databaseNoteObject.getNoteTarget());
boolean startDateChanged = checkStartDateChange(noteObjectFromUser.getStartDateTimestamp(), databaseNoteObject.getStartDateTimestamp());
if(noteTargetTimeChanged || startDateChanged){
 if (!noEdit) {
 gantt = true;
 }
 noEdit = true;
}

Well, we could, because there are no side-effects in that code. So move it to a separate function like so:

boolean checkNoteTargetTime(??NoteTarget?? fromUser, ??NoteTarget?? fromDatabase){
 boolean hasNoteTargetFromUser = fromUser != null;
 boolean hasNoteTargetFromDB = fromDatabase != null;
 if (hasNoteTargetFromUser != hasNoteTargetFromDB) {
 return true;
 } else if(hasNoteTargetFromUser){
 Date fromUserDate = new Date(fromUser.getTime());
 Date savedDate = new Date(fromDatabase.getTime());
 return (fromUserDate.after(savedDate) || savedDate.after(fromUserDate));
 }
 return false;
}

I don't know the type, I assume you do, though.

You can do the same for checkStartDateChange.