Heavily-criticized tax calculator

Question

I just created this during a live screencast. The people who were on there were very critical of almost every line of code. They say I am a horrible coder and know nothing. Looking at this tax calculator, would you say I'm an OK coder?

Demo

I don't think they're right, but I require one or more objective subject-matter experts to confirm whether or not the code is horrible.

<?php

// Set up autoloading
set_include_path(get_include_path() . PATH_SEPARATOR .
 realpath('../'));

// This uses my Thrive framework, which extends
// the Flourish framework.
// NOTE: Using require, or require_once has performance penalties that aren't
//       necessary.  If the file isn't loaded successfully, the app will crash
//       anyway.
include_once 'thrive/Autoloader.php';
new Thrive_Autoloader;

// Iteration 2: Create the Data Models.

// TIP: By always building out class datatypes instead of relying on hashmaps (array keys0, you
//      make it much easier to create what's called an API that people can easily use later.
class TaxBracket
{
    public $min;
    public $max;
    public $rate;
}

// Let's create an interface for our new taxing system.
interface API_Tax
{
    public function getTaxLiability(fMoney $revenue, fMoney $deductions);
}

// It makes more sense to use a Factory for this.
// Uses the Factory Design Pattern.
class USFederalTaxesFactory
{
    public static function create(fMoney $revenue, fMoney $deductions, $numOfEmployees)
    {
        // TODO: Rearchitect out the magic Constants.
        // TODO: Add a proper Service Locator Pattern here...
        $taxes = array(
            'ssi' => 'SocialSecurityTax',
            'medicare' => 'MedicareTax',
            //'unemployment' => 'UnemploymentTax',
        );
        foreach ($taxes as $taxName => $taxClass)
        {
            $tax = new $taxClass();
            /** @var $tax API_Tax */
            $liabilities[$taxName] = $tax->getTaxLiability($revenue, $deductions);
        }

        $tax = new UnemploymentTax($numOfEmployees);
        $liabilities['unemployment'] = $tax->getTaxLiability($revenue, $deductions);

        $incomeTax = new USFederalIncomeTax($revenue, $deductions, $liabilities);

        return $incomeTax;
    }
}

// FIXME: Figure out a way to save the API_Tax interface ;-/
class USFederalIncomeTax /* implements API_Tax*/
{
    // AVOID MAGIC CONSTANTS.
    // FIXME: These need to be moved to their own classes. It's a serious weakness right now!!!!
    const INCOME_TAX_KEY = 'income';
    const SSI_TAX_KEY = 'ssi';
    const MEDCARE_TAX_KEY = 'medicare';
    const UNEMPLOYMENT_TAX_KEY = 'unemployment';

    /** @var TaxBracketManager */
    protected $bracketManager;

    /** @var TaxBracket[] */
    protected $brackets;

    /** @var fMoney */
    protected $revenue;

    /** @var fMoney */
    protected $deductions;

    /** @var fMoney[] */
    protected $taxLiabilities;

    public function __construct(fMoney $revenue, fMoney $deductions, array $otherTaxLiabilities, $bracketManager = null)
    {
        $this->amountOwed = new fMoney('0', 'USD');

        $this->revenue = $revenue;
        $this->deductions = $deductions;
        $this->taxLiabilities = $otherTaxLiabilities;

        if ($bracketManager === null)
        {
            $bracketManager = new TaxBracketManager();
        }
        $this->bracketManager = $bracketManager;
    }

    /**
     * @param $tax
     * @return fMoney
     * @throws LogicException*/
    public function getLiabilityByTax($tax)
    {
        if (!isset($this->taxLiabilities[$tax]))
        {
            throw new LogicException("No tax liabilities named '$tax'");
        }

        return $this->taxLiabilities[$tax];
    }

    public function getTaxLiability()
    {
        $brackets = $this->fetchTaxBracketRates();

        // Federal income tax algorithm works like this:
        // Total Revenue - Qualified Deductions - Other Fed Taxes -> Tax Brackets -> Rate.
        // Minimum possible taxes owed: $0.

        $otherTaxes = new fMoney(0, 'USD');
        // TODO: It'd be a nice-to-have to be able to dynamically figure out which
        // taxes are non-income and just do a foreach() here...
        $otherTaxes = $otherTaxes->add($this->getLiabilityByTax(self::SSI_TAX_KEY));
        $otherTaxes = $otherTaxes->add($this->getLiabilityByTax(self::MEDCARE_TAX_KEY));
        $otherTaxes = $otherTaxes->add($this->getLiabilityByTax(self::UNEMPLOYMENT_TAX_KEY));

        $amountOwed = $this->calculateTaxLiability($this->revenue, $this->deductions, $otherTaxes);
        $this->taxLiabilities[self::INCOME_TAX_KEY] = $amountOwed;
        return $amountOwed;
    }

    protected function calculateTaxLiability(fMoney $taxableRevenue, fMoney $deductions, fMoney $otherTaxes)
    {
        $taxableRevenue = $taxableRevenue->sub($deductions);
        $taxableRevenue = $taxableRevenue->sub($otherTaxes);

        $totalTaxLiability = new fMoney(0, 'USD');
        foreach ($this->brackets as /** @var TaxBracket */ $bracket)
        {
            // Assume 500 0000
            // Algorithm: Get subvalue -> get liability -> add liability -> subtract subvalue -> continue
            if ($bracket->max !== null)
            {
                $amountTaxedInBracket = $bracket->max - $bracket->min;
            }
            else
            {
                $amountTaxedInBracket = $taxableRevenue;
            }

            if ($taxableRevenue->lte($amountTaxedInBracket))
            {
                $amountTaxedInBracket = $taxableRevenue;
            }

            $taxLiability = new fMoney($amountTaxedInBracket, 'USD');
            $taxLiability = $taxLiability->mul($bracket->rate);

            $totalTaxLiability = $totalTaxLiability->add($taxLiability);

            $taxableRevenue = $taxableRevenue->sub($amountTaxedInBracket);

            if ($taxableRevenue->lte(0))
            {
                break;
            }
        }

        if (isset($_GET['debug']))
        {
            echo "</pre>";
        }

        return $totalTaxLiability;
    }

    protected function fetchTaxBracketRates()
    {
        if ($this->brackets !== null)
        {
            return $this->brackets;
        }

        // TODO: Load this from a database table.
        // FIXME: Add more brackets.
        // TIP: By doing a little extra work now, all i have to do later is do $pdo->fetchArray() later ;-)
        // I **LOVE** our complicated corporate income tax system!! WOOO!!!!!

        $brackets = $this->bracketManager->fetchAll();
        $this->brackets = $brackets;

        return $brackets;
    }
}

class TaxBracketManager
{
    public function fetchTaxBrackets()
    {
        $bracketsInfo = array(
            array('min' => 0,        'max' => 50000,    'rate' => 0.15),
            array('min' => 50001,    'max' => 75000,    'rate' => 0.25),
            array('min' => 75001,    'max' => 100000,   'rate' => 0.34),
            array('min' => 100001,   'max' => 335000,   'rate' => 0.39),
            array('min' => 335001,   'max' => 10000000, 'rate' => 0.34),
            array('min' => 10000001, 'max' => 15000000, 'rate' => 0.35),
            array('min' => 15000001, 'max' => 18333333, 'rate' => 0.38),
            array('min' => 18333334, 'max' => null,     'rate' => 0.35),
        );

        $brackets = array();
        foreach ($bracketsInfo as $i)
        {
            $bracket = new TaxBracket;
            $bracket->min = $i['min'];
            $bracket->max = $i['max'];
            $bracket->rate = $i['rate'];

            $brackets[] = $bracket;
        }

        return $brackets;
    }
}

// Social security tax does not allow for deductions. It is off of the entire gross wage.
class SocialSecurityTax implements API_Tax
{
    public function getTaxLiability(fMoney $revenue, fMoney $deductions)
    {
        $taxLiability = $this->calculateTaxLiability($revenue);

        return $taxLiability;
    }

    protected function calculateTaxLiability(fMoney $taxableRevenue)
    {
        $maxTaxable = $this->fetchMaxTaxableAmount();

        $taxableRevenue = min($maxTaxable, $taxableRevenue);
        $taxRate = $this->fetchTaxRate();
        $taxLiability = $taxableRevenue->mul($taxRate);

        if (isset($_GET['debug']))
        {
            echo "<pre>";
            echo "[ssi] Taxable Revenue: $taxableRevenue\n";
            echo "[ssi] Tax Rate: $taxRate\n";
            echo "[ssi] Total Tax Liability: $taxLiability\n";
            echo "</pre>";
        }

        return $taxLiability;
    }

    protected function fetchMaxTaxableAmount()
    {
        return new fMoney(110100, 'USD');
    }

    protected function fetchTaxRate()
    {
        return 0.062;
    }
}

// Medicare tax does not allow for deductions. It is off of the entire gross wage.
class MedicareTax implements API_Tax
{
    public function getTaxLiability(fMoney $revenue, fMoney $deductions)
    {
        $taxLiability = $this->calculateTaxLiability($revenue);

        return $taxLiability;
    }

    protected function calculateTaxLiability(fMoney $taxableRevenue)
    {
        $taxRate = $this->fetchTaxRate();
        $taxLiability = $taxableRevenue->mul($taxRate);

        if (isset($_GET['debug']))
        {
            echo "<pre>";
            echo "[medicare] Taxable Revenue: $taxableRevenue\n";
            echo "[medicare] Tax Rate: $taxRate\n";
            echo "[medicare] Total Tax Liability: $taxLiability\n";
            echo "</pre>";
        }

        return $taxLiability;
    }

    protected function fetchTaxRate()
    {
        return 0.0145;
    }
}

// Unemployment Insurance tax does not allow for deductions. It is off of the entire gross wage.
// Source: http://workforcesecurity.doleta.gov/unemploy/uitaxtopic.asp
class UnemploymentTax implements API_Tax
{
    protected $numOfEmployees;

    public function __construct($numOfEmployees = null)
    {
        if ($numOfEmployees !== null)
        {
            echo "Num of Employees 2.5: $numOfEmployees";
            $this->setNumberOfEmployees($numOfEmployees);
        }
    }

    public function setNumberOfEmployees($numOfEmployees)
    {
        echo "Num of Employees 3: " . $numOfEmployees;
        $this->numOfEmployees = $numOfEmployees;
    }

    public function getTaxLiability(fMoney $revenue, fMoney $deductions)
    {
        if (empty($this->numOfEmployees))
        {
            throw new LogicException("Cannot calculate Unemployment tax without specifying the number of employees.");
        }

        $taxLiability = new fMoney(0, 'USD');
        for ($a = 0; $a < $this->numOfEmployees; ++$a)
        {
            $taxLiability = $taxLiability->add($this->calculateTaxLiability($revenue));
        }

        return $taxLiability;
    }

    protected function calculateTaxLiability(fMoney $taxableRevenue)
    {
        if ($taxableRevenue->lte($this->fetchMinimumAmountToTax()))
        {
            return new fMoney(0, 'USD');
        }

        $maxTaxLiability = $this->fetchMaxTaxLiability();
        $taxRate = $this->fetchTaxRate();

        $taxLiability = $taxableRevenue->mul($taxRate);

        if ($taxLiability->gte($maxTaxLiability))
        {
            $taxLiability = $maxTaxLiability;
        }

        if (isset($_GET['debug']))
        {
            echo "<pre>";
            echo "[unemployment] Taxable Revenue: $taxableRevenue\n";
            echo "[unemployment] Tax Rate: $taxRate\n";
            echo "[unemployment] Total Tax Liability: $taxLiability\n";
            echo "</pre>";
        }

        return $taxLiability;
    }

    protected function fetchMinimumAmountToTax()
    {
        return new fMoney(1500, 'USD');
    }

    protected function fetchMaxTaxLiability()
    {
        return new fMoney(56.00, 'USD');
    }

    protected function fetchTaxRate()
    {
        return 0.062;
    }
}


class FedTaxes
{
    /** @var fMoney */
    public $income;
    /** @var fMoney */
    public $ssi;
    /** @var fMoney */
    public $medicare;
    /** @var fMoney */
    public $unemployment;
    /** @var fMoney */
    public $total;

    public function __construct()
    {
        $this->income = new fMoney(0, 'USD');
        $this->ssi = new fMoney(0, 'USD');
        $this->medicare = new fMoney(0, 'USD');
        $this->unemployment = new fMoney(0, 'USD');
        $this->total = new fMoney(0, 'USD');
    }
}

// Main execution path.
function getTaxLiability()
{
    if (!empty($_POST['gross_income']))
    {
        // I prefer filter_var($_POST) over filter_input(INPUT_POST) because it
        // is **impossible** to simulate form data in unit tests with filter_input().
        if (($grossIncome = filter_var($_POST['gross_income'], FILTER_SANITIZE_NUMBER_FLOAT)) === false)
        {
            throw new InvalidArgumentException("Invalid input for gross income, only numbers are accepted.");
        }
        if (($expenses = filter_var($_POST['expenses'], FILTER_SANITIZE_NUMBER_FLOAT)) === false)
        {
            throw new InvalidArgumentException("Invalid input for expenses, only numbers are accepted.");
        }
        if (($numOfEmployees = filter_var($_POST['num_of_employees'], FILTER_SANITIZE_NUMBER_INT)) === false)
        {
            throw new InvalidArgumentException("Invalid input for number of employees, only integers are accepted.");
        }
        echo "Num of Employees: $numOfEmployees";

        fMoney::setDefaultCurrency('USD');
        $taxManager = USFederalTaxesFactory::create(new fMoney($grossIncome), new fMoney($expenses), $numOfEmployees);
        $amountOwed = $taxManager->getTaxLiability();

        $fedTaxes = new FedTaxes;
        $fedTaxes->income = $amountOwed;
        $fedTaxes->ssi = $taxManager->getLiabilityByTax('ssi');
        $fedTaxes->medicare = $taxManager->getLiabilityByTax('medicare');
        $fedTaxes->unemployment = $taxManager->getLiabilityByTax('unemployment');

        // TODO: fMoney **really** should be able to add multiple values at once. I mean, come on!
        foreach (array('income', 'ssi', 'medicare', 'unemployment') as $tax)
        {
            $fedTaxes->total = $fedTaxes->total->add($fedTaxes->$tax);
        }

        return $fedTaxes;
    }

    return null;
}

$fedTaxes = null;
try
{
    $fedTaxes = getTaxLiability();
}
catch(InvalidArgumentException $e)
{
    $errorMessage = "Oops! An error has occurred:<br/>\n";
    $errorMessage .= $e->getMessage();
}
catch(Exception $e)
{
    $errorMessage = "Oops: An error has occured.";
    $errorMessage .= $e->getMessage();

    error_log($e->getMessage());
}

?>
<?php
// Start of the view.
$e_grossIncome = isset($_POST['gross_income']) ? htmlspecialchars($_POST['gross_income']) : '';
$e_expenses = isset($_POST['expenses']) ? htmlspecialchars($_POST['expenses']) : '';
$e_numOfEmployees = isset($_POST['num_of_employees']) ? htmlspecialchars($_POST['num_of_employees']) : '';

if (isset($errorMessage))
{
    $e_errorMessage = htmlspecialchars($errorMessage);
}
// Iteration 1: Create the HTML.
?>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN"
"http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
    <head>
        <title>Federal Corporate Income Tax Calculator</title>
<style type="text/css">
table#tax_data_table th { text-align: left; }
table#tax_data_table td { text-align: right; }
</style>
    </head>
    <body>
        <h1>Federal Corporate Income Tax Calculator</h1>
        <p>This app is designed to calculate the estimated income tax of a corporation.</p>
        <h4>Disclaimer</h4>
        <p>This app is for educational purposes only. It is by no means a substitute for proper tax accounting services!</p>
        <p><em>Note: Because of <a href="http://workforcesecurity.doleta.gov/unemploy/uitaxtopic.asp">Congressional bungling</a>, the Unemployment Taxrate
            for 2011 is overstated (usually by just 0.2%).</em></p>
<?php
if (isset($e_errorMessage)):
?>
        <div id="error_messag">
            <?php echo $e_errorMessage; ?>
        </div>
<?php endif; ?>
        <div id="income_form">
            <form method="post">
                <table id="income_data_table">
                    <tr>
                        <th><label for="gross_income">Gross income:</label></th>
                        <td><input type="text" name="gross_income" value="<?php echo $e_grossIncome; ?>"/></td>
                    </tr>
                    <tr>
                    <th><label for="expenses">Expenses:</label></th>
                        <td><input type="text" name="expenses" value="<?php echo $e_expenses; ?>"/></td>
                    </tr>
                    <tr>
                        <th><label for="expenses">No. of Employees:</label></th>
                        <td><input type="text" name="num_of_employees" value="<?php echo $e_numOfEmployees; ?>"/></td>
                    </tr>
                    <tr>
                        <td span="2"><input type="submit" value="Calculate"></td>
                    </tr>
                </table>
            </form>
        </div>
<?php
if ($fedTaxes instanceof FedTaxes):
?>
        <div id="taxes_data">
            <h3>Tax Information</h3>
            <table id="tax_data_table">
                <tr>
                    <th>Income Tax: </th>
                    <td><?php echo $fedTaxes->income; ?></td>
                </tr>
                <tr>
                    <th>Social Security Tax:</th>
                    <td><?php echo $fedTaxes->ssi; ?></td>
                </tr>
                <tr>
                    <th>Medicare Tax:</th>
                    <td><?php echo $fedTaxes->medicare; ?></td>
                </tr>
                <tr>
                    <th><a href="http://workforcesecurity.doleta.gov/unemploy/uitaxtopic.asp">Unemployment Tax:</a></th>
                    <td><?php echo $fedTaxes->unemployment; ?></td>
                </tr>
                <tr>
                    <th>Total Liability:</th>
                    <td><?php echo $fedTaxes->total; ?></td>
                </tr>
            </table>
        </div>
<?php endif; ?>
    </body>
</html>

Answer 1

Are you a horrible programmer...no probably not.

Is there room for improvement? There usually is.

here's my two cents: I see one factory class, but then you have a whole-lotta creation work going on throughout the rest of your code new fMoney, new TaxBracketManager, new TaxBracket, etc...

When you hard code these dependencies into your methods how can you test them? What if you needed to inject a Stub/Mock/Spy into your code to verify some conditional logic is actually executed; or executed in a specific manner? What if you needed to inject a Saboteur into you code to validate your failure points and ensure your code dies gracefully? Continue down your path w/ Dependency Injection and you can work to make this better. If you're having a hard time trying to figure out how to remove these, then maybe look into an abstract factory pattern.

Speaking of DI and Factories, your USFederalTaxesFactory instantiates a copy of your USFederalIncomeTax object but never passes in the fourth parameter, $bracketManager. Two things wrong I see here: You're doing "work" in the constructor every single time you call this factory b/c it does a conditional check (big no-no) to see if the object is NULL, then does more "work" by creating a new object (another big no-no) since it evaluates to true every single time - just pass in what you want to pass in.

For example, FedTaxes, should have each of those five fMoney objects created outside of it and passed in as parameters and set on those object properties.

new FedTaxes(new fMoney(0, 'USD'), new fMoney(0, 'USD'), new fMoney(0, 'USD'), new fMoney(0, 'USD'), new fMoney(0, 'USD'));

class FedTaxes
{
    /** @var fMoney */
    public $income;
    /** @var fMoney */
    public $ssi;
    /** @var fMoney */
    public $medicare;
    /** @var fMoney */
    public $unemployment;
    /** @var fMoney */
    public $total;

    public function __construct(fMoney $income, fMoney $ssi, fMoney $medicare,
                                fMoney $unemployment, fMoney $total)
    {
        $this->income = $income;
        $this->ssi = $ssi;
        $this->medicare = $medicare;
        $this->unemployment = $unemployment;
        $this->total = $total;
    }
}

Strive to write testable code and you will find the by-product is cleaner more loosely coupled objects that you can reuse more easily and maintain exponentially easier.

Also, it looks like your tax classes UnemploymentTax, MedicareTax, etc.. are a good use for the Strategy Design Pattern. What was your reason to make the tax rates of these classes methods instead of class constants? ie: CONST TAX_RATE = 0.0145. These seem like prime candidates for a constant to me.

...thats just a few quick things that popped out at me, hopefully you can take something away from it.

Answer 2

People who want to teach other should not write code like this.

1. Not even slightest indication that you understand what Separation of Concerns is. The pattern that you are using is called Big Ball of Mud. You have everything in the same file.

2. No understanding of HTML. The code you wrote is not valid, it uses tables for layout and completely ignores semantics.

3. Code is tightly coupled. Instead of using dependency injection, you have sprinkled new operator all over the codebase.

4. Why have you written an interface if you never use it ?

5. Pointless use of static factory antipattern.

6. Computation in the constructors making them virtually untestable.

7. Long methods with high cyclomatic complexity.

8. Hardcoded values in almost every class violating OCP.

9. Two classess with not behavior: TaxBracket and FedTaxes. And some magical fMoney class which has never been defined.

Basically , if someone handed this to me as a homework, I would give C. Code demonstrates knowledge of language structures, but no understanding of underlying principles and practices.

Answer 3

Veilig is right and made some very good points +1. There's nothing "horrible" about this code. Everyone can stand to use some improvement somewhere, even professionals. So saying this is "horrible" is just naive and immature. Don't listen to anyone with such negative criticism, especially if they aren't willing to explain themselves. Even constructive criticism should be taken with a grain of salt. Criticism is good to learn from, but you shouldn't let it dictate how you do something. Take note of it, do your own research, and come to your own conclusions. Especially mine :) That being said, here's some more research for you.

Comments are good, but should not be excessive, should not be useless, and their placement is important too. Upon first scanning this code I did not realize you had code between your first comment blocks, I thought that was just a long introductory comment. This would have been easier to spot if you were using the proper comment syntax for multiline comments (see below). Similarly, you should not have comments inside you methods, use PHPDoc comments to explain what needs to be explained and let your code explain the rest. Especially don't have comments INSIDE your parameter lists as this makes them hard to read and could cause confusion. As for useless comments, don't have a comment just to be witty. There were a couple of these and they just distract from the code. Example // I **LOVE** our complicated corporate income tax system!! WOOO!!!!!. This right here would make other programmers think you are immature and might be why someone might throw this kind of insult at you. Not saying they are right to do so, just that I've seen people get bashed for less.

/* This uses my Thrive framework, which extends
   the Flourish framework.
   NOTE: Using require, or require_once has performance penalties that aren't
        necessary.  If the file isn't loaded successfully, the app will crash
        anyway.
*/

Speaking of PHPDoc comments, the one's you have here could use some work. You shouldn't define a doccomment in the middle of a foreach loop. At this point it isn't even a doccomment anymore. Doccomments, besides being used to documment your code, are used to interface your code with your IDE. Good doccomments allow you to easily see what is going on without needing to go to the file/method/property/class in question. Additionally, doccomments should come before the element being documented rather than after, there was at least one instance where this happened, though that was inside a method and so the doccomment was useless anyways. And, as mentioned above, they shouldn't be inside your parameter lists, I found one inside the parameter list for a foreach loop. Correct doccomments look like this:

/** Sometimes this should have a description as well
  * @var fMoney[]
  */
protected $taxLiabilities;

/** Short Desc
 *
 * Long Description
 * @param $tax
 * @return fMoney
 * @throws LogicException
 */
public function getLiabilityByTax( $tax ) {

Incorrect:

$tax = new $taxClass();
/** @var $tax API_Tax */

foreach ($this->brackets as /** @var TaxBracket */ $bracket)

I don't think there is such a thing as a "Magic Constant" at least not in the same way that you mean. PHP has magic constants such as __FILE__, but a "magic" anything usually refers to an undefined number or string that was not set up as a variable or constant, is hardcoded into the script, and usually is untraceable or unintelligible. So, a constant, by definition, can not be "magical" as it has been defined and it is known where it has come from. As pointed out, that is usually how "magic" things are fixed. Now, your comment about moving this into its own class to fix this is concerning. A class should not just contain constants. If you have a number of constants you want to integrate into a system, you can set up a config file, but a class is overkill. So the TaxBracket class also seems a bit unjustified. The following are examples of "magical" things I found in your code.

/*Would be better as a constant (because it shouldn't change)
and placed into that config file I mentioned above. As it stands
these are just numbers. Numbers with structure, but numbers.*/
$bracketsInfo = array(
    array('min' => 0,        'max' => 50000,    'rate' => 0.15),
    array('min' => 50001,    'max' => 75000,    'rate' => 0.25),
    array('min' => 75001,    'max' => 100000,   'rate' => 0.34),
    array('min' => 100001,   'max' => 335000,   'rate' => 0.39),
    array('min' => 335001,   'max' => 10000000, 'rate' => 0.34),
    array('min' => 10000001, 'max' => 15000000, 'rate' => 0.35),
    array('min' => 15000001, 'max' => 18333333, 'rate' => 0.38),
    array('min' => 18333334, 'max' => null,     'rate' => 0.35),
);

/*Where did "110100" come from? What is it? What about "USD"?
These are both magical. They came from no where and have no
explanation. USD should be assumed as we are talking about US
Federal Taxes, so this should be set up as a constant or default
parameter value.*/
protected function fetchMaxTaxableAmount()
{
    return new fMoney(110100, 'USD');
}

/*Again, this would be better served as a constant. It isn't going to change
except from year to year, and if your config file is set up properly it will be
easy enough to exchange these tax brackets and rates for each year*/
protected function fetchTaxRate()
{
    return 0.062;
}

Make sure to clean up your debugging code from live code. There are a few instances where you still have $_GET[ 'debug' ] in your code, and all it is doing now is making your code harder to read because of the bulk these statements add. I understand this was done in a livecast and maybe you got cut off short, but this still stands.

Finally, make sure you separate your code logically into different files. I can't tell from this post, but from the looks of it, some of this code appears to be grouped into the same file. Each class should be in its own file. Each interface in its own. Configuration settings, such as setting the include path, should be done in the config file, or in the HTML file in which this code is called.

I hope this helps!

Answer 4

Looking at this code, would you say I'm an OK coder?

Yes, I would say that you are an OK coder even though there are some fairly egregious issues with the code you've presented.

Everyone has to start somewhere and I personally love teaching others how to get better and to become better multi-discipline programmers. This generally has the nice side-effect of making one a better coder.

At the end of the day, the world doesn't need YASPT (yet another s***y PHP tutorial).

I just created this during a live screencast. The people who were on there were very very very critical of almost every line of code. They say I am a horrible coder and know nothing.

A couple things:

• I would recommend that if you are charging people for this, you do some preparation. To teach beginners bad practices and wave it off under the guise of "doing it on the fly" is disrespectful, unprofessional, and unethical.
• If you really want to continue, you should study the methods of true professionals such as Gary Bernhardt and Geoffrey Grosenbach.