Skip to main content
Code Review

Return to Answer

First suggestion is that you shouldn't write the same code twice.

Your $_GET-if-conditions can be put into a separate function.

$this->handleCookieByGetParameter('c', 'qanda_questions_category');
$this->handleCookieByGetParameter('t', 'qanda_questions_tag');
public function handleCookieByGetParameter($parameter, $name)
{
 if ( isset($_GET[$parameter]) ) {
 if ( empty($_GET[$parameter]) ) {
 unset_cookie($name);
 } else {
 set_cookie($name, $parameter$_GET[$parameter]);
 }
 }
}

and then I recommend that you check your error before you handle your other stuff:

if ( !$this->category_cookie && !$this->tag_cookie ) {
 return header("Location: /myweb/questions".make_url_query($_GET, [],['c','t']));
 // better an RedirectException
}
$query_where = $query_join = '';
if ( $this->category_cookie ) {
 $query_where .= "AND qa.category = :c";
 $this->parameters[":c"] = $_COOKIE['qanda_questions_category'];
}
if ( $this->tag_cookie ) {
 $query_join .= " INNER JOIN qanda_tags qt ON qt.qanda_id = qa.id
 INNER JOIN tags ON tags.id = qt.tag_id";
 $query_where .= " AND tags.name = :t";
 $this->parameters[":t"] = $_COOKIE['qanda_questions_tag'];
}
return $this->index($query_where, $query_join, __FUNCTION__);

my next step would be to look to extract the query code into a separate function, so I could use it for other methods too.

First suggestion is that you shouldn't write the same code twice.

Your $_GET-if-conditions can be put into a separate function.

$this->handleCookieByGetParameter('c', 'qanda_questions_category');
$this->handleCookieByGetParameter('t', 'qanda_questions_tag');
public function handleCookieByGetParameter($parameter, $name)
{
 if ( isset($_GET[$parameter]) ) {
 if ( empty($_GET[$parameter]) ) {
 unset_cookie($name);
 } else {
 set_cookie($name, $parameter);
 }
 }
}

and then I recommend that you check your error before you handle your other stuff:

if ( !$this->category_cookie && !$this->tag_cookie ) {
 return header("Location: /myweb/questions".make_url_query($_GET, [],['c','t']));
 // better an RedirectException
}
$query_where = $query_join = '';
if ( $this->category_cookie ) {
 $query_where .= "AND qa.category = :c";
 $this->parameters[":c"] = $_COOKIE['qanda_questions_category'];
}
if ( $this->tag_cookie ) {
 $query_join .= " INNER JOIN qanda_tags qt ON qt.qanda_id = qa.id
 INNER JOIN tags ON tags.id = qt.tag_id";
 $query_where .= " AND tags.name = :t";
 $this->parameters[":t"] = $_COOKIE['qanda_questions_tag'];
}
return $this->index($query_where, $query_join, __FUNCTION__);

my next step would be to look to extract the query code into a separate function, so I could use it for other methods too.

First suggestion is that you shouldn't write the same code twice.

Your $_GET-if-conditions can be put into a separate function.

$this->handleCookieByGetParameter('c', 'qanda_questions_category');
$this->handleCookieByGetParameter('t', 'qanda_questions_tag');
public function handleCookieByGetParameter($parameter, $name)
{
 if ( isset($_GET[$parameter]) ) {
 if ( empty($_GET[$parameter]) ) {
 unset_cookie($name);
 } else {
 set_cookie($name, $_GET[$parameter]);
 }
 }
}

and then I recommend that you check your error before you handle your other stuff:

if ( !$this->category_cookie && !$this->tag_cookie ) {
 return header("Location: /myweb/questions".make_url_query($_GET, [],['c','t']));
 // better an RedirectException
}
$query_where = $query_join = '';
if ( $this->category_cookie ) {
 $query_where .= "AND qa.category = :c";
 $this->parameters[":c"] = $_COOKIE['qanda_questions_category'];
}
if ( $this->tag_cookie ) {
 $query_join .= " INNER JOIN qanda_tags qt ON qt.qanda_id = qa.id
 INNER JOIN tags ON tags.id = qt.tag_id";
 $query_where .= " AND tags.name = :t";
 $this->parameters[":t"] = $_COOKIE['qanda_questions_tag'];
}
return $this->index($query_where, $query_join, __FUNCTION__);

my next step would be to look to extract the query code into a separate function, so I could use it for other methods too.

Improve Grammar
Source Link
Stephen Rauch
  • 4.3k
  • 12
  • 24
  • 36

at firstFirst suggestion is that you should don'tshouldn't write the same code twice.
your $_GET-if-conditions

Your $_GET-if-conditions can be put into a seperateseparate function.

$this->handleCookieByGetParameter('c', 'qanda_questions_category');
$this->handleCookieByGetParameter('t', 'qanda_questions_tag');
public function handleCookieByGetParameter($parameter, $name)
{
 if ( isset($_GET[$parameter]) ) {
 if ( empty($_GET[$parameter]) ) {
 unset_cookie($name);
 } else {
 set_cookie($name, $parameter);
 }
 }
}

and then I recommend that you to check your error before you handle your other stuff:

if ( !$this->category_cookie && !$this->tag_cookie ) {
 return header("Location: /myweb/questions".make_url_query($_GET, [],['c','t']));
 // better an RedirectException
}
$query_where = $query_join = '';
if ( $this->category_cookie ) {
 $query_where .= "AND qa.category = :c";
 $this->parameters[":c"] = $_COOKIE['qanda_questions_category'];
}
if ( $this->tag_cookie ) {
 $query_join .= " INNER JOIN qanda_tags qt ON qt.qanda_id = qa.id
 INNER JOIN tags ON tags.id = qt.tag_id";
 $query_where .= " AND tags.name = :t";
 $this->parameters[":t"] = $_COOKIE['qanda_questions_tag'];
}
return $this->index($query_where, $query_join, __FUNCTION__);

my next step willwould be to look like to extract the query code into a seperateseparate function, so I cancould use it for other methods too.

at first you should don't write the same code twice.
your $_GET-if-conditions can put into a seperate function.

$this->handleCookieByGetParameter('c', 'qanda_questions_category');
$this->handleCookieByGetParameter('t', 'qanda_questions_tag');
public function handleCookieByGetParameter($parameter, $name)
{
 if ( isset($_GET[$parameter]) ) {
 if ( empty($_GET[$parameter]) ) {
 unset_cookie($name);
 } else {
 set_cookie($name, $parameter);
 }
 }
}

and then I recommend you to check your error before you handle your other stuff:

if ( !$this->category_cookie && !$this->tag_cookie ) {
 return header("Location: /myweb/questions".make_url_query($_GET, [],['c','t']));
 // better an RedirectException
}
$query_where = $query_join = '';
if ( $this->category_cookie ) {
 $query_where .= "AND qa.category = :c";
 $this->parameters[":c"] = $_COOKIE['qanda_questions_category'];
}
if ( $this->tag_cookie ) {
 $query_join .= " INNER JOIN qanda_tags qt ON qt.qanda_id = qa.id
 INNER JOIN tags ON tags.id = qt.tag_id";
 $query_where .= " AND tags.name = :t";
 $this->parameters[":t"] = $_COOKIE['qanda_questions_tag'];
}
return $this->index($query_where, $query_join, __FUNCTION__);

my next step will look like to extract the query code into a seperate function, so I can use it for other methods too.

First suggestion is that you shouldn't write the same code twice.

Your $_GET-if-conditions can be put into a separate function.

$this->handleCookieByGetParameter('c', 'qanda_questions_category');
$this->handleCookieByGetParameter('t', 'qanda_questions_tag');
public function handleCookieByGetParameter($parameter, $name)
{
 if ( isset($_GET[$parameter]) ) {
 if ( empty($_GET[$parameter]) ) {
 unset_cookie($name);
 } else {
 set_cookie($name, $parameter);
 }
 }
}

and then I recommend that you check your error before you handle your other stuff:

if ( !$this->category_cookie && !$this->tag_cookie ) {
 return header("Location: /myweb/questions".make_url_query($_GET, [],['c','t']));
 // better an RedirectException
}
$query_where = $query_join = '';
if ( $this->category_cookie ) {
 $query_where .= "AND qa.category = :c";
 $this->parameters[":c"] = $_COOKIE['qanda_questions_category'];
}
if ( $this->tag_cookie ) {
 $query_join .= " INNER JOIN qanda_tags qt ON qt.qanda_id = qa.id
 INNER JOIN tags ON tags.id = qt.tag_id";
 $query_where .= " AND tags.name = :t";
 $this->parameters[":t"] = $_COOKIE['qanda_questions_tag'];
}
return $this->index($query_where, $query_join, __FUNCTION__);

my next step would be to look to extract the query code into a separate function, so I could use it for other methods too.

Source Link
Sysix
  • 126
  • 3

at first you should don't write the same code twice.
your $_GET-if-conditions can put into a seperate function.

$this->handleCookieByGetParameter('c', 'qanda_questions_category');
$this->handleCookieByGetParameter('t', 'qanda_questions_tag');
public function handleCookieByGetParameter($parameter, $name)
{
 if ( isset($_GET[$parameter]) ) {
 if ( empty($_GET[$parameter]) ) {
 unset_cookie($name);
 } else {
 set_cookie($name, $parameter);
 }
 }
}

and then I recommend you to check your error before you handle your other stuff:

if ( !$this->category_cookie && !$this->tag_cookie ) {
 return header("Location: /myweb/questions".make_url_query($_GET, [],['c','t']));
 // better an RedirectException
}
$query_where = $query_join = '';
if ( $this->category_cookie ) {
 $query_where .= "AND qa.category = :c";
 $this->parameters[":c"] = $_COOKIE['qanda_questions_category'];
}
if ( $this->tag_cookie ) {
 $query_join .= " INNER JOIN qanda_tags qt ON qt.qanda_id = qa.id
 INNER JOIN tags ON tags.id = qt.tag_id";
 $query_where .= " AND tags.name = :t";
 $this->parameters[":t"] = $_COOKIE['qanda_questions_tag'];
}
return $this->index($query_where, $query_join, __FUNCTION__);

my next step will look like to extract the query code into a seperate function, so I can use it for other methods too.

lang-php

AltStyle によって変換されたページ (->オリジナル) /