Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

Yesterday I had posted a question regarding optimizing editMethod here on code-review. Thanks to an awesome answer by Pimgd, I was able to merge conditional checks, checking conditions and few other things.

I am posting this as a follow-up question for the other question for further optimization of the method. All suggestions are welcome!.

Code now :

@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;

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

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

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

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

                Person zugweisedPerson = this.personService.getPersonById(noteObjectFromUser.getZugwisenPersonId());
                if (!(zugweisedPerson == null)) {
                    groupNoteHistory.setWhatHasChanged("zugweised");
                    groupNoteHistory.setChangedMessage("Hat die Note " + zugweisedPerson.getFirstName() + " zugewiesen");
                    gantt = false;
                    noEdit = true;
                }
            }
        }

        boolean hasStartFromUser = noteObjectFromUser.getStartDateTimestamp() != null;
        boolean hasStartFromDB = databaseNoteObject.getStartDateTimestamp() != null;

// For below line, also check screenshot attached. 
        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;
            }
        }

        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";
        }
    }

sendOutNotifications method :

 private void sendOutNotifications(Long groupAccountId, GroupCanvas groupCanvas, GroupSection groupSection,
                                      GroupNotes mnotes, GroupNoteHistory groupNoteHistory, int personid, String email,
                                      String firstName, String subject, String text, boolean gantt, String oldName) {

        List<GroupMembers> groupMembersList = this.groupMembersService.returnGroupMembers(groupAccountId);
        GroupAccount groupAccount = this.groupAccountService.getGroupById(groupAccountId);
        Person editingUser = this.personService.getPersonById(personid);
        String type = "note";
        if (gantt)
            type = "gantt";
        for (GroupMembers groupMembers : groupMembersList) {
            Notification notification = this.notificationService.createNotification(personid, true, "/section/listing/" + groupCanvas.getMcanvasid() + "?note=" + mnotes.getMnoticesid(), subject, text,
                    type, groupSection.getMsectionid(), groupCanvas.getMcanvasid(), mnotes.getMnoticesid(), mnotes.getMnotecolor());

            if (!(groupNoteHistory.getWhatHasChanged() == null)) {
                if (groupNoteHistory.getWhatHasChanged().equals("generalchange")) {
                    notification.setSubject(groupAccount.getGroupName() + ", " + groupCanvas.getMcanvasname());
                    notification.setText("Note editiert: <span class='bold'>" + mnotes.getMnotetag() + "</span> ");
                } else {
                    if (groupNoteHistory.getWhatHasChanged().equals("colorchange")) {
                        notification.setSubject(groupAccount.getGroupName() + ", " + groupCanvas.getMcanvasname());
                        notification.setText("Note Farbe geändert: <span class='bold'>" + mnotes.getMnotetag() + "</span> ");
                    }
                    if (groupNoteHistory.getWhatHasChanged().equals("tagchange")) {
                        notification.setSubject(groupAccount.getGroupName() + ", " + groupCanvas.getMcanvasname());
                        notification.setText("Note Überschrift geändert: <span class='bold'>" + oldName + "</span> " + " in <span class='bold'> " + mnotes.getMnotetag() + "</span>");
                    }
                    if (groupNoteHistory.getWhatHasChanged().equals("textchange")) {
                        notification.setSubject(groupAccount.getGroupName() + ", " + groupCanvas.getMcanvasname());
                        notification.setText("Note Text geändert: <span class='bold'>" + mnotes.getMnotetag() + "</span> ");
                    }
                    if (groupNoteHistory.getWhatHasChanged().equals("zugweised")) {
                        notification.setSubject(groupAccount.getGroupName() + ", " + groupCanvas.getMcanvasname());
                        notification.setText("Note <span class='bold'>" + mnotes.getMnotetag() + "</span> " + editingUser.getFirstName() + " zugewiesen");
                    }
                }
            }

            this.chatService.sendNotification(notification, groupMembers.getMemberid());

            if (!(groupMembers.getMemberUsername().equals(email)) && (!gantt)) {
                this.notificationService.addNotification(notification, groupMembers.getMemberid());
                UnreadNotes unreadNotes = this.unreadNotesService.createUnreadNoteEntry(groupAccountId, groupMembers.getMemberUsername(), groupCanvas.getMcanvasid());
                this.unreadNotesService.saveUnreadNoteEntry(unreadNotes, mnotes.getMnoticesid());

            }
        }
        this.noteLockService.deleteNoteLockForUser(mnotes.getMnoticesid());
    }

Screenshot :

Warning screenshot

share|improve this question
1  
As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. Please see How to get the best value out of Code Review - Asking Questions for guidance on writing good question titles. – BCdotWEB Mar 18 at 10:33
    
Huh, that looks weird, gimme a few mins to look at that warning – Pimgd Mar 18 at 10:45
    
@Pimgd : Please take your time. Thank you. ::-) – We are Borg Mar 18 at 10:48
up vote 5 down vote accepted

So guess what!

When you've got two values...

0 0
0 1
1 0
1 1

And eliminate the cases where they're not the same...

0 0
1 1

They are the same! So checking both is not needed, which is the warning you're seeing.

So the IDE is correct, the second half can go.

This warning probably made you wary to implement the suggested changes all the way, but most of the stuff I pointed out earlier is present in your code in the same way.

Specifically:

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

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

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

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

        Person zugweisedPerson = this.personService.getPersonById(noteObjectFromUser.getZugwisenPersonId());
        if (!(zugweisedPerson == null)) {
            groupNoteHistory.setWhatHasChanged("zugweised");
            groupNoteHistory.setChangedMessage("Hat die Note " + zugweisedPerson.getFirstName() + " zugewiesen");
            gantt = false;
            noEdit = true;
        }
    }
}

Seem familiar?

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 (savedDate.after(fromUserDate) || fromUserDate.after(savedDate)) {
            if (!noEdit) {
                gantt = true;
            }
            noEdit = true;
        }

    }
}

I sure hope so, because it's pretty much the same, except for an added else case.

So let's grab the table from last time:

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

That's the case that decides whether you get in the if statement...

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

And this is the case that decides whether you REACH the if statement.

Oh snap! They're the same!

I'm serious - when you have !a OR !b, one of them must be set. Then you check for both cases where only one is set. So the only remaining case is the case where they're both set; there is no option where none are set!

So you can remove the extra code and simplify it in the same manner:

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;
    }
}

Aren't conditionals fun.

Of course, I say it never gets executed; be sure to check what the semantics of that case were, perhaps you've got a bug!


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.


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.

share|improve this answer
    
One confusion, in your boolean hasNoteTargetFromUser, there is an else clause originally, where I am calling setChangedMessage(). The code you suggested doesn't have this. Are you sure about splitting up the method even more, where two checks are done externally. I ask because already the method is split into two parts, where one part handles sending out notifications. I am updating the main post to include that method as well. Gives a basic idea. – We are Borg Mar 18 at 12:08
    
Added the else clause accordingly for the first problem I mentioned. – We are Borg Mar 18 at 12:32
    
@WeareBorg as you can see, it's impossible for that else to trigger. You can never get into that case. Assign the conditionals to booleans and see what your IDE tells you. And YES I suggest splitting off checks to separate functions; ideally this function would consist of about 8 function calls, each performing 1 "step". – Pimgd Mar 18 at 12:43
    
Thank you for the changes. Took some time and testing, but was able to integrate them and I am happy to inform the tests passed. Manual testing as well as via Selenium. Thank you. Do you really think the method should be broken into 8 parts? I wouldn't mind creating another question and to be honest I am curious. Ur call !!! – We are Borg Mar 18 at 13:36
    
@WeareBorg I think it should most definitely be split up more. Followup question would be great; we're almost there. – Pimgd Mar 18 at 13:41

Since Pimgd already wrote a really nice review on the editGroupNote already, I'll go for the sendOutNotification-method:

private void sendOutNotifications(Long groupAccountId, GroupCanvas groupCanvas, GroupSection groupSection,
                                      GroupNotes mnotes, GroupNoteHistory groupNoteHistory, int personid, String email,
                                      String firstName, String subject, String text, boolean gantt, String oldName) {

Let's just go and count the arguments there for a moment... 12

You should be running around in circles, screaming, when you see a method like this in your code.

There's two possibilities:

  1. That method does too many things (and usually has a name from here to the moon and back)
  2. Your use of arguments is improperly scoped, the method should be split or otherwise restructured. Consider a wrapper object for the arguments.

In your case this looks a lot like the latter.

Here's a few things that I noticed while skimming over that code:

You retrieve a List<GroupMembers> from your group. From a semantic point of view that does not make sense. A class generally encapsulates a single thing. As such it should be a List<GroupMember>...

This also makes the loop easier to understand (at least for me personally)


    Person editingUser = this.personService.getPersonById(personid);

You have a person in the calling method, yet you pass the personId as well as two additional fields of the person to this method. This doesn't make sense. Instead you should pass the person to the method directly.
This alone gets rid of two arguments.

Similarly applies to groupAccount where you actually pass the groupAccountId you retrieve from the groupCanvas in your edit-method. And that after you had already retrieved that very groupAccount as "ownedAccount" in your calling method.

I assume asking your Services for an instance is not really cheap. Don't needlessly do it. Manage and reuse your instances where you can. It's faster, cleaner and usually results in code easier to maintain.


    String type = "note";
    if (gantt)
        type = "gantt";

this could be shorter:

final String type = gantt ? "gantt" : "note";

this also has the added benefit of immutability. You don't have to worry about it down the line.


 for (GroupMembers groupMembers : groupMembersList) {
            Notification notification = this.notificationService.createNotification(personid, true, "/section/listing/" + groupCanvas.getMcanvasid() + "?note=" + mnotes.getMnoticesid(), subject, text,
                    type, groupSection.getMsectionid(), groupCanvas.getMcanvasid(), mnotes.getMnoticesid(), mnotes.getMnotecolor());

You create a Notification for each member in a Group. Interestingly that notification doesn't even care about anything that belongs to the groupMember.

Since you call into a Service to create that Notification, I assume that creating these isn't a simple matter of creating an Object, but involves I/O of some kind.

Assuming that you can deliver a single notification to many different users you should be able to move this out of the loop:

final Notification notification = this.notificationService.createNotification(personid, true, "/section/listing/" + groupCanvas.getMcanvasid() + "?note=" + mnotes.getMnoticesid(), subject, text,
          type, groupSection.getMsectionid(), groupCanvas.getMcanvasid(), mnotes.getMnoticesid(), mnotes.getMnotecolor());
for (GroupMembers groupMembers : groupMembersList) {

   if (!(groupNoteHistory.getWhatHasChanged() == null)) {
       if (groupNoteHistory.getWhatHasChanged().equals("generalchange")) {
           notification.setSubject(groupAccount.getGroupName() + ", " + groupCanvas.getMcanvasname());
           notification.setText("Note editiert: <span class='bold'>" + mnotes.getMnotetag() + "</span> ");
       } else {
           if (groupNoteHistory.getWhatHasChanged().equals("colorchange")) {
               notification.setSubject(groupAccount.getGroupName() + ", " + groupCanvas.getMcanvasname());
               notification.setText("Note Farbe geändert: <span class='bold'>" + mnotes.getMnotetag() + "</span> ");
           }
           if (groupNoteHistory.getWhatHasChanged().equals("tagchange")) {
               notification.setSubject(groupAccount.getGroupName() + ", " + groupCanvas.getMcanvasname());
               notification.setText("Note Überschrift geändert: <span class='bold'>" + oldName + "</span> " + " in <span class='bold'> " + mnotes.getMnotetag() + "</span>");
           }
           if (groupNoteHistory.getWhatHasChanged().equals("textchange")) {
               notification.setSubject(groupAccount.getGroupName() + ", " + groupCanvas.getMcanvasname());
               notification.setText("Note Text geändert: <span class='bold'>" + mnotes.getMnotetag() + "</span> ");
           }
           if (groupNoteHistory.getWhatHasChanged().equals("zugweised")) {
               notification.setSubject(groupAccount.getGroupName() + ", " + groupCanvas.getMcanvasname());
               notification.setText("Note <span class='bold'>" + mnotes.getMnotetag() + "</span> " + editingUser.getFirstName() + " zugewiesen");
           }
       }
   }

This whole thing is display logic. Don't mingle abstraction levels... ever

This is display logic, something that shouldn't even be here. It's supposed to happen when you actually display the Notification to your end-user and not when you create it.

share|improve this answer
    
Thank you for the answer. I agree it is bad to put display logic in here. The only other option was to insert in JS. But as the notification would remain the same, why to generate them again and again. So we generate them, save in DB, and PUSH via Cometd... Why is this bad? About the creation of Notification object, it doesn't require IO, that's colleagues doing. I put it in constructor now. But I can't take it outside for-loop, reason.. the object is simply not sent & persisted after the first GroupMember. Why is that happening? Any ideas, because it baffled me also the first time. Continued. – We are Borg Mar 18 at 13:33
    
Also, I have reduced the parameters required for the method to 8. I have made some changes. How would you like to proceed? Kindly let me know. I also have it on pastebin : pastebin.com/CrEqC6LC . – We are Borg Mar 18 at 13:34
    
I will wait for your reply. Thank you. I have also created a new question which contains the updated method of sendOutNotifications based upon your suggestion. Kindly have a look. codereview.stackexchange.com/questions/123216/… – We are Borg Mar 18 at 13:59

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.