Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have a wordpress code, which I am using to set and retrieve user-meta.

Here is the code:

<h3><?php _e("Posted by",'AuctionTheme');?>:</h3>
<p><a href="<?php echo get_bloginfo('siteurl');?>/user-profile/<?php the_author() ?>"><?php the_author();?></a>

<?php // favorite image
global $current_user;
get_currentuserinfo();
$user_id = $current_user->ID;
$key = "FevSel";
$single = 'true';

global $post;

$seller = $post->post_author;

if($_POST['update-fav-sel-meta'])
{
    update_user_meta($user_id,$key,$seller);
}                       

$fav_sellers = get_user_meta($user_id,$key,$single);
echo $fav_sellers ."hello";

if($search == $fav_sellers)
{
?>

    <div class="btn-fav-true">
        <button class="btn-fav-false">Favorite Seller</button>
    </div>

<?php
}
else
{
?>
<div class="btn-fav-false">
    <form action="#">
        <input type="hidden" name="do" value="update-fav-sel-meta">
        <input type="submit" value="Add as favorite" />
    </form>
</div>

<?php
} 
?>
</p>

Please let me know where I am making the mistake.

Notes:

  1. It is kind of boolean button. If an author is not listed on current logged in user's favorite list, we add him. If he is listed, then we give a button indicating that this author is listed on your favorite list. I plan to add a (X) near the true button so the user can remove the author from the list.

  2. When using update_post_data with a single string, instead of array, how do I append a string value to the already stored array in post_meta?

  3. I am using Auction Theme from SiteMile framework.

Thanks.

share|improve this question
2  
this should be in StackOverflow, since it's not a working piece of code but a request for an advice – Zathrus Writer Aug 17 '12 at 8:24
Thank you Zathrus. I will post this on Stack Overflow. – Darshan Thanki Aug 17 '12 at 10:17

closed as off topic by Corbin, James Khoury, Paul, Winston Ewert Aug 28 '12 at 22:29

Questions on Code Review Stack Exchange are expected to relate to code review request within the scope defined by the community. Consider editing the question or leaving comments for improvement if you believe the question can be reworded to fit within the scope. Read more about reopening questions here.If this question can be reworded to fit the rules in the help center, please edit the question.

1 Answer

First of all, I edited your post for indentation... That was just bad... Your code should never be that heavily indented, HTML, PHP or otherwise. Indentation is first and foremost a tool for legibility. When it starts getting in the way of that, then something is wrong. Typically that means you are trying to do too much, or you could be doing something wrong. I would suggest looking at your HTML and figuring out what is causing all that indentation and clean it up or eliminate it. Now, you were asking about that PHP... I can't tell if you are saying you are actually having issues with this code, or if you just want help cleaning this up. Either way, I'm not just going to answer the questions you posed at the end, this is Code Review after all. I want to help you make this look better, so please read the entire post.

What is _e()? What does it do? This is a horrible function name. Variables and functions should be descriptive, but not excessively so. Try to leave unnecessary words out of your variable and function names, such as the "the" in the_author(). Another problem with these functions, two apparently echo HTML somewhere inside instead of returning a value that can be echoed like get_bloginfo() does. Functions that return HTML are hard to reuse, functions that return values can be reused in many different ways. Another benefit to doing this is that the return values of those functions can be set to variables so that you only have to call them once. For instance, you are calling the_author() twice, and if, as I suspect, this function accesses a database, you are adding unnecessary overhead to your website. Also, don't mix your quotations. Pick one.

Don't use globals! Here is your mantra, "Globals are bad. Eval is Evil." If you repeat that to yourself everyday and remember it, I guarantee you will be a better programmer for it. Not that you are using eval, but remember that anyways. Now, I'm assuming the reason you are setting $current_user as a global is so that get_currentuserinfo() can use it to set up the user class. Instead, set up your user class like normal, but then return it so that you can set its value to $current_user directly.

Alright, I understand why you set $user_id, I am even assuming that you set $key so that it can easily be changed. But why set $single? And why as a string? I would just remove that and declare TRUE directly in the get_user_meta() function.

Again, another global, but this time I have no idea where you are getting it from, and this is the biggest problem with globals. I can't help you here except to say, repeat your mantra. If you need to pass values from one function to another, use the function's parameters and return the results. If you need to pass values from one page to another, use the GET, POST, COOKIE, or SESSION arrays.

You should never use POST, GET, or any user supplied data directly. You are just asking for cross site scripting. Check out PHP's filter_input() function if your PHP version is >= 5.2. Also, I am unsure, but can you actually use hyphens "-" in POST/GET data? I think its just like variables and such and this is considered an illegal character.

$update = filter_input( INPUT_POST, 'update-fav-sel-meta', FILTER_SANITIZE_STRING );

What is $search? Where did it come from? I can't find it defined. PHP is probably throwing a warning here and then saying something along the lines, "Can't find $search, assuming SEARCH" and then proceeding as if you were comparing the string "search" to $fav_sellers.

I'm not sure what this update_post_data() function is that you are speaking of, but if you want to append data to a pre-existing array you would do so like this.

$array[] = $string;

Hope this helps, and sorry if I did not answer your initial question, its a bit vague.

share|improve this answer

Not the answer you're looking for? Browse other questions tagged or ask your own question.