Welcome Guest, Not a member yet? Register   Sign In
First mini app--Please offer constructive criticism
#1

[eluser]echo_boom[/eluser]
This is my first attempt at MVC coding/scripting--please be kind. I would like some guidance on ANYTHING I could be doing better.

I have posted 4 files to github: gist:

1 Controller: ‘main.php’
https://gist.github.com/2651546

2 Views: ‘control_panel.php’ and ‘main.php’
https://gist.github.com/2651569
https://gist.github.com/2651579

1 helper extension file: ‘MY_file_helper.php’
https://gist.github.com/2651556

This mini web application allows a user to upload an image. It will then check to see if the image has greater width than 405 px and also that it’s aspect ratio is 1:1 - 3:2 for landscape OR 1:1 - 2:3 for portrait. If the image meets that criteria it will then resize the image down to 405px width using
$config[‘master_dim’] = ‘width’;

I am trying to create a control panel page for users in a hypothetical company where it displays their avatar and allows them to upload a new image.

I have not included any database interaction yet. I plan on adding this once I know I’m on the right track. After I add the database interaction, I will ask once more for some constructive criticism, but I pick things up relatively quickly and I’ll probably be bothering everyone on the forums much less after that.

Please help and I appreciate all those who spend even 1 minute helping me. Thank you!
#2

[eluser]Rok Biderman[/eluser]
It looks perfectly fine to me, except I would use flashdata to send the error info to control panel and then redirect rather than just load the view. You might have other functions in the panel that expect the whole control_panel method invoked and this is a good way of futureproofing.
#3

[eluser]echo_boom[/eluser]
Thank you for the feedback Smile.

So I read up on what you suggested: Flashdata (Session Class) and redirect function (URL helper), but was confused on how it could be implemented in my situation.

So I should keep loading the VIEW control_panel ONLY inside the control_panel() function inside CONTROLLER, but for the load VIEWS inside the do_upload() function, you are suggesting those be changed so that they redirect to the control_panel() function? Like:

redirect('/control_panel/');
* my routes.php in config directory is setup so I can just put in controller method instead of all the URI segments

So basically, you are saying that anything that is NOT inside the control_panel() function in main.php CONTROLLER should be redirected rather than load view? The loading of VIEW control_panel should ONLY TAKE PLACE inside the control_panel() function?

[quote author="Coccodrillo" date="1336636898"] You might have other functions in the panel that expect the whole control_panel method invoked and this is a good way of futureproofing.[/quote]

I am not sure what you mean by this due to my lack of experience. Can you please provide me with a hypothetical situation I might run across in the future that helps me understand what you are talking about?

Thank you again!
#4

[eluser]Rok Biderman[/eluser]
Sure, I'll try to explain. You try to upload a file and something doesn't work and this view gets loaded. You are now on main/do_upload insted on main/control_panel url. Now, anything you might have with relative paths in it or using segments from the uri class gets messed up. The second thing is, your errors will automatically get erased after displaying once, no need worrying about setting them to ''.

If you instead do something like inside you main contro_panel view
Code:
if (isset($this->session->flashdata('error'))) {
    echo $this->session->flashdata('error');
}

you could also use this for getting errors from any other methods you might use. If you start using modular extensions (HMVC) it's might get even more handy.

There is nothing that could be called wrong with your design, in fact I like it, this is just the way I've gotten used to doing things that in my experience takes the least effort. I've seen some Codeigniter users do it either way.

After rereading this, maybe I'm not super concise this early. If I come up with a better example I'll post here again.
#5

[eluser]echo_boom[/eluser]
Thank you again for the reply! It took me a few days, but I have changed my mini app to what your advice suggested. Now that I play around with it and reload the control_panel VIEW and whatnot I see that it is MUCH, MUCH cleaner this way.

Please check the CONTROLLER main.php and the VIEW control_panel.php which have changed:
Controller:
https://gist.github.com/2722411
View:
https://gist.github.com/2722421

Basically, instead of setting an error/status variable and loading the VIEW control_panel, I set the FLASHDATA and then REDIRECT to URI “…/main/control_panel” which is the function control_panel() that loads my VIEW control_panel.php with NO variable passed to it since flashdata was set. I know there might be a few ways to implement a flashdata setting and then redirection, but this is what I found to be most logical, so if there are issues you see with that please let me know =).

[quote author="Coccodrillo" date="1336809865"]

If you instead do something like inside you main contro_panel view
Code:
if (isset($this->session->flashdata('error'))) {
    echo $this->session->flashdata('error');
}

[/quote]

I found out that isset() does not work with methods such as $this->session->flashdata(), but rather the FLASHDATA is set to FALSE if there is nothing there so I just checked if it was FALSE instead.

Please have a look again--I eagerly await your reply. Thank you!
#6

[eluser]echo_boom[/eluser]
http://ellislab.com/forums/viewthread/183398/#867776

I found an old thread that states that isset() can not be used on methods. Please have a look and thank you again... I eagerly await your approval of updated app =)!
#7

[eluser]Rok Biderman[/eluser]
I was writing blindly, must have mistaken it with another method. What you did now is correct other than you could now just write

Code:
echo $this->session->flashdata('img_info');

Now, there was nothing wrong with your upload functions from the start, but I found this link. Very similar to what you are doing here, but without the usage of the middling temp folder which is perhaps something worth considering (ignore the ugly usage of $_FILES though). Also in a grander view (considering what we have here would be just a small part of an application) I wouldn't create custom static routes for upload functions, but this is unimportant.

You're good to go, imo. Don't worry about architecture too much. You'll hate the way you just did it in a year anyway. I know I did.
#8

[eluser]echo_boom[/eluser]
[quote author="Coccodrillo" date="1337327533"]Very similar to what you are doing here, but without the usage of the middling temp folder which is perhaps something worth considering [/quote]

What would the server/hardware performance benefits--if any--be in NOT using a middle temp folder? I only plan on getting traffic from hypothetical employees for a company (minimal traffic worst case scenario), but I still am curious on how websites with large amounts of traffic implement such image resizing techniques.

[quote author="Coccodrillo" date="1337327533"]I wouldn’t create custom static routes for upload functions, but this is unimportant.[/quote]

Are you referring to the “…/application/config/routes.php” routes configuration file where I:

Code:
$route['default_controller'] = "main";
$route['404_override'] = '';
$route['(:any)'] = "main/$1";

Thanks again :gulp:
#9

[eluser]Rok Biderman[/eluser]
First of all, I think we're talking minute details here. One way or the other is ok. But I was working under assumption that this is your first try and the complexity (also traffic) of your app will increase. If not, what you posted the first time is superb.




Theme © iAndrew 2016 - Forum software by © MyBB