Code for review svn commit: 7499 and svn commit: 7500

[Laurel A. Williams]

Agg - I clicked email before I was finished. Please ignore my previous 
attempt to reply.

Let me try again.

> Great help Colin,
>
> Here are some thoughts inline below...
>
> Laurel
>
> Colin Clark wrote:
>> Hi Laurel,
>>
>> I've take a look at the commits you mentioned below. I'd like Jacob 
>> to take a look at them when he gets a chance, too.
>>
>> It's really nice to see the Builder server-side coding coming along 
>> well. A couple of quick comments and questions...
>>
>> I can't quite tell why you've structured your BuilderUtilityClass as 
>> a full-fledged class with instance methods. There seems to be no 
>> state here, just a couple of standalone utility functions. Indeed the 
>> naming suggests as much--it's not an object with any real identity, 
>> just a collection of functions. Maybe it's easier to ditch the class 
>> and just invoke these functions directly? Or perhaps there's a bit 
>> more thinking about how to structure the various bits of logic in 
>> your code?

I appreciate the point about the BuilderUtilityClass - It was a first 
pass at getting the unit tests to work - all the SimpleTest examples 
used a Class rather than a collection of functions, so I modelled the 
examples without thinking. I will revise that in my next iteration.

>>
>> You've got the list of available modules hardcoded at the moment, but 
>> I notice that you've got a comment in the code saying that this will 
>> change later. How will you determine these module values? From 
>> parsing Infusion's Ant build.properties file?

I had intended to determine the list of modules from the ant 
build.properties file as you suggest. Since I needed to revise the 
jsonProcessor (which also parses the ant build file) I thought I would 
tackle those tasks concurrently.

>>
>> I notice that you always return true from your processPostVariables() 
>> method, but then this value is checked in case any errors occurred. 
>> Looks like a bug to me, or perhaps something you haven't had a chance 
>> to implement yet.
>>
>> $successPost = processPostVariables();
>> if (!$successPost)
>> {
>>     returnError("Cannot process input variables");
>>     exit (1);
>> }
If you look carefully at processPostVariables line 37, you will note 
that if the post variable does not exist, the function returns false. 
However you are correct that I didn't check the other post variables and 
return false if they had a problem. Thx for catching that.
>>
>> In postProcessor.php, I notice that you define $minified and 
>> $includes as global variables. How come? If it's necessary, is there 
>> a way you can define a class instead that will store this state and 
>> the methods that depend on it without requiring globals?
>>
I also agree with your comment about the global variables and will work 
them into some other data structure. It was necessary to declare them 
global to access them in the way I did, but of course, that isn't the 
only way to do it, and certainly not the best way. I think a class 
defining the output results, $minified, $includes and $excludes would be 
much better.
>> Hope this helps,
>>
>> Colin
>

-- 
Laurel A. Williams
Adaptive Technology Resource Centre
University of Toronto