5
\$\begingroup\$

Following function will parse string resp, which will be like shown in following example. I will get an IP address in the form of a hexadecimal string like "C0A2AA32" and I need to convert that string and save it to a file.

Example of resp: "This is just a string with ip="2342A022" now I need to parse it"

void WritePublicIPAddressToFile(String resp)
{
    //find string in response
    String stringToFind = "ip="; 

    //get index of stringToFind
    int indexOfIP = resp.indexOf(stringToFind);

    String newString = "";

    //find exact ip address which will be after = in stringToFind 
    for(int i = indexOfIP; i<resp.length(); i++)
    {
        while(resp.charAt(i++) != '\"');

        while(resp.charAt(i) != '\"')
        {
            newString += resp.charAt(i++); 
        }

        break;
    }

    int lenNewString = newString.length();
    String newIPAddress = "";

    //get two bytes from hex string and convert to ipaddress 
    if(lenNewString == 8) //lenth of ipaddress raw data should be 8
    {
        long l;
        for(int i = 0;i<lenNewString;i+=2)
        {
             l = Long.parseLong(String.format("%c%c", newString.charAt(i),newString.charAt(i+1)),16);
             newIPAddress += String.format("%d.", l);
        }
    }

    StringBuilder sb = new StringBuilder(newIPAddress);
    sb.deleteCharAt(sb.length() - 1);//remvoe last '.' in ipaddress



    //final ipaddress to save in file
    newIPAddress = stringToFind + "\"" + sb.toString() + "\"";

    //write IP Address to file

    File file = new File("SomeFilePath.txt");
    FileWriter fileWriter;

    try {

        fileWriter = new FileWriter(file);
        fileWriter.write(newIPAddress);

        fileWriter.flush();
        fileWriter.close();

    } catch (IOException e2) {

        e2.printStackTrace();

    }
}
\$\endgroup\$
4
\$\begingroup\$

For starters you can simplify the first part by utilizing the fact that indexOf can accept a second character indicating start point. This would lead to something like the following:

// Find indices for IP-address
int startIndex = resp.indexOf('\"', resp.indexOf("ip=")) + 1;
int endIndex = resp.indexOf('\"', startIndex);

String ipString = resp.substring(startIndex, endIndex);

Secondly, you can simplify the conversion by utilizing the substring method a little, and improve a little on performance by using StringBuilder, like in the following:

StringBuilder ipAddress = new StringBuilder();
int lenIpString = ipString.length();

//get two and two bytes from hex string and append to ipaddress 
if (lenIpString == 8) {
    long ipPart;
    for(int i = 0; i < lenIpString; i += 2)
    {
         ipPart = Long.parseLong(ipString.substring(i, i + 2), 16);
         ipAddress.append(String.format("%d.", ipPart));
    }

} else {
    throw new Exception("IP-string not long enough");
}

Note that this also does something if in the rare case the ipString is not 8 characters long. This also requires for the method to have an throws Exception added, and you should probably choose a better exception rather than Exception.

Finally your write to file can be enhanced to the following:

//write IP Address to file
File file = new File("SomeFilePath.txt");
FileWriter fileWriter;

try {
    fileWriter = new FileWriter(file);
    fileWriter.write(
        String.format("ip=\"%s\"",
                      ipAddress.substring(0, ipAddress.length() - 1)));

    fileWriter.flush();
    fileWriter.close();

} catch (IOException exception) {
    exception.printStackTrace();
}

Finally some general notes on coding style:

  • Consider splitting into three functions – These three code blocks could be extracted into (two or three) separate functions, where one serves as the output to the next. This would follow the single-responsibility principle of coding. One variant could change this function into:

    void WritePublicIPAddressToFile(String resp, String filename)
    {
       String ipAddress = findAndConvertIPAddress(resp);
    
       if (!ipAddress.isEmpty()) {
           writeIPAddressToFile(filename, ipAddress);
       }
    }
    

    Here I assume that the findAndConvertIPAddress() would return the empty string if the IP-address is not found as expected, and use that to bypass the file write if necessary. Now this function has a much clearer responsibility of finding and writing that IP-address to the file. No need to read lots of code to understand whats happening.

  • Use StringBuilder instead of String concatenation – In general try to avoid building new strings to a minimum, and use StringBuilder when building new strings, or a variation over String.format().

  • Consider having the filename as a parameter – Your code might be constructed specific for Code Review, but with a name like WritePublicIPAddressToFile I would expect that filename was one of the parameters.
  • Avoid using l as variable name – Especially in combination with i or 1, it can be really confusing to use lower-case L as a variable name. Use more descriptive names instead.
\$\endgroup\$

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.