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 have the following code, but I'm looking for a way to make the code read easier.

public void synchronizePageAndExcecuteVerifyNode() throws Exception {
    String nameTestcase = this.getClass().getSimpleName();
    WriteTo.testgoal(nameTestcase, TestgoalState.nvt, "");

    Boolean testDataGenerated = eventTestData.eventDetailMultipleDays;
    Boolean currentPage = driver.getTitle().contains(Value.TITLE_EVENT_MULTIPLE_DAYS);

    if (testDataGenerated == true) {
        if(currentPage == true){
            this.verifyEventDetailFull();
        }else{
            WriteTo.testgoal(nameTestcase, TestgoalState.NOK,
                    "Page synchronize failed");
            throw new Exception("PAGE SYNCHRONIZE FAILED");
        }
    } else {
        WriteTo.testgoal(nameTestcase, TestgoalState.NOK,
                "testdata is not generated");
        throw new Exception("NO TESTDATA GENERATED");
    }
}
share|improve this question
2  
To make life easier for reviewers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. See also this meta question –  Simon André Forsberg yesterday
    
there is one function that creates a testdata node on a website by using the selenium library. After the creation of the testdata, i want to verify the page that was just created. Only before the the verify i want to check that the testdata is created and that I am on the correct page. –  Apjuh yesterday
6  
@Apjuh The comment should be included in the question. Comments are not the right place to make that kind of precision on a question –  Marc-Andre yesterday

5 Answers 5

up vote 15 down vote accepted

if (condition == true)

Instead of

if (testDataGenerated == true) {

just write

if (testDataGenerated) {

Rewriting ifs

You can move your checks to the top of the method like this to avoid nested ifs:

    if (!testDataGenerated) {
        WriteTo.testgoal(nameTestcase, TestgoalState.NOK,
                "testdata is not generated");
        throw new Exception("NO TESTDATA GENERATED");
    }
    if(!currentPage){
        WriteTo.testgoal(nameTestcase, TestgoalState.NOK,
                "Page synchronize failed");
        throw new Exception("PAGE SYNCHRONIZE FAILED");
    }
    this.verifyEventDetailFull();
share|improve this answer
    
Instinctively how I would have written that, as soon as I saw the poster code above my first thought was to handle the exceptions at the top. –  stephenbayer 23 hours ago
  • Are you sure that you need a Boolean? Considering how your code looks, currentPage and testDataGenerated cannot be null (if they would be then they would throw a NullPointerException. Use boolean instead which cannot be set to null
  • Try to avoid throwing Exception, create or use a subclass of Exception instead.
  • Speaking of "make the code read easier", there is no need to use CAPS LOCK FOR THE EXCEPTION MESSAGES. (Sorry, just had to show you what I mean).
  • Use spacing more consistently:

    if (testDataGenerated) {
    

    is better than

    if(currentPage){
    

    Just like } else { is better than }else{

share|improve this answer
1  
-1, formatting isn't better with spaces, nor without. I agree with consistency, but spacing or no spacing is entirely opinion based. –  Josue Espinosa 12 hours ago
    
Well, you can't just -1 for ONE part of an answer that seems opinion based, when the rest is good. –  Kilazur 7 hours ago
3  
@Kilazur sure he can. Although I would disagree. The oracle as well as the google style guide have a definite opinion on this. And even though there are no studies as to what is more readable, most Java programmers are used to the first version. (of course, any IDE can change this easily. But it can also change inconsistent spacing, which most people would agree is not acceptable.) –  tim 7 hours ago

The other answers have provide some good suggestions about the code formatting and styling, so here's mine on the logic.

The way I see it, you only need to call verifyEventDetailFull() when the test data is generated and currentPage is true (I'm not quite sure why the title containing a certain value is 'current page', but that's beyond my point). Otherwise, you 'handle the error' by saying that 'Page synchronize failed' if the test data is generated but currentPage is false, or say 'Test data is not generated' if that is indeed the case.

Given some slight, and in my humble opinion reasonable, assumptions based on the cases I have described above, I have an implementation for the handle error part:

private void handleError(final String testCaseName, final String errorDescription) {
    WriteTo.testgoal(testCaseName, TestgoalState.NOK, errorDescription);
    throw new Exception(errorDescription);
}

I think it's fair enough to use one errorDescription for printing some output and using it for the Exception message. I will now present the full suggestion:

public void synchronizePageAndExcecuteVerifyNode() throws Exception {
    final String testCaseName = this.getClass().getSimpleName();
    WriteTo.testgoal(testCaseName, TestgoalState.nvt, "");
    boolean isTestDataGenerated = eventTestData.eventDetailMultipleDays;
    if (isTestDataGenerated && driver.getTitle().contains(Value.TITLE_EVENT_MULTIPLE_DAYS)) {
        verifyEventDetailFull();
    } else {
        handleError(testCaseName, isTestDataGenerated ? "Page synchronize failed" : "Test data is not generated");
    }
}

private void handleError(final String testCaseName, final String errorDescription) {
    WriteTo.testgoal(testCaseName, TestgoalState.NOK, errorDescription);
    throw new Exception(errorDescription);
}

The only boolean value I need to store first is isTestDataGenerated. It is a good naming convention to begin boolean variables with is. Inside the else part, isTestDataGenerated allows us to switch between the correct error description. As mentioned above, we will only encounter the 'Page synchronize failed' case when the if condition holds true for isTestDataGenerated but false for 'current page'. Or you can simply read it as there is no test data generated, hence that error description in the false clause of the conditional operator.

share|improve this answer

Names of variables should be chosen well. If you name a variable currentPage, I would assume that it contains a page number (1, 2, 3, 4, etc). If you want a boolean variable that is true if we are on the current page, you could call it isCurrentPage. If you want a boolean variable that is true if synchronization to the current page succeeded (I have no idea what that means in the context, but that's what you seem to be checking), call it isSynchronizedToCurrentPage.

Especially when you change currentPage == true to currentPage, the first one indicates that currentPage is some kind of boolean. Reading if (currentPage) I only think "What???".

share|improve this answer

I would write like this to improve readability :

if (false == testDataGenerated) {
     WriteTo.testgoal(nameTestcase, TestgoalState.NOK,
            "testdata is not generated");
     throw new Exception("NO TESTDATA GENERATED");
}

if(false == currentPage){
    WriteTo.testgoal(nameTestcase, TestgoalState.NOK,
            "Page synchronize failed");
    throw new Exception("PAGE SYNCHRONIZE FAILED");
}

this.verifyEventDetailFull();
share|improve this answer

We're looking for long answers that provide some explanation and context. Don't just give a one-line answer; explain why your answer is right, ideally with citations. Answers that don't include explanations may be removed.

2  
Hey almaruf. This seems more like a comment to Tim's answer. Furthermore, I'm hesitant to say that false == is more readable than !... –  Schism yesterday
6  
Not using Yoda conditions, the first step to readability is. –  Paul Griffiths yesterday
    
@Schism - that was the point . Moreover, if the value of testDataGenerated is set to null than (! testDataGenerated) will pass (not sure about JAVA, but it is true for PHP), may be this isnt what you always want :) –  almaruf yesterday
    
@almaruf in Java: Main.java:12: error: bad operand type <null> for unary operator '!' –  Cruncher yesterday
1  
@BPugh: It doesn't have any merit for improving readability, which is what the question is concerned with. Even for preventing the type of bug you mention, any modern and sane compiler will alert you to this when you do it the normal way, so even that point is on shaky ground. Code Complete is a good book, but it has its fair share of dubious recommendations - in my view, this is one of them. –  Paul Griffiths 13 hours ago

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.