Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code review #38

Open
marquesarthur opened this issue Oct 18, 2018 · 0 comments
Open

Code review #38

marquesarthur opened this issue Oct 18, 2018 · 0 comments

Comments

@marquesarthur
Copy link
Collaborator

Disclaimer: this does not affect the team grading but it's something that I usually do both in 310 or 410 as this kind of feedback may bring new ideas to the table.

I took a quick look at the project and the parsing and evaluating follows the code presented in the lectures, but you could further refine the evaluating functions.

As some examples:

@Override
    public String evaluate() throws FileNotFoundException, UnsupportedEncodingException {
        String retVal = "cmds.poly" + object + "(n=\'"+name+"\')\n";
        for(Map.Entry<String, String> entry : propertyMap.entrySet()) {
            String attrName = entry.getKey();
            String attrValue = entry.getValue();

            if (!Vector.isVector(attrValue)) {
                // Check if value is in symbol table
                attrValue = Main.getValue(attrValue);
            }

            // Check if the value is a vector
            if (Vector.isVector(attrValue)){
                Vector aV = Vector.fromString(attrValue);
                if(attrName.equals("color")) {
                    retVal += "setColor(\'" + name + "\', " + aV.a/255.0+ ", " + aV.b/255.0 + ", " + aV.c/255.0 + ")\n";
                }
                else {
                    retVal += "cmds.setAttr(\'" + name + "." + attrName + "\', " + aV.a + ", " +
                            aV.b + ", " + aV.c + ", type=\"double3\")\n";
                }
            }
            else {
                try {
                    double attrDouble = Double.parseDouble(attrValue);
                    retVal += "cmds.setAttr(\'" + name + "." + attrName + "\', " + attrDouble + ")\n";
                }
                catch (NumberFormatException e) {
                    retVal += "cmds.setAttr(\'" + name + "." + attrName + "\'" + attrValue + ")\n";
                }
            }
        }
        System.out.println(retVal);
        return retVal;
    }

Could use a factory to distinguish the execution flow from the cases where isVector is true and the cases where it is not. Another interesting thing that could be used more often is String templating. Compilation wise, Java optimizes String concatenation and there is no difference IIRC. But string templating will help coders seeing the whole block and where the variables will fit. As an example:

"cmds.group("+objectList.toString().replace("[","").replace("]","")+", name='"+groupName+"')";

Could be turned to:

String param1 = objectList.toString().replace("[","").replace("]","")
String param2 = groupName;
"cmds.group(%s name='%s')".format(param1, param2);

It helps both during readability as well as as in debugging. It's easier to debug and locate errors that may happen during development.

public static STATEMENT getSubStatement()

Could also use a map adding the string of the if comparison as a key and the function as a value.
All these lines would be replaced by something like:

token = tokenizer.top()
if map_token_func.contain(token) {
    return map_token_func.get(token)
} else {
    System.out.println("Unknown command found: " + tokenizer.getNext());
}

Please remember that you all did a great job, take this comments lightly. I'll be glad to discuss more about refactoring and their trade-offs if you guys are interested.

Looking forward to seeing the video during the Film Fest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant