Raimondas's avatar

Finished my first Laravel ecommerce website github link

So I've finished my first Laravel ecommerce website and I'm kind a new to developing websites. This is a link to github repository:

https://github.com/Rhymondas/Armytag

Maybe you could tell me some opinions about the code, what should I do to make it more better ? I'm thinking about improving the code and making a really cool shoping cart that one day maybe I could sell :)

0 likes
9 replies
willvincent's avatar

At quick glance it looks like you're making some, possibly unreasonable, assumptions about things. For example, in the cart controller;

        $product['line1'] = $request->input('line1');
        $product['line2'] = $request->input('line2');
        $product['line3'] = $request->input('line3');
        $product['line4'] = $request->input('line4');
        $product['line5'] = $request->input('line5');
        $product['line6'] = $request->input('line6');
        $product['line7'] = $request->input('line7');
        $product['line8'] = $request->input('line8');
        $product['line9'] = $request->input('line9');
        $product['line10'] = $request->input('line10');

What if I needed 11 or 15 lines? Or only 1, or none?

This should probably instead be an array (not sure where the form is, didn't feel like digging) but something like this in the markup:

<input type="text" name="line[]" ...>
<!-- include some sort of javascript 'add another line' button. -->

Then in the controller you can just loop through $request->input('line') which would be an array.

Also you'll almost certainly want to provide translations for error messages, and other user feedback, and possible route names as well. Not everyone understands .. what is it, Lithuanian?

Raimondas's avatar

@willvincent Thanks for response. I'll try to upload English version repository also! Yeah, it's Lithuanian

GwynBleidd's avatar
 if(isset($p['line1'])) {
                $order_products->line1 = $p['line1'];
            } else{
                $order_products->line1 = '';
            }
            if(isset($p['line2'])) {
                $order_products->line2 = $p['line2'];
            }else{
                $order_products->line2 = '';
            }
            if(isset($p['line3'])) {
                $order_products->line3 = $p['line3'];
            }else{
                $order_products->line3 = '';
            }
            if(isset($p['line4'])) {
                $order_products->line4 = $p['line4'];
            }else{
                $order_products->line4 = '';
            }
            if(isset($p['line5'])) {
                $order_products->line5 = $p['line5'];
            }else{
                $order_products->line5 = '';
            }
            if(isset($p['line6'])) {
                $order_products->line6 = $p['line6'];
            }else{
                $order_products->line6 = '';
            }
            if(isset($p['line7'])) {
                $order_products->line7 = $p['line7'];
            }else{
                $order_products->line7 = '';
            }
            if(isset($p['line8'])) {
                $order_products->line8 = $p['line8'];
            }else{
                $order_products->line8 = '';
            }
            if(isset($p['line9'])) {
                $order_products->line9 = $p['line9'];
            }else{
                $order_products->line9 = '';
            }
            if(isset($p['line10'])) {
                $order_products->line10 = $p['line10'];
            }else{
                $order_products->line10 = '';
            }

Aside from how ineffective that is as pointed out above, even that logic could be transformed into just simple:

        for ($i = 1; $i <= 10; $i++) {
            $order_products->{'line' . $i} = (isset($p['line' . $i])) ? $p['line' . $i] : '';
        }

or even better with PHP7:

        for ($i = 1; $i <= 10; $i++) {
            $order_products->{'line' . $i} = $p['line' . $i] ?? '';
        }
Raimondas's avatar

@GwynBleidd Hey I exchanged that IF statement with your FOR loop and it works fine. Can u check if I included it correctly ? I've updated my CartController :)

GwynBleidd's avatar

@Raimondas Well yes you did, although I just pointed that as an example of code optimization (that is something that you never stop learning I suppose). Obviously I won't look through the whole codebase (nor do I consider myself 'pro' enough to do that), but these examples are what you can base your further optimizations off...

Right off the bat, one more instance in the same fashion, instead of:

        $product['line1'] = $request->input('line1');
        $product['line2'] = $request->input('line2');
        $product['line3'] = $request->input('line3');
        $product['line4'] = $request->input('line4');
        $product['line5'] = $request->input('line5');
        $product['line6'] = $request->input('line6');
        $product['line7'] = $request->input('line7');
        $product['line8'] = $request->input('line8');
        $product['line9'] = $request->input('line9');
        $product['line10'] = $request->input('line10');

you can do

        for ($i = 1; $i <= 10; $i++) {
            $product['line' . $i] = $request->input('line' . $i);
        }

Althought I'd rather remake it as per @willvincent 's comment.

This:

        $order->name = $request->input('name');
        $order->lastname = $request->input('lastname');
        $order->email = $request->input('email');
        $order->phone = $request->input('phone');
        $order->post_code = $request->input('post_code');
        $order->street = $request->input('street');
        $order->flat = $request->input('flat');
        $order->house = $request->input('house');
        $order->city = $request->input('city');
        $order->district = $request->input('district');
        $order->comment = $request->input('comment');

I'd remake to this:

$props = ['name', 'lastname', 'email', 'phone', 'post_code', 'street', 'flat', 'house', 'city', 'district', 'comment'];
foreach ($props as $prop) {
    $order->{$prop} = $request->input($prop);
}

And so on, basically, anywhere you can notice a repeatable pattern of code, it likely can be shortened and beautified somehow.

Also, I have no idea what 1.44 is:

$subtotal += 1.44;

but that certainly seems to me like not a good idea to hardcode such values into controller; I'd rather store it somewhere and get it where you need it along the lines of

$addedValue = Config::get('settings.addedValue');
$subtotal += $addedValue;
Raimondas's avatar

@GwynBleidd Thank you! Everything works, never thought about these approaches. Talking about 1.44 it's the price that is paid for shipping.

How to get that value from "settings.addedValue" ?

$addedValue = Config::get('settings.addedValue');

I've never used this, can you point me to some documentation or explain ? Thanks

bashy's avatar

@GwynBleidd Easier to just use config() so you don't need import and it's faster to type :P

1 like

Please or to participate in this conversation.