looking good. if it's working, awesome. but if you're interested in some simplifications or improvements, i'll share them here, unsolicited, and you can do with that knowledge what you will. Hopefully it may help others as well.
This code looks a LOT like code i wrote when i was at your stage of coding. But i ran into some things along the way that perhaps you may be able to avoid without learning the hard way like i did.
You're on the right track by thinking "this code could be simplified". This is almost always true. However, don't get caught in the trap of "i need to make this code as simplified as possible". There is a threshold beyond which further simplifying code will actually make it less intuitive, harder to debug and harder for another coder to read. A good goal to strive for is that your code is highly readable (for me, this means that your functions and variables are named in a very clear and unambiguous way. for example, don't use a variable name like "io" to store the opacity of a given item.. use "itemOpacity" so your variables don't need to be decoded. this is probably a poor example, but you get the point.), while also minimizing inefficiencies.
The first part of that goal looks great in your code. your variable names and functions are clearly named. The second part is where we are going to look at some improvements.
The very first thing that stood out to me when i looked at your code here, is that your function declarations occur inside of for loops. Inside nested for loops at that. This means that every time you meet the conditions of running one of these functions, you are re-declaring the function. This is inherently inefficient since the function itself is not changing. When you write a loop, ask yourself "is there anything inside the body of this loop that never changes?". If the answer is yes, that portion should not be inside the loop. Your function declarations should exist in the top-level scope of the test() function. For our purposes, I will refer to this scope as "script-global" because this scope encompasses everything we need for this script.
So, now that we've identified that the function declarations remain static throughout the execution of the script, we can pull those out to script-global scope. This will help eliminate clutter, make your code easier to read, and improve efficiency.
Another efficiency improvement could be found by taking a step back and really analyzing your "j" for loop. In this loop, you're beginning by looping every pageItem in the document (special note.. be careful any time you do this because at the document scope, pathItems is an array of every single pathItem in that document, including pathItems that are nested inside groups or compound paths. It's generally a good idea to identify a more narrow scope to process as this will help prevent extremely long execution times on larger files). Then for each pathItem in the document, you are looping every layer in the document. and for every layer, you're performing a check on the same pathItem. So if you had 10 layers in your document, you'd be checking the same pathItem 10 times to see whether it is a spot color. This is inherently inefficient. It's generally a good idea to work top down when traversing the DOM. the "larger" elements should come first in your nesting order, otherwise you risk redundancy. In this case, since your second loop (the one with the variable "r") has no relationship to it's parent loop, it's gotta go. As a fun experiment, you can get a more comprehensive view of the inefficiency i'm talking about by adding a console log message to the beginning of each of these loops. Just add something simple like $.writeln("j = " + j);Then run the script again and watch the console. I think you'll find that even in a relatively small document, there's a lot more happening than you think.
the TLDR of the above paragraph is as follows.. You start by looping every path item. then you loop the layers in the document. then you loop the pageItems of each layer. Since you're looping the pageItems of the layers, you should remove either the "j" loop, or the "y" loop.
Next, let's look at your loop structure. Surely many people will have their own opinions about this, but my opinion is that if you find yourself nesting two for loops directly inside one another, it's probably a good opportunity to create a function instead. Otherwise your loop variables can get confusing. This is an issue of readability as well as an issue of ease of debugging. I couldn't tell you how much time i spent debugging some code because i used the wrong loop variable somewhere inside a nested for loop.
I feel like i'm rambling too much here, so I'll just post what i would consider to be the cleaner and more efficient code. Then you can ask questions if it doesn't seem to make sense.
function test()
{
var docRef = app.activeDocument;
var layers = docRef.layers;
var paths = docRef.pathItems;
//this doesn't change throughout this script,
//so let's declare it at the script-global scope.
var pathLen = paths.length;
var opacityRelationships = {
"100": 100,
"99": 70,
// rest of relationships go here e.g.
}
function updateOpacityGradient(item)
{
var stops = item.fillColor.gradient.gradientStops;
var curOpacity; //it's generally agreed that when possible, variables should be declared outside of loops
for (var x = 0, len = stops.length; x < len; x++)
{
curOpacity = Math.floor(stops.opacity);
stops.opacity = opacityRelationships[curOpacity];
}
}
function updateOpacitySpot(item)
{
var curOpacity = Math.floor(item.opacity).toString();
item.opacity = opacityRelationships[curOpacity];
}
//first let's process the gradient stops
for (var i = 0; i < pathLen; i++)
{
//in this first condition, you should always protect yourself against
//runtime errors by first checking that a given path is 'filled' before
//you try to access it's fillColor.
if (paths.filled && paths.fillColor == "[GradientColor]")
{
updateOpacityGradient(paths);
}
}
//now let's look at the opacity of non-gradient pathItems
for (var i = 0; i < pathLen; i++)
{
//again, make sure it's filled before you check the fillColor
if (paths.filled & paths.fillColor == "[SpotColor]")
{
updateOpacitySpot(paths);
}
}
}
test();
1 Correct answer
29
Replies
AdChoices