HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Blog engine or somesuch

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
blogenginesomesuch

Problem

Would someone tell how I can improve this piece of code?

```
per_page = $per_page;
}

function set_total_page($total) {
$this->totalPage = $total;
}

function set_page($page) {
$this->page = $page;
}

function get_page() {
return $this->page;
}

function get_per_page() {
return $this->per_page;
}

function get_total_page() {
return $this->totalPage;
}

function counts($cats) {
$sql = "select count(post_id) from Post where category='$cats'";
$result = $this->query($sql);
$rows = mysqli_fetch_row($result);

return $rows;
}

function cats() {
$sql = "select distinct category from Post ";
$result = $this->query($sql);
while ($rows = mysqli_fetch_array($result)) {
$row[] = $rows;
}
mysqli_free_result($result);
$this->closeDb();
return $row;
}

function postByCats($cats) {
if ($cats == NULL)
return $this->Posts($cats);
$sql = "select * from Post where category='$cats'";
$result = $this->query($sql);
while ($row = mysqli_fetch_assoc($result))
$tuts[] = $row;
mysqli_free_result($result);
$this->closeDb();
return $tuts;
}

function Posts($many = NULL) {
$sql = "select count(post_id) from Post";
$per = $this->set_per_page(3);
$query = $this->query($sql);
$num = mysqli_fetch_row($query);
$numrows = $num[0];
if (isset($_GET['page']) && is_numeric($_GET['page'])) {
$num = intval($_GET['page']);
$this->set_page($num);
} else {
$num = 1;
$this->set_page($num);
}

//offset and number per page
$total = ceil($numrows / $this->get_per_page());
$this->set_total_page($total);
$num_off = $this->get_per_page();
if ($this->get_page() > $this->get_tota

Solution

If the code will be distributed, read or maintained by others, you might want to look at your naming conventions as they're a bit inconsistent, for example sometimes you use underscore separated names such as get_per_page(), but other times you use sulkingCamelCase such as postByCats() and Posts() in class case. Either one is fine, as long as you use the same throughout.

Aside from your single line getters and setters, the function names you've chosen are not always indicative of what the function does. Ideally you shouldn't have to read the actual code to know what a function does, consider these changes:

function counts($cats)
function getNumberOfPostsForCategory($category)

function cats()
function getCategories()


You also haven't got any access modifiers on your methods. As paginate() directly prints out a segment of HTML which wouldn't be valid on its own, I'm guessing that it's probably only used once somewhere in your View and as such should be private or protected to prevent misuse.

As for the actual code, aside from collapsing some statements there's not much to change. These lines in paginate() don't appear to be doing anything though:

$sql = 'select count(post_id) from Post';
$query = $this->query($sql);


In addition to the empty else statement in the Posts() method.

Code Snippets

function counts($cats)
function getNumberOfPostsForCategory($category)

function cats()
function getCategories()
$sql = 'select count(post_id) from Post';
$query = $this->query($sql);

Context

StackExchange Code Review Q#51875, answer score: 2

Revisions (0)

No revisions yet.