Page 1 of 1

Is this good coding?

Posted: Sat Sep 23, 2006 11:45 am
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,

Posted: Sat Sep 23, 2006 11:50 am
by feyd
Are you sure you wanted to post this here, in PHP - Code, and not in Code Critique?

Posted: Sat Sep 23, 2006 11:51 am
by impulse()
Theroy & Design?

Posted: Sat Sep 23, 2006 11:53 am
by feyd
T&D is not for code review of this sort.

Posted: Sat Sep 23, 2006 11:59 am
by impulse()
OK. Can you move me to Code Critique please?

Posted: Sat Sep 23, 2006 11:59 am
by Chris Corbyn
Moved it to Critique ;)

Posted: Sat Sep 23, 2006 12:55 pm
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.

Posted: Sat Sep 23, 2006 1:15 pm
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.

Posted: Sat Sep 23, 2006 1:52 pm
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!

Posted: Sat Sep 23, 2006 2:43 pm
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>

Posted: Sat Sep 23, 2006 3:12 pm
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.

Posted: Sat Oct 07, 2006 10:21 pm
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.