D5145c421cd25af6fa577c15219add90

This method feels like it's way more complicated than it should be.

Update: added <?php to colourise.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
<?php
function get_handle($type)
{
	if (type=="r")
	{
		if (isset($this->fhr)) return $this->fhr;
		else user_error("File not open for reading");
	}
	elseif (type=="w")
	{
		if (isset($this->fhw)) return $this->fhw;
		else user_error("File not open for writing");
	}
	elseif (type=="a")
	{
		if (isset($this->fha)) return $this->fha;
		else user_error("File not open for appending");
	}
	elseif (!isset($type))
	{
		if (isset($this->fhr)) return $this->fhr;
		elseif (isset($this->fhw)) return $this->fhw;
		elseif (isset($this->fha)) return $this->fha;
		else user_error ("File not open");
	}
	else user_error("Invalid type");
}
?>

Refactorings

No refactoring yet !

73415a883aec7c0a7aada9c4cdb208b5

Christoffer

February 9, 2010, February 09, 2010 08:03, permalink

No rating. Login to rate!

Your code isn't bad, but here is a cleaner version. $type will allways have to be set if you do not define a standard value in the function definition, so we can skip that part entirely.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
<?php
function get_handle($type)
{
  // If everything is ok, just return what was requested.
  $mode = 'fh'.$type;
  if ( isset($this->$mode) )
  {
    return $this->$mode;
  }
	
  // If the handle is not set, generate an error.
  switch($type)
  {
    case 'r':
      user_error("File not open for reading");
      break;
    case 'w':
      user_error("File not open for writing");
      break;
    case 'a':
      user_error("File not open for appending");
      break;
    default:
      user_error("Invalid type");
  }
}
?>
D5145c421cd25af6fa577c15219add90

Nathan

February 9, 2010, February 09, 2010 10:17, permalink

No rating. Login to rate!

Thanks for that. It does make the code a lot simpler setting a default $type, but the ideal is that if the user leaves it blank, the function detects how the file has been opened (obviously it can be opened in more that one way, but this would be rare, and they shouldn't be leaving it blank if this is the case). Having said all that, I've had a go at refactoring your solution further!!!

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
<?php
function get_handle($type='!') // $type set to something invalid, so that user explicitly has to declare it.
{
  // If everything is ok, just return what was requested.
  $mode = 'fh'.$type;

  if ( isset($this->$mode) )
    return $this->$mode;

  $types = array('r' => 'reading', 'w' => 'writing', 'a' => 'appending');

  // Work out what kind of error to return
  if (in_array($type,array_keys($types)))
    user_error("File not open for ".$types[$type]);
  else
    user_error("Invalid type");
}
?>

Your refactoring





Format Copy from initial code

or Cancel