Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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?

share|improve this question
1  
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 –  Marc-Andre Mar 14 at 17:24
    
If this is tagged with different language, then it may imply that it's example code. Such code is off-topic here. –  Jamal Mar 14 at 17:40
    
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'. –  konijn Mar 14 at 17:48
    
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 ;-). –  Zar Mar 14 at 17:49

6 Answers 6

up vote 10 down vote accepted

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 :
        .....
    ....
}
share|improve this answer
    
Nice, I really liked the enum solution; perhaps a bit overkill in my scenario, but nice. Thanks! –  Zar Mar 15 at 0:00

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
    }
    
share|improve this answer
    
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. –  cbojar Mar 14 at 18:49
1  
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. –  cbojar Mar 14 at 18:56
    
@cbojar - Yep, Java uses the same instance for identical static strings. –  Davor Mar 15 at 7:42

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)) {
    // ...
}
share|improve this answer

UPDATE

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 updateRequired = (newValue != null)
boolean deleteRequired = (currentValue != null)

if (updateRequired ^ deleteRequired) {
    //Play more Galaga
    //do More Work
}

Reference to this Comment and this Answer


OLD

I was thinking about a new answer and this just came to me.

I assume that you only want to update when newValue has something in it and you only want to delete when currentValue has something in it, so why don't you do something like this:

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

which would actually lead me to change it so that if you have something that needs to happen if one or the other is true then you would write it out like this

if (updateRequired || deleteRequired) {
    //play Galaga
    //do some stuff
}

share|improve this answer
1  
The problem with this is what if current value and new value are not null and are the same. You would update/delete even though it is unnecessary. –  cbojar Mar 14 at 18:58
    
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) {....} –  rolfl Mar 14 at 20:13
    
The if-statement behaves like the original, but the explanatory variables updateRequired and deleteRequired are no longer accurate. –  200_success Mar 14 at 22:59
    
@200_success there are some changes that I need to make to my answer. new understanding for what is going on in the question. I just haven't had the time to do it, I was moving offices. –  Malachi Mar 14 at 23:02

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.

share|improve this answer

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.

share|improve this answer
3  
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 :)). –  cbojar Mar 14 at 18:35
    
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? –  Malachi Mar 14 at 18:38

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.