1
\$\begingroup\$
   //not an elf file. try PE parser
        PE pe=PEParser.parse(path);
        if(pe!=null)
        {
            PESignature ps =pe.getSignature();
            if(ps==null||!ps.isValid())
            {
                //What is it?
                Toast.makeText(this,"The file seems that it is neither an Elf file or PE file!",3).show();
                throw new IOException(e);
            }
        }
        else
        {
            //What is it?
            Toast.makeText(this,"The file seems that it is neither an Elf file or PE file!",3).show();
            throw new IOException(e);
        }

How can I organize the above code, so that

 //What is it?
          Toast.makeText(this,"The file seems that it is neither an Elf file or PE file!",3).show();
          throw new IOException(e);

Appears only once, or just be better-looking(easy to read)?

Summary

Please comment or advise on organizing the if statements.

\$\endgroup\$
  • 3
    \$\begingroup\$ The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How to Ask for examples, and revise the title accordingly. \$\endgroup\$ – Mast Oct 8 '18 at 16:31
2
\$\begingroup\$

Your business logic is getting muddled with your error-checking logic. I would recommend extracting your "null-to-Exception" conversions into different methods, so they don't appear here.

Ideally, PEParser.parse() and PE.getSignature() would not return null values, and would instead throw an exception directly if they encountered an issue. If you have the option to change these methods, I would do so. Otherwise, I would recommend wrapping them in methods that will convert null return values to exceptions, like so:

PE GetPEFromPath(String path)
{
    PE pe = PEParser.parse(path);
    if (pe == null)
    {
        throw new IOException();
    }
    return pe;
}

PESignature GetSignatureFromPE(PE pe)
{
    PESignature ps = pe.getSignature();
    if (ps == null || !ps.isValid())
    {
        throw new IOException();
    }
    return ps;
}

These methods should return a valid object, or throw an exception - no other options. This way, we can write our business logic in a try block with no null-check interruptions, and if something fails, we will move gracefully to the catch block.

Now, we can transform your given code snippet into something more readable:

//not an elf file. try PE parser
try
{
    PE pe = GetPEFromPath(path);
    PESignature ps = GetSignatureFromPE(pe);
}
catch (IOException e)
{
    Toast.makeText(this, "The file seems that it is neither an Elf file or PE file!", 3).show();
    throw e;
}
\$\endgroup\$
  • \$\begingroup\$ What do you think about this?stackoverflow.com/questions/51703711/… \$\endgroup\$ – KYHSGeekCode Oct 9 '18 at 2:05
  • 1
    \$\begingroup\$ @KYHSGeekCode Performance is not a major concern when using exceptions, since the moment the Exception gets thrown, a situation you didn't really want to handle occurred. At that point, you've strayed form the normal use-case path of the software. If a proper path is specified to a proper file, there is no problem, right? Like the comment there explains, only use them in exceptional situations and you're fine. \$\endgroup\$ – Mast Oct 9 '18 at 5:07
  • 1
    \$\begingroup\$ In addition to the comment by @Mast , avoiding Exceptions for the sake of performance without profiling your code is just a form of premature optimization. If I have the choice between more expressive code using Exception handling, and less expressive code that avoids Exceptions, I would take the former every time. If it turns out from profiling later that Exceptions are my bottleneck, I will restructure the code with that in mind, but until that point I would rather have the more expressive code. \$\endgroup\$ – cariehl Oct 9 '18 at 18:34

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.