Hello,
Thank you and I look forward for more recommendations and comments. I
will upload the files once I incorporate all the suggestions and comments.
Sonam
On 2/9/2012 4:15 AM, Christian Boltz wrote:
> Hello,
>
> Am Mittwoch, 8. Februar 2012 schrieb Sonam Penjor:
>> I am attaching the complete files that I have been working on the bulk
>> email creation for the review.
> Thanks a lot!
>
>> Should you have any inquiries on the
>> attached files, I would be happy to explain.
>>
>> I will appreciate for the comments, advices and recommendations.
> You will regret this offer ;-))
>
>
> Some questions first:
>
> - Do you have some example CSV files I could use for testing?
> (That would be easier than creating them myself ;-)
> Bonus points if you also have some CSV files with invalid data
> (duplicate usernames, invalid mail addresses, whatever)
> - what's the biggest (in number of lines) CSV file you tested?
> - some documentation/description how the CSV file should look like
> (allowed columns etc.) would be useful. I'd even place it next to the
> form in the template file.
> - did I get it right that your only modification to en.lang is the
> addition of 14 texts after this comment:
> /* Newly added for the uploading Bulk email ID */
> (just to avoid I overlooked something, it seems en.lang got some
> changes since you copied it)
> I'm attaching an updated en.lang - if you have to do more changes,
> add more texts etc. please use the attached en.lang as base.
>
> Now to some notes on your code:
> - please don't play around with error_reporting - there's a good reason
> I have a very strict setting for it on my development machine ;-)
> I know notices can be annoying sometimes, but often they help to find
> bugs like typos in a variable name which would be hard to find
> otherwise
> - your class needs to be named model/UploadCsvFile.php - otherwise the
> autoloader won't find it
> - better error handling would be nice. For example, if I upload an
> invalid csv file, I get (only) an error message "Invalid Delimiter".
> It would be better to display the error message using flash_error()
> and to display the form again
> - the "Invalid Delimiter" error message seems to be misleading - I
> uploaded a file with the correct delimiter but wrong column headers
> (first line: "a,b,c,d") and it complained about "Invalid Delimiter".
> From reading the code, I'd say you should print out the error message
> you get from scanCsvfile() instead of just checking for failure.
> - I have attached a patch for create-bulkmailbox.php and
> model/UploadCsvFile.php with some small changes and inline notes -
> that's easier than including them in the mail. I also attached the
> modified files, but reading the patches is probably easier.
> - in the class, please explicitely mark the functions as private or
> public
>
> So much for the first review ;-)
> The above is what I noticed when reading the code. I didn't test much
> (except with an invalid CSV file ;-) yet, but the above and the inline
> comments will already give you several ideas what you can improve ;-)
>
> And finally: I hope you aren't shocked too much about all the things I
> found in your code ;-))
>
>
> Regards,
>
> Christian Boltz
>
>
> ------------------------------------------------------------------------------
> Keep Your Developer Skills Current with LearnDevNow!
> The most comprehensive online learning library for Microsoft developers
> is just $99.99! Visual Studio, SharePoint, SQL - plus HTML5, CSS3, MVC3,
> Metro Style Apps, more. Free future releases when you subscribe now!
> http://p.sf.net/sfu/learndevnow-d2d
>
>
> _______________________________________________
> Postfixadmin-devel mailing list
> Pos...@li...
> https://lists.sourceforge.net/lists/listinfo/postfixadmin-devel
--
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.
|
