7
\$\begingroup\$

I'm currently developing a method which performs an update of some data; much like the following simplified logic:

boolean updateRequired = (currentValue == null && newValue != null);
boolean deleteRequired = (currentValue != null && newValue == null);

if(updateRequired || deleteRequired) {

    // ...

}

Which probably is fine enough by itself. However, I can't loose the feeling that I'm somehow re-inventing the wheel. The above is just simplified, current/new-Value are strings if that matters.

Is there any way to make this more neat?

\$\endgroup\$
  • 1
    \$\begingroup\$ Is it c#, java or php? Your question is off-topic too, you need working and complete code to be that you want reviewed. You can have a language independent answer even if you specified which language you're using \$\endgroup\$ – Marc-Andre Mar 14 '14 at 17:24
  • \$\begingroup\$ If this is tagged with different language, then it may imply that it's example code. Such code is off-topic here. \$\endgroup\$ – Jamal Mar 14 '14 at 17:40
  • \$\begingroup\$ This question appears to be off-topic because it is about example code, not the actual code you wrote. This question would be more suitable for 'Programmers'. \$\endgroup\$ – konijn Mar 14 '14 at 17:48
  • \$\begingroup\$ I've edited the question to be specified to Java. I thought to make the question as broad as possible (as this isn't really language specific). Stackoverflow-damaged ;-). \$\endgroup\$ – Zar Mar 14 '14 at 17:49
10
\$\begingroup\$

Depending on your use case, I recommend two options. Both of them are based on a separate function for checking things....

At its simplest, have a function:

private static final boolean needToDoSomething(String currentValue, String newValue) {
    return currentValue == null && newValue != null || currentValue != null && newValue == null;
}

Then you can just have:

if (needToDoSomething(currentValue, newValue)) {
    ....
}

If you need to do something more special than just the 'needs modification' test, for example, you need to distinguish inside the if block between these conditions, then I would recommend an Enum with a static method, for example:

public enum EditState {
    ADDED, DELETED, MODIFIED, UNCHANGED;

    public static getState(String currentValue, String newValue) {
        if (currentValue == newValue) {
            // this covers null == null too
            return UNCHANGED;
        }
        if (currentValue != null && newValue == null) {
            return DELETED;
        }
        if (newValue != null && currentValue == null) {
            return ADDED;
        }
        return currentValue.equals(newValue) ? UNCHANGED : MODIFIED;
    }


    public boolean isStateChanging() {
        return this != UNCHANGED;
    }
}

Then, with this Enum class, you can be more specific:

EditState state = EditState.getState(currentValue, newValue);

switch (state) {
    case UNCHANGED :
        .....
    case DELETED :
        .....
    ....
}
\$\endgroup\$
  • \$\begingroup\$ Nice, I really liked the enum solution; perhaps a bit overkill in my scenario, but nice. Thanks! \$\endgroup\$ – Zar Mar 15 '14 at 0:00
4
\$\begingroup\$

In my opinion, you haven't included enough code to make this a good Code Review question, but I'll try to answer anyway.

If currentValue == null and newValue != null, then it seems to me that createRequired would be a better name than updateRequired.

You appear to be checking to see if exactly one of currentValue or newValue is null.

if ((currentValue == null) != (newValue == null)) {
    // ...
}
\$\endgroup\$
3
\$\begingroup\$

Those few lines of code are very confusing

  • If the value was null and now not null, you are really creating or inserting, not updating. I would call it insertRequired

    boolean updateRequired = (currentValue == null && newValue != null);
    
  • Why would you have an if condition that triggers for both inserting and deleting, it would not make any sense. It should be

    if( insertRequired ){
       //doSomething
    }
    else if( deleteRequired ){
       //doSomething else
    }
    
\$\endgroup\$
  • \$\begingroup\$ Actually, since this is Java, that doesn't necessarily work. String a = "String"; String b = "String"; a != b == true; == and != compare references, which may be different for equivalent string content. You would use .equals() but you'd still have to do null checks to make sure at least one wasn't null. \$\endgroup\$ – cbojar Mar 14 '14 at 18:49
  • 1
    \$\begingroup\$ Actually I just checked that and the example I gave does seem to give the same reference, but if you change it to String a = "String"; String b = new String("String");, then it has the expected effect. \$\endgroup\$ – cbojar Mar 14 '14 at 18:56
  • \$\begingroup\$ @cbojar - Yep, Java uses the same instance for identical static strings. \$\endgroup\$ – Davor Mar 15 '14 at 7:42
2
\$\begingroup\$

I think I know what you are looking for. you only want to do stuff if

  • x = false and y = true
  • x = true and y = false

and not when

  • x = false and y = false
  • x = true and y = true

in that case I would think that what you want is an XOR or an Exclusive Or

boolean isNewValueAssigned = newvalue != null;
boolean isCurrentValueAssigned = currentValue != null;
if (isNewValueAssigned ^ isCurrentValueAssigned) {
    //Play more Galaga
    //do More Work
}

I think that this makes it more clear what you are trying to do.

Reference to this Comment and this Answer

\$\endgroup\$
  • 1
    \$\begingroup\$ The XOR operator in Java ^ is inended for use as a bitwise XOR, not a logical XOR of booleans. For a logical check, use != like if (updateRequired != deleteRequired) {....} \$\endgroup\$ – rolfl Mar 14 '14 at 20:13
1
\$\begingroup\$

You could always condense it as follows:

boolean needToDoSomething = currentValue == null ? newValue != null : newValue == null;

if(needToDoSomething) {
    // ...
}

This is probably less clear than what you have, and gains you very little. Also, if you later branch based off of the individual values, you'd have to calculate them at that point anyway. In other words, probably best to leave it alone.

\$\endgroup\$
1
\$\begingroup\$

Just a thought I just had, what happens if both are false??


it looks like you have two separate Boolean variables that are meant to indicate two totally different things

  • Update
  • Delete

so really if this is the case you should leave the variables the way that they are and show us the rest of the code.

I imagine it looks something like this

boolean updateRequired = (currentValue == null && newValue != null);
boolean deleteRequired = (currentValue != null && newValue == null);

if (updateRequired) {
    object.Update();
} else if (deleteRequired) {
    object.Delete();
}

or maybe as two separate if statements altogether

if (updateRequired) {
    object.Update();
}

if (deleteRequired) {
    object.Delete();
}

I am betting you probably want to delete first and then insert the new data/object/whatever.


I looked at the question and it looks like you might want to delete first and then update/insert, if that is the case then you just need to flippy floppy the update and delete code.

\$\endgroup\$
  • 3
    \$\begingroup\$ You're assuming that he performs different operations whether it's an update or a delete. That isn't necessarily true (but it probably is :)). \$\endgroup\$ – cbojar Mar 14 '14 at 18:35
  • \$\begingroup\$ there is a major structure issue here if OP is doing the same thing when one or the other are true. if both are false what happens? \$\endgroup\$ – Malachi Mar 14 '14 at 18:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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