current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 2 down vote favorite

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
    
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 3 hours ago
    
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 3 hours ago
1  
@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 1 hour ago

3 Answers 3

up vote 5 down vote

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
up vote 5 down vote
  • 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
up vote 0 down vote

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.
    
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 59 mins ago
1  
Not using Yoda conditions, the first step to readability is. –  Paul Griffiths 59 mins ago
    
@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 55 mins ago
    
@almaruf in Java: Main.java:12: error: bad operand type <null> for unary operator '!' –  Cruncher 2 mins 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.