Is this good coding?

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

Post Reply
impulse()
Forum Regular
Posts: 748
Joined: Wed Aug 09, 2006 8:36 am
Location: Staffordshire, UK
Contact:

Is this good coding?

Post by impulse() »

Would someone mind reviewing my code and tell me if that's as best I could do a calculator. Are there are advanced methods of doing such a project?

Code: Select all

<form method="post"
      action="calc1.php">
<center><input type="text"
       name="valOne"
       value=""><br>
<input type="radio"
       name="calc"
       value="add"/> Add<br>
<input type="radio"
       name="calc"
       value="subtract"/> Subtract<br>
<input type="radio"
       name="calc"
       value="divide"/> Divide <br>
<input type="radio"
       name="calc"
       value="multiply"/> Multiply <br>
<input type="text"
       name="valTwo"
       value=""><br>
<input type="submit"
       value="=">
</form>
<?php

$valOne = $_REQUEST["valOne"];
$valTwo = $_REQUEST["valTwo"];
$calculation = $_REQUEST["calc"];

function add($valOne, $valTwo) {
  return $valOne + $valTwo;
  }

function subtract($valOne, $valTwo) {
  return $valOne - $valTwo;
  }

function divide($valOne, $valTwo) {
  return $valOne / $valTwo;
  }

function multiply($valOne, $valTwo) {
  return $valOne * $valTwo;
  }

if ($calculation == "add") {
  $equals = add($valOne, $valTwo);
  ?><h2><?php
  echo "$valOne $calculation $valTwo = $equals";
  $firstVisit == "no";
  }
   
if ($calculation == "subtract") {
  $equals = subtract($valOne, $valTwo);
  ?><h2><?php 
  echo "$valOne $calculation $valTwo = $equals";
  $firstVisit == "no";
  }
   
if ($calculation == "divide") {
  $equals = divide($valOne, $valTwo);
  ?><h2><?php
  echo "$valOne $calculation $valTwo = $equals";
  $firstVisit == "no";
  }
   
if ($calculation == "multiply") {
  $equals = multiply($valOne, $valTwo);
  ?><h2><?php
  echo "$valOne $calculation $valTwo = $equals";
  $firstVisit == "no";
  }
   
   
?>
http://stesbox.co.uk/php/adcalc.php Preview

Regards,
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

Are you sure you wanted to post this here, in PHP - Code, and not in Code Critique?
impulse()
Forum Regular
Posts: 748
Joined: Wed Aug 09, 2006 8:36 am
Location: Staffordshire, UK
Contact:

Post by impulse() »

Theroy & Design?
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

T&D is not for code review of this sort.
impulse()
Forum Regular
Posts: 748
Joined: Wed Aug 09, 2006 8:36 am
Location: Staffordshire, UK
Contact:

Post by impulse() »

OK. Can you move me to Code Critique please?
User avatar
Chris Corbyn
Breakbeat Nuttzer
Posts: 13098
Joined: Wed Mar 24, 2004 7:57 am
Location: Melbourne, Australia

Post by Chris Corbyn »

Moved it to Critique ;)
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Post by Christopher »

It looks like you are going in the right direction. I would make the first checks be to validate that the values and operand are ok. Then a switch statement would probably reduce some code duplication rather than all of those ifs.
(#10850)
alex.barylski
DevNet Evangelist
Posts: 6267
Joined: Tue Dec 21, 2004 5:00 pm
Location: Winnipeg

Post by alex.barylski »

Are there advanced methods of doing such a project?

Yup. :P

In school I wrote a calculator which parsed infix mathematical expressions and used reverse polish notation to evaluate them. That is an example of an advanced approach.

Today you have functions and runtime expression evaluation at your fingertips using eval() and the like.

Using a GUI certainly makes things more interesting...so I'd personally stick with the approach your using.

If your using PHP then you would really complicate things by implenting a stack just to emulate a real calculator, because you would need to store your operators/operands in a session stack.

If this is for a school project and you MUST use PHP use the method your using. If you can use Javascript, and want bonus points, implement the calculator using a stack and RPN (reverse polish notation).

Sessions would add a lot of fluff to a small project like this.
User avatar
Ambush Commander
DevNet Master
Posts: 3698
Joined: Mon Oct 25, 2004 9:29 pm
Location: New Jersey, US

Post by Ambush Commander »

There's an implementation of this in PHP in the MediaWiki extension ParserFunctions. If you want a sample I suggest you go look it up, but try implementing it yourself first!
User avatar
Benjamin
Site Administrator
Posts: 6935
Joined: Sun May 19, 2002 10:24 pm

Post by Benjamin »

The way you have formatted your HTML is a personal pet peeve of mine. The first thing I do when I see code like that is change it. I guess it boils down to preference but I certantly don't like it. Also, your center tag is depreciated and wasn't closed. Other than that you did a good job.

Code: Select all

<form method="post" action="calc1.php">
  <div style="text-align: center;">
    <input type="text" name="valOne" value="" /><br />
    <input type="radio" name="calc" value="add" /> Add<br />
    <input type="radio" name="calc" value="subtract" /> Subtract<br />
    <input type="radio" name="calc" value="divide" /> Divide <br />
    <input type="radio" name="calc" value="multiply" /> Multiply <br />
    <input type="text" name="valTwo" value="" /><br />
    <input type="submit" value="=" />
  </div>
</form>
User avatar
Chris Corbyn
Breakbeat Nuttzer
Posts: 13098
Joined: Wed Mar 24, 2004 7:57 am
Location: Melbourne, Australia

Post by Chris Corbyn »

astions wrote:Also, your center tag is depreciated and wasn't closed.
Depends upon the DTD being used. But after you mentioned that I look at the OP's code to see a combination of self-closing and non-closed tags so whatever doctype is being used it won't validate.
User avatar
RobertGonzalez
Site Administrator
Posts: 14293
Joined: Tue Sep 09, 2003 6:04 pm
Location: Fremont, CA, USA

Post by RobertGonzalez »

I know it's been a couple of weeks, but I took a look at the code and noticed that there is absolutely no validation for type. If you are making a calculator you would think that the inputs would be numeric, but there is no testing for that. In fact, it will take any input and try to use it.

There is also no validation for actual passed data. So you can submit an empty form with no notices.

The use of $_REQUEST when there is no expectation of $_GET or $COOKIE should be changed to $_POST.

And as has been mentioned, I would use a switch for calculating inside the function.
Post Reply