Get the environment variable's value

I've written a function which can get the environment variable's value. The input parameter would one of the following:

• ${machine}/bin

• ${machine/bin}

• ${machine}/bin${path${OS}}/path

I would need to check the values which start with ${}

I've written a function which is recursive in nature, and I would appreciate it if anyone can review the code for me and tell me what can be done better:

private String replaceWithEnvironmentVariables(String specialChars) {
    String result="";
    boolean flag=true;
    try{
        result = specialChars;
        String temp = "";
        char loopBreaker = '}';
        char dollarCheck = ';
        char slashCheck = '/';
        int index = result.indexOf("{");
        if (index == -1){
            result = result.replaceAll("[{},$]", "");
            return result;
        }
        for (int i = index; i < result.length(); i++) {
            if(slashCheck==result.charAt(i))
                break;
            temp += result.charAt(i);
            if(dollarCheck==result.charAt(i) || loopBreaker==result.charAt(i)){
                break;
            }
        }
        String sub = temp.replaceAll("[{},$/]", "");
        if(sub==null || sub.length()==0){
            sub="";
            flag=false;
            result = result.replace(temp, sub);
        }
        if(flag){
            String returnSub = getEnvironmentVariableValue(sub);
            result = result.replace(temp, returnSub);
        }
    }catch(NullPointerException nullPointer){
        nullPointer.printStackTrace();
    }catch(Exception except){
        except.printStackTrace();
    }
    return replaceWithEnvironmentVariables(result);
}

private String replaceWithEnvironmentVariables(String specialChars) {

Naming. specialChars are no special chars, it's String in which variables get substituted. Maybe pattern or alike.

String result="";

Spacing. Needless assignment as it gets overwritten 3 lines below. Should be

String result = specialChars;

boolean flag=true;

Naming... flag means actually nothing.

String temp = "";

And neither does temp. A single meaningless name is sometimes tolerable (especially in short methods), but with more, it's getting harder and harder to understand.

char dollarCheck = '

Dollar cheque? Check? ✔? It should be a constant and called e.g.

private static static DOLLAR_CHAR = '

or better "variable start char" or alike. Prefer to express what it means to what it is, as the latter is obvious.

if (index == -1){
        result = result.replaceAll("[{},$]", "");
        return result;
    }


I'd personally throw an exception for malformed inputs like foo}bar.

if(slashCheck==result.charAt(i))


That's Yoda style, the form var == expr is more common.

Now I've got lost... you parse the string somehow... OK, let's ignore it.

String sub = temp.replaceAll("[{},$/]", "");
if(sub==null || sub.length()==0){


Know your libraries. sub can't be null. Instead of sub.length()==0, you can use sub.isEmpty().

sub="";


This line is useless now.

... catch ...


Just leave it out. Let the user deal with the problem. You may know, that e.g., using an empty String is acceptable in case of an error, but you surely don't know it here. Just document the exception and let the poor user deal with it. Here, "user" means you in another piece of the program.

NullPointerException


I see a single place where it can be thrown:

String returnSub = getEnvironmentVariableValue(sub);
result = result.replace(temp, returnSub);


Don't let this happen. Maybe throw a new MissingEnvironmentVariableException(sub), maybe allow the caller to specify a replacement, e.g., like this

private String replaceWithEnvironmentVariables(
        String pattern, 
        Map specificReplacements,
        String defaultReplacement) {...}


where specificReplacements contains the default values for missing variables and defaultReplacement gets used as a last resort. Allow defaultReplacement to be null, but then throw an exception if it should be needed.;


Dollar cheque? Check? ✔? It should be a constant and called e.g.

%%CODEBLOCK_5%%

or better "variable start char" or alike. Prefer to express what it means to what it is, as the latter is obvious.

%%CODEBLOCK_6%%

I'd personally throw an exception for malformed inputs like foo}bar.

%%CODEBLOCK_7%%

That's Yoda style, the form var == expr is more common.

Now I've got lost... you parse the string somehow... OK, let's ignore it.

%%CODEBLOCK_8%%

Know your libraries. sub can't be null. Instead of sub.length()==0, you can use sub.isEmpty().

%%CODEBLOCK_9%%

This line is useless now.

%%CODEBLOCK_10%%

Just leave it out. Let the user deal with the problem. You may know, that e.g., using an empty String is acceptable in case of an error, but you surely don't know it here. Just document the exception and let the poor user deal with it. Here, "user" means you in another piece of the program.

%%CODEBLOCK_11%%

I see a single place where it can be thrown:

%%CODEBLOCK_12%%

Don't let this happen. Maybe throw a new MissingEnvironmentVariableException(sub), maybe allow the caller to specify a replacement, e.g., like this

%%CODEBLOCK_13%%

where specificReplacements contains the default values for missing variables and defaultReplacement gets used as a last resort. Allow defaultReplacement to be null, but then throw an exception if it should be needed.;

or better "variable start char" or alike. Prefer to express what it means to what it is, as the latter is obvious.

%%CODEBLOCK_6%%

I'd personally throw an exception for malformed inputs like foo}bar.

%%CODEBLOCK_7%%

That's Yoda style, the form var == expr is more common.

Now I've got lost... you parse the string somehow... OK, let's ignore it.

%%CODEBLOCK_8%%

Know your libraries. sub can't be null. Instead of sub.length()==0, you can use sub.isEmpty().

%%CODEBLOCK_9%%

This line is useless now.

%%CODEBLOCK_10%%

Just leave it out. Let the user deal with the problem. You may know, that e.g., using an empty String is acceptable in case of an error, but you surely don't know it here. Just document the exception and let the poor user deal with it. Here, "user" means you in another piece of the program.

%%CODEBLOCK_11%%

I see a single place where it can be thrown:

%%CODEBLOCK_12%%

Don't let this happen. Maybe throw a new MissingEnvironmentVariableException(sub), maybe allow the caller to specify a replacement, e.g., like this

%%CODEBLOCK_13%%

where specificReplacements contains the default values for missing variables and defaultReplacement gets used as a last resort. Allow defaultReplacement to be null, but then throw an exception if it should be needed.;

Dollar cheque? Check? ✔? It should be a constant and called e.g.

%%CODEBLOCK_5%%

or better "variable start char" or alike. Prefer to express what it means to what it is, as the latter is obvious.

%%CODEBLOCK_6%%

I'd personally throw an exception for malformed inputs like foo}bar.

%%CODEBLOCK_7%%

That's Yoda style, the form var == expr is more common.

Now I've got lost... you parse the string somehow... OK, let's ignore it.

%%CODEBLOCK_8%%

Know your libraries. sub can't be null. Instead of sub.length()==0, you can use sub.isEmpty().

%%CODEBLOCK_9%%

This line is useless now.

%%CODEBLOCK_10%%

Just leave it out. Let the user deal with the problem. You may know, that e.g., using an empty String is acceptable in case of an error, but you surely don't know it here. Just document the exception and let the poor user deal with it. Here, "user" means you in another piece of the program.

%%CODEBLOCK_11%%

I see a single place where it can be thrown:

%%CODEBLOCK_12%%

Don't let this happen. Maybe throw a new MissingEnvironmentVariableException(sub), maybe allow the caller to specify a replacement, e.g., like this

%%CODEBLOCK_13%%

where specificReplacements contains the default values for missing variables and defaultReplacement gets used as a last resort. Allow defaultReplacement to be null, but then throw an exception if it should be needed.

StackExchange Code Review Q#67420, answer score: 5