patternphpMajor
Heavily-criticized tax calculator
Viewed 0 times
taxheavilycriticizedcalculator
Problem
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.
```
'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 ($bracket
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.
```
'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 ($bracket
Solution
People who want to teach other should not write code like this.
-
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.
-
No understanding of HTML. The code you wrote is not valid, it uses tables for layout and completely ignores semantics.
-
Code is tightly coupled. Instead of using dependency injection, you have sprinkled
-
Why have you written an interface if you never use it ?
-
Pointless use of static factory antipattern.
-
Computation in the constructors making them virtually untestable.
-
Long methods with high cyclomatic complexity.
-
Hardcoded values in almost every class violating OCP.
-
Two classess with not behavior:
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.
-
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.
-
No understanding of HTML. The code you wrote is not valid, it uses tables for layout and completely ignores semantics.
-
Code is tightly coupled. Instead of using dependency injection, you have sprinkled
new operator all over the codebase.-
Why have you written an interface if you never use it ?
-
Pointless use of static factory antipattern.
-
Computation in the constructors making them virtually untestable.
-
Long methods with high cyclomatic complexity.
-
Hardcoded values in almost every class violating OCP.
-
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.
Context
StackExchange Code Review Q#15307, answer score: 25
Revisions (0)
No revisions yet.