Code Cleanup and API Development

[Acidshock]

What do others think about maybe cleaning up the code, making it more function and/or class based? I would be willing to throw in some time doing this but I am not sure if others would want this or be willing to hop on and help as well. Maybe we can get a bounty proposal from mflorell? Case of jolt?

[williamconley]

That's not a question, that's volunteerism. You don't have to 'ask' about that. Start converting code into "best practice" version for the language/area in which you are professional. Then begin submitting the revised code on the Issue Tracker. If it passes "muster", it'll get included and you can keep going.

If you're not sure ... look through the issue tracker for items posted by others and you'll see the methodology.

But remember: Anything that slows down the system is BAD.

[mflorell]

Keep in mind that the vicidial project has over 200,000 lines of code in it, which would take quite a while to rewrite. Also, a standards document should be decided upon before starting any conversion so efforts are not duplicated or wasted.

[Acidshock]

Ok I will see if I can come up with a standards document. It will be a first for me but I am willing to spend my time and entertain it.

[Acidshock]

How about this for a first run? I took a lot of it from Drupal's standards however changed some of the standards based on how I saw previous code done in Vici.










Indenting and Whitespace
Use an indent of 2 spaces, with no tabs.
Lines should have no trailing whitespace at the end.
All text files should end in a single newline (\n). This avoids the verbose "\ No newline at end of file" patch warning and makes patches easier to read since it's clearer what is being changed when lines are added to the end of a file.

Operators
All binary operators (operators that come between two values), such as +, -, =, !=, ==, >, etc. should have a space before and after the operator, for readability. For example, an assignment should be formatted as $foo = $bar; rather than $foo=$bar;. Unary operators (operators that operate on only one value), such as ++, should not have a space between the operator and the variable or number they are operating on.

Casting
Put a space between the (type) and the $variable in a cast: (int) $mynumber.

Control Structures
Control structures include if, for, while, switch, etc. Here is a sample if statement, since it is the most complicated of them:

if (condition1 || condition2)
{
if(condition3 == condition4)
{
action1;
}
}
elseif (condition3 && condition4)
{
action2;
}
else
{
defaultaction;
}

Control statements should have one space between the control keyword and opening parenthesis, to distinguish them from function calls.
You are strongly encouraged to always use curly braces even in situations where they are technically optional. Having them increases readability and decreases the likelihood of logic errors being introduced when new lines are added.

For switch statements:
switch (condition)
{
case 1:
action1;
break;

case 2:
action2;
break;

default:
defaultaction;
}
For do-while statements:
do
{
actions;
} while ($condition);


Function Calls

Functions should be called with no spaces between the function name, the opening parenthesis, and the first parameter; spaces between commas and each parameter, and no space between the last parameter, the closing parenthesis, and the semicolon. Here's an example:
$var = foo($bar, $baz, $quux);

As displayed above, there should be one space on either side of an equals sign used to assign the return value of a function to a variable. In the case of a block of related assignments, more space may be inserted to promote readability:
$short = foo($bar);
$long_variable = foo($baz);

Function Declarations
function funstuff_system($field)
{
$system["description"] = t("This module inserts funny text into posts randomly.");
return $system[$field];
}

Arguments with default values go at the end of the argument list. Always attempt to return a meaningful value from a function if one is appropriate.

Class Constructor Calls
When calling class constructors with no arguments, always include parentheses:
$foo = new MyClassName();

This is to maintain consistency with constructors that have arguments:
$foo = new MyClassName($arg1, $arg2);

Note that if the class name is a variable, the variable will be evaluated first to get the class name, and then the constructor will be called. Use the same syntax:
$bar = 'MyClassName';
$foo = new $bar();
$foo = new $bar($arg1, $arg2);


Arrays

Arrays should be formatted with a space separating each element (after the comma), and spaces around the => key association operator, if applicable:

$some_array = array('hello', 'world', 'foo' => 'bar');
Note that if the line declaring an array spans longer than 80 characters (often the case with form and menu declarations), each element should be broken into its own line, and indented one level:
$form['title'] = array(
'#type' => 'textfield',
'#title' => t('Title'),
'#size' => 60,
'#maxlength' => 128,
'#description' => t('The title of your node.'),
);

Note the comma at the end of the last array element; This is not a typo! It helps prevent parsing errors if another element is placed at the end of the list later.


Quotes

ViciDIAL does not have a hard standard for the use of single quotes vs. double quotes. Where possible, keep consistency within each module, and respect the personal style of other developers.
With that caveat in mind: single quote strings are known to be faster because the parser doesn't have to look for in-line variables. Their use is recommended except in two cases:
1. In-line variable usage, e.g. "<h2>$header</h2>".
2. Translated strings where one can avoid escaping single quotes by enclosing the string in double quotes. One such string would be "He's a good person." It would be 'He\'s a good person.' with single quotes. Such escaping may not be handled properly by .pot file generators for text translation, and it's also somewhat awkward to read.


String Concatenations

Always use a space between the dot and the concatenated parts to improve readability.
<?php
$string = 'Foo' . $bar;
$string = $bar . 'foo';
$string = bar() . 'foo';
$string = 'foo' . 'bar';
?>

When you concatenate simple variables, you can use double quotes and add the variable inside; otherwise, use single quotes. The use of the dot is preferable to enhance readability.

<?php
$string = "Foo $bar";
?>

When using the concatenating assignment operator ('.='), use a space on each side as with the assignment operator:
<?php
$string .= 'Foo';
$string .= $bar;
$string .= baz();
?>


Comments

Inline documentation for source files should follow the Doxygen formatting conventions.

Non-documentation comments are strongly encouraged. A general rule of thumb is that if you look at a section of code and think "Wow, I don't want to try and describe that", you need to comment it before you forget how it works.

Non-documentation comments should use capitalized sentences with punctuation. Sentences should be separated by single spaces. All caps are used in comments only when referencing constants, for example TRUE.

Comments should be on a separate line immediately before the code line or block they reference. For example:

// Unselect all other contact categories.
db_query('UPDATE {contact} SET selected = 0');

If each line of a list needs a separate comment, the comments may be given on the same line and may be formatted to a uniform indent for readability.

C style comments (/* */) and standard C++ comments (//) are both fine, though the former is discouraged within functions (even for multiple lines, repeat the // single-line comment). Use of Perl/shell style comments (#) is discouraged.


PHP Code Tags

Always use <?php ?> to delimit PHP code, not the shorthand, <? ?>.
Semicolons

The PHP language requires semicolons at the end of most lines, but allows them to be omitted at the end of code blocks. ViciDIAL coding standards require them, even at the end of code blocks. In particular, for one-line PHP blocks:
<?php print $tax; ?> -- YES
<?php print $tax ?> -- NO


Example URLs

Use "example.com" for all example URLs, per RFC 2606.


Naming Conventions


Functions and variables

Functions and variables should be named using lowercase, and words should be separated with an underscore. Functions should in addition have a grouping name as a prefix, to avoid name collisions between modules. Example:
admin_view_lead($lead_id)
agent_view_lead($lead_id)


Persistent Variables

Persistent variables should be named using all lowercase letters, and words should be separated with an underscore. They should use the grouping name as a prefix, to avoid name collisions between modules.


Constants

Constants should always be all-uppercase, with underscores to separate words. This includes pre-defined PHP constants like TRUE, FALSE, and NULL. Module-defined constant names should also be prefixed by an uppercase spelling of the module they are defined by.


Global Variables

If you need to define global variables, their name should start with a single underscore followed by the grouping name and another underscore.


Classes


Classes should be named using "CamelCase." For example:
<?php
abstract class DatabaseConnection extends PDO {
?>

Class methods and properties should use "lowerCamelCase":
<?php
public $lastStatement;
?>

The use of private class methods and properties should be avoided -- use protected instead, so that another class could extend your class and change the method if necessary. Protected (and public) methods and properties should not use an underscore prefix, as was common in PHP 4-era code.


File names

All documentation files should have the file name extension ".txt" to make viewing them on Windows systems easier. Also, the file names for such files should be all-caps (e.g. README.txt instead of readme.txt) while the extension itself is all-lowercase (i.e. txt instead of TXT).
Examples: README.txt, INSTALL.txt, TODO.txt, CHANGELOG.txt etc.



DoxyGen Standards:

Documenting classes and interfaces

Each class and interface should have a doxygen documentation block, and each member variable, constant, and function/method within the class or interface should also have its own documentation block. Example:
<?php
/**
* Represents a prepared statement.
*/
interface DatabaseStatementInterface extends Traversable {

/**
* Executes a prepared statement.
*
* @param array $args
* Array of values to substitute into the query.
* @param array $options
* Array of options for this query.
*
* @return
* TRUE on success, FALSE on failure.
*/
public function execute($args = array(), $options = array());
}

/**
* Represents a prepared statement.
*
* Default implementation of DatabaseStatementInterface.
*/
class DatabaseStatementBase extends PDOStatement implements DatabaseStatementInterface {

/**
* The database connection object for this statement DatabaseConnection.
*
* @var DatabaseConnection
*/
public $dbh;

/**
* Constructs a DatabaseStatementBase object.
*
* @param DatabaseConnection $dbh
* Database connection object.
*/
protected function __construct($dbh) {
// Function body here.
}

/**
* Implements DatabaseStatementInterface::execute().
*
* Optional explanation of the specifics of this implementation goes here.
*/
public function execute($args = array(), $options = array()) {
// Function body here.
}

/**
* Returns the foo information.
*
* @return object
* The foo information as an object.
*
* @throws MyFooUndefinedException
*/
public function getFoo() {
// Function body here.
}

/**
* Overrides PDOStatement::fetchAssoc().
*
* Optional explanation of the specifics of this override goes here.
*/
public function fetchAssoc() {
// Call PDOStatement::fetch to fetch the row.
return $this->fetch(PDO::FETCH_ASSOC);
}
}
?>
Notes:
• Leave a blank line between class declaration and first docblock.
• Use a 3rd person verb to begin the description of a class, interface, or method (e.g. Represents not Represent).
• For a member variable, use @var to tell what data type the variable is.
• Use @throws if your method can throw an exception, followed by the name of the exception class. If the exception class is not specific enough to explain why the exception will be thrown, you should probably define a new exception class.
• Make sure when documenting function and method return values, as well as member variable types, to use the most general class/interface possible. E.g., document the return value of db_select() to be a SelectQueryInterface, not a particular class that implements SelectQuery.



PHP Exceptions

Basic conventions
1. As Exceptions are classes, they should follow all coding standards for object-oriented code like any other class.
2. All Exceptions must end with the suffix "Exception".
3. All Exceptions must include an appropriate translated string as a message, unless the Exception is thrown very early in the bootstrap process before the translation system is available. That is extremely rare.
4. Exception classes should be named for the subsystem to which they relate, and the type of error. That is, [Subsystem][ErrorType]Exception.

The use of subclassed Exceptions is preferred over reusing a single generic exception class with different error messages as different classes may then be caught separately.

Example:
<?php
class WidgetNotFoundException extends Exception {}

function use_widget($widget_name) {
$widget = find_widget($widget_name);

if (!$widget) {
throw new WidgetNotFoundException(t('Widget %widget not found.', array('%widget' => $widget_name)));
}
}
?>


Try-catch blocks

try-catch blocks should follow a similar line-breaking pattern to if-else statements, with each catch statement beginning a new line.
Example:
<?php
try {
$widget = 'thingie';
$result = use_widget($widget);

// Continue processing the $result.
// If an exception is thrown by use_widget(), this code never gets called.
}
catch (WidgetNotFoundException $e) {
// Error handling specific to the absence of a widget.
}
catch (Exception $e) {
// Generic exception handling if something else gets thrown.
watchdog('widget', $e->getMessage(), WATCHDOG_ERROR);
}
?>



Inheritance

PHP requires that all exceptions inherit off of the Exception class, either directly or indirectly.

When creating a new exception class, it should be named according to the subystem they relate to and the error message they involve. If a given subsystem includes multiple exceptions, they should all extend from a common base exception. That allows for multiple catch blocks as necessary.
<?php
class FelineException extends Exception {}

class FelineHairBallException extends FelineException {}

class FelineKittenTooCuteException extends FelineException {}

try {
$nermal = new Kitten();
$nermal->playWith($string);
}
catch (FelineHairBallException $e) {
// Do error handling here.
}
catch (FelineKittenTooCuteException $e) {
// Do different error handling here.
}
catch (FelineException $e) {
// Do generic error handling here.
}
// Optionally also catch Exception so that all exceptions stop here instead of propagating up.
?>

[mflorell]

That was a lot to chew through.

Some of these are already being done, and some of these would be incredibly impractical to do(like renaming all variable names).

Overall, what I would be worried about most is doing enough testing after these changes were made to each file. right now things work pretty well, there are some minor bugs but ViciDial is really quite stable. If we go rewriting literally everything, some mistakes will be made and a lot of time will need to be spent testing every one of the thousands of features to make sure they all still work after each set of rewrites.

The next thing I would worry about is spending hundreds of hours rewriting everything with little or no performance gain while we could be adding new features that would benefit everyone with that amount of development time.

Also, what about perl and javascript? Some of these coding standards will not work in those languages at all.

[williamconley]

Alternately:

any NEW code could be written to the standards and anything rewritten could also use them. in a few months most of the system would have been reformatted.

since none of these items impact functionality, it would be more of a "best practices for submitting code". likely make it easier for everyone to read/submit if there were some form of basic rules.

and if Matt put them in (one at a time after being sure each one was viable!) on a wiki ... that standards wiki could continue to grow and all code warriors would at least have some reference for preferences. even if they don't follow it uniformly ... it's a start.

of course: a statement regarding languages to which these apply (or a page for each language) would be necessary.

[Acidshock]

Well one thing I think would be awesome to address is the way the variables of a session/lead are passed. It would be a lot better if they were a class and passed via serialization in php. That way you could address things like

$lead->phoneNumber = $phone_number;

The downside is that its not easy to make a request non-programatically by hand writing the url. It also is sort of a core function of vicidial right now.

I am thinking that maybe I can make a class and make a couple functions to translate between the old and new ways. Also it would be nice to automate some of the SQL operations into some class functions rather than writing it out manually.

[covarrubiasgg]

Maybe this could be done when the porting to asterisk 1.8 starts . As far as i know (from post in other threads) to do vicidial asterisk 1.8 capable, a lot of modules have to be rewritten.

I agree with Matt, if it is not broken, dont fix it .

But currenty the vici code is some kind of spagetti ñ_ñ, dont take me wrong it is a delicious spagetti indeed , but it is hard for people who is not involved with the developement of the project to work with the code like that, people like me i would be glad to help improve vicidial, but it take me hours to find out how something works in the system, even when i love coding.

As far as i know and correct me if i am wrong, there was a plan to port vicidial to asterisk 1.8 when it comes stable and intesive tested . If this is planned maybe it can be done using coding standards.

I have worked in dev projects before where i startup with a big spagetti of code, and try to fix it (or just organize it because it is not broken) is really painful, i loss a lot of features, new bugs appears and so on. But when for some reason i have to upgrade, instead of find the way to make my spagettu work, i just start over with a clean up code.

NOTE: This is just a opinion, i am not telling you in any way how to do your job, because i know i cant do it better job than the job you have done until now.

Cheers!

[mflorell]

If we had the resources(time and/or people) we would do a lot more code cleanup and documentation, but nobody wants to pay for this, so it doesn't get done.

As for volunteers doing the work, we are certainly open to that, but our experiences in the past show donated code is very often inadequately tested, meaning more work for us validating that nothing else breaks with the addition of new code.

[williamconley]

Thus the concept of getting a "standards" document for each of the programming languages "in place" to begin writing new code to "Standards" (with Matt's approval). If it were something available on the Wiki ... all new code conforming could be helpful for The Vicidial Group to interpret the code more easily and perhaps even decrease integration of foreign code into their systems.

After enough NEW code is written, eventually "cleaning up areas" (as each area is touched for whatever reason) could cause the code to be completely rewritten in a year or so.

Perhaps

[mflorell]

It's not the coding style that is the problem on donated code, it's the fact that it has not been tested enough usually. The contributor has focused on only the feature they want to add without testing for the effects on all of the other features they may not use.

Before we put up coding standards, someone should convert an existing file to the new format so we can confirm that the standards make sense, and so others have a reference file to work from.

Also, we need to have standards for all of the following languages:
- Perl
- PHP
- Javascript
- HTML

[Acidshock]

Ok I will pick up some of the php stuff. I am not very good at perl but the php I should be ok. I can handle the other stuff but its not a usual language I use so I might spend more time then usual.