2
\$\begingroup\$

I'm extremely new to scripting (this is one of my first scripts so please don't laugh too hard at how inefficient or weird it is). I'm trying to have my script copy a .tiff image into a template file, change the text in the template file and then save as using the name off the .tiff files. At which point it moves on the the next open tiff image and does the same.

It works, but I'm wondering if all those ifs can be better written as a function or if there is some other better way to write it. But I'm not quite sure which direction to take.

var dlg=  
"dialog{text:'Script Interface',bounds:[100,100,500,220],"+   
"txt_static:StaticText{bounds:[15,20,390,79] ,text:'Dont forget to change the PN and the SN of the template file first!! Click ok to continue, cancel to stop the script.', properties:{multiline:true}},"+
"btn_ok:Button{bounds:[10,80,190,101] , text:'ok' },"+ 
"btn_closedlg:Button{bounds:[210,80,390,101] , text:'Cancel' }};"  

var win = new Window(dlg,'PSD Creator');  

win.center();  

win.btn_ok.onClick = function() {   

    win.close(1);

    var doc = app.activeDocument;
    var openDoc = app.documents;
    var docNum = openDoc.length;

    //select tiff, select template, paste change text
    for (var i = 1; i < docNum; i++){   

        var layerNum = doc.layers.length

        app.activeDocument = app.documents[i]; // Select that document.

        //Tiff document name and its path after the new document is selected
        var setName = app.activeDocument.name;
        var setLenth = setName.length;
        var viewNumber = setName.slice (setLenth-8,setLenth-4);
        var tiffPath = app.activeDocument.path;
        //also document path name in order to use it as a file name
        var psdName = setName.substring (0,setLenth-19);

        app.activeDocument.selection.selectAll(); //select all
        app.activeDocument.selection.copy(); //copy;

        app.activeDocument = app.documents[0]; //select template
        app.activeDocument.activeLayer = app.activeDocument.layers["Layer 0"]; //select layer [0]
        app.activeDocument.paste();//paste


        //////////////////////////////////////
        //Changing the text inside the view //
        //////////////////////////////////////

        var textLayer = app.activeDocument.layers["VIEW TEXT"]//select layerset "VIEW TEXT"
        app.activeDocument.layers.getByName("VIEW TEXT").visible = true;                
        var x = viewNumber.toString();

            if (x == "0001") {
                app.activeDocument.layers.getByName("VIEW TEXT").textItem.contents = ("GL");
            }
            if (x == "0002") {
                app.activeDocument.layers.getByName("VIEW TEXT").textItem.contents = ("GL");
            }
            if (x == "0003") {
                app.activeDocument.layers.getByName("VIEW TEXT").textItem.contents = ("Z2 LF");
            }                   
            if (x == "0004") {
                app.activeDocument.layers.getByName("VIEW TEXT").textItem.contents = ("Z1 LA");
            }           
            if (x == "0005") {
                app.activeDocument.layers.getByName("VIEW TEXT").textItem.contents = ("Z3 LA");
            }
            if (x == "0006") {
                app.activeDocument.layers.getByName("VIEW TEXT").textItem.contents = ("Z4 RLF");
            }
            if (x == "0007") {
                app.activeDocument.layers.getByName("VIEW TEXT").textItem.contents = ("AL");
            }
            if (x == "0008") {
                app.activeDocument.layers.getByName("VIEW TEXT").textItem.contents = ("Z3 LF");
            }
            if (x == "0009") {
                app.activeDocument.layers.getByName("VIEW TEXT").textItem.contents = ("Z2 LF");
            }
            if (x == "0010") {
                app.activeDocument.layers.getByName("VIEW TEXT").textItem.contents = ("Z2 LA");
            }
            if (x == "0011") {
                app.activeDocument.layers.getByName("VIEW TEXT").textItem.contents = ("Z3 RLF");
            }
            if (x == "0012") {
                app.activeDocument.layers.getByName("VIEW TEXT").textItem.contents = ("CA");
            }
            if (x == "0013") {
                app.activeDocument.layers.getByName("VIEW TEXT").textItem.contents = ("Z4 LF");
            }
            if (x == "0014") {
                app.activeDocument.layers.getByName("VIEW TEXT").textItem.contents = ("Z4 RLA");

            }

            ////////
            //END //
            ////////


        var viewName = app.activeDocument.layers.getByName("VIEW TEXT").textItem.contents;

        function saveTemplate (name) {

            var file = new File(tiffPath + "/" + name + '.psd');
            var saveOptions = new PhotoshopSaveOptions();

            saveOptions.embedColorProfile = true;
            saveOptions.layers = true;

            doc.saveAs(file, saveOptions, true);
        }       

        saveTemplate(psdName + " " + viewName.toLowerCase());

        app.activeDocument.activeLayer.remove();

    }



}

win.show();
\$\endgroup\$
2
\$\begingroup\$

All the if statements can be rephrased by using a dictionary. Where the key is the x value in your code.

views = {"0001": "Gl", ...etc}
app.activeDocument.layers.getByName("VIEW TEXT").textItem.contents = views[viewNumber.toString()];
\$\endgroup\$
  • \$\begingroup\$ Doe this answer also need the looping through the key value pairs? \$\endgroup\$ – geeves Oct 4 '17 at 15:16
  • \$\begingroup\$ @geeves No. You just use views[viewNumber] to get the correct value. \$\endgroup\$ – TimSparrow Oct 4 '17 at 15:48
1
\$\begingroup\$

Some general considerations:

  1. replace the long if (x== ) part with a switch statement, it would be more compact and readable. Placing it in a separate function is even better. Define the value of contents in the switch, then update it in one statement. Like this:

    switch(x) {
    
      case '0004':
        content = 'Z1 LA';
      break;
      // more cases skipped
    }
    app.activeDocument.layers.getByName("VIEW TEXT").textItem.contents = content;
    
  2. Avoid using 'magic' numbers in the code

    var psdName = setName.substring (0,setLenth-19);
    

This is bad, as hardcoded numbers are difficult to understand. Use named constants or calculate these numbers, so that it is easy to understand why are you using this value.

upd:use a json array of viewNumber:content pairs suggested in another answer is a great idea.

\$\endgroup\$
  • \$\begingroup\$ Can you clarify 'magic' numbers? Do you mean adding numbers that only seem to mean something to the person writing the code? The images that I need incorporated into my template are automatically generated with a suffix ".RGB_color.00xx", so it's this string that needs to be removed. So, my guess would be that I should look into a way to use the string itself that needs to be removed instead of setLenth-19? \$\endgroup\$ – Rebecca Taylor Oct 4 '17 at 14:56
  • \$\begingroup\$ @RebeccaTaylor exactly. To a code reviewer, the value of 19 means nothing, and one has to analyze the code deeper to understand its meaning. Also, such values tend to change with time (eg. label text changes, thus its length). \$\endgroup\$ – TimSparrow Oct 4 '17 at 14:59
  • \$\begingroup\$ @RebeccaTaylor in your case, I would suggest to declare the suffix as a variable, and use its length in the code. \$\endgroup\$ – TimSparrow Oct 4 '17 at 15:03

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.