Refactoring a Laravel application layer

My Laravel project starts to grow and I start to deal with fat controllers and models. I am probably off the path of SOLID principles and my code is not dry.

I decided to redesign my layer logic to follow more SOLID and try to be drier. After doing some research on the Internet, I came to the following conclusion:

Enter the image description here

Let me explain this diagram:

  1. First we start with a view in which the user takes an action and sends the request to the controller.
  2. The responsibility of the controller is to process the request and give the user the answer. 3 different levels (blue numbers) can be called up:
    • service: to handle business logic such as calculations, special promotions, etc.
    • Repository: where all the query logic is placed. For example, if you want to use the index method to return the user list with users who have more than 100 posts and are sorted by name (example 1).
    • Laravel resource (transformers): with the responsibility to convert a model to JSON. When we change our table, we don't have to change all the views and controllers or models that are affected by this change. Everything is done in one place.

Example 1:

 # UserController.php
 public function index()
 {
     $users = new UserCollection($this->UserRep->usersWithPost());
     return view('user-list', compact('users'));
 }

 # UserRepository.php
 public function usersWithPost($min = 100)
 {
     return $this->model->where('post_count', '>=', $min)->orderBy('name');
 }

 # UserResource.php
 public function toArray($request)
 {
     return (
         'id' => $this->id,
         'name' => $this->name,
         'email' => $this->email,
         'post_count' => $this->post_count,
         'created_at' => $this->created_at,
         'updated_at' => $this->updated_at,
     );
 }

  1. Service calls (green numbers):
    • It can call the repository when data from my model is needed to perform an action.
    • It can also call Laravel Resource "Transformers" if there were repository calls.
  2. The repository uses the eloquent model to continue the query in my data store (MySQL).

In this way, I plan to redesign my code. However, I have no experience with Laravel and would like to ask more experienced developers if they can tell me if I'm on the right track.

Do you think it makes sense and is good practice?

I would like to emphasize that I will not switch between ORMs. I will use eloquently with MySQL as a data store. So I plan to put all of my queries in repositories to have a different level of query logic.

Python – refactoring multiple "else if" in one parser

I worked on a shop receipt parser that extracts payment data. Here is the text I'm analyzing:

  * Vic107Payment 
    Text: PINNEN
    TicketData: POI: 12345678                   
KLANTTICKET
--------------------------------
Terminal:                 ABC123
Merchant:                 123456
Periode:                    1234
Transactie:             12345678


MAESTRO         
(A0000000012345)                
MAESTRO             
Kaart:       xxxxxxxxxxxxxxx1234
Kaartserienummer:              0

BETALING            
Datum:          01/01/2020 04:15
Autorisatiecode:          123ABC

Totaal:                 1,00 EUR

Leesmethode: NFC Chip           

    CardTypeId: 1234
    CardTypeText: MAESTRO
    ReceiptNumber: 
    DrawerAmount: 1,00
    Number: 1
    DrawerId: drawers/default
    DrawerNumber: 1
    Amount: 1,00
    IsCancelable: True
===

Here is the code that analyzes this fragment from the receipt. Now I'm worried that people will have trouble reading and / or caring for it, so I was wondering if it could be improved in any way.

def parse_card_payment(product):
    # cp stands for card payment
    cp_poi = None
    cp_terminal = None
    cp_merchant = None
    cp_period = None
    cp_transaction = None
    cp_card = None
    cp_card_serial_number = None
    cp_date = None
    cp_authorisation_code = None
    cp_total = None
    cp_card_type_id = None
    cp_card_type_text = None
    cp_drawer_id = None
    cp_drawer_amount = None
    cp_cancelable = None
    cp_card_type = None

    for line in product.strip().split('n'):
        if 'POI' in line:
            cp_poi = line.split(':')(1).strip()
        elif 'Terminal' in line:
            cp_terminal = line.split(':')(1).strip()
        elif 'Merchant' in line:
            cp_merchant = line.split(':')(1).strip()
        elif 'Periode' in line:
            cp_period = line.split(':')(1).strip()
        elif 'Transactie' in line:
            cp_transaction = line.split(':')(1).strip()
        elif 'Kaart:' in line:
            cp_card = line.split(':')(1).strip()
        elif 'Kaartserienummer' in line:
            cp_card_serial_number = line.split(':')(1).strip()
        elif 'Datum' in line:
            cp_date = line.split(':')(1).strip()
        elif 'Autorisatiecode' in line:
            cp_authorisation_code = line.split(':')(1).strip()
        elif 'Totaal' in line:
            cp_total = line.split(':')(1).strip()
        elif 'CardTypeId' in line:
            cp_card_type_id = line.split(':')(1).strip()
        elif 'CardTypeText' in line:
            cp_card_type_text = line.split(':')(1).strip()
        elif 'DrawerAmount' in line:
            cp_drawer_amount = line.split(':')(1).strip()
        elif 'DrawerId' in line:
            cp_drawer_id = line.split(':')(1).strip()
        elif 'Cancelable' in line:
            cp_cancelable = line.split(':')(1).strip()
        elif 'Leesmethode' in line:
            cp_card_type = line.split(':')(1).strip()

    cp = {'cp_poi': cp_poi,
          'cp_terminal': cp_terminal,
          'cp_merchant': cp_merchant,
          'cp_period': cp_period,
          'cp_transaction': cp_transaction,
          'cp_card': cp_card,
          'cp_card_serial_number': cp_card_serial_number,
          'cp_date': cp_date,
          'cp_authorisation_code': cp_authorisation_code,
          'cp_total': cp_total,
          'cp_card_type_id': cp_card_type_id,
          'cp_card_type_text': cp_card_type_text,
          'cp_drawer_id': cp_drawer_id,
          'cp_drawer_amount': cp_drawer_amount,
          'cp_cancelable': cp_cancelable,
          'cp_card_type': cp_card_type}
    return cp

Refactoring – Use timestamps instead of version numbers

This actually refers to the query published here
Date as software version number

A good example of this:

https://github.com/LydiaMarieWilliamson/ALGLIB_cpp

We have completely abolished the version numbering and are simply using time stamps – except for the second; especially since the revision and test process itself is largely automated. You will see the end of the sequence at the end of the supplementary notes (Notes.htm).

Over time, we anticipate the likelihood that if the level of automation expands to cover more software developer activities, the revisions will be published in seconds in real time and a resolution of milliseconds may be required. Tens of thousands and even hundreds of thousands of rows are changed with every change. With other software, this can potentially lead to millions and more as we adopt larger and larger code bases.

dry – what do you do if refactoring results in more copies being inserted?

For example, I converted several raw functions for each document type / table into a generic class for database access, which used a model class as an argument.

However, I inserted this generic class several times with the same argument. it is impossible to dry further.

ps, at least the code base is smaller and more focused!

Beginners – Refactoring code for Kotlin

Thank you for your response to Code Review Stack Exchange!

  • Please be sure to answer the question, Provide details and share your research!

But avoid

  • Ask for help, clarify, or respond to other answers.
  • Make statements based on opinions; Provide them with references or personal experience.

Use MathJax to format equations. Mathjax reference.

For more information, see our tips on writing great answers.

refactoring – use "magic" strings due to one-time usage error?

If you declare it separately, you can reference it in unit (or integration) tests. If you end up having to change the contents of the string, you only have to change it once (where it is defined) instead of making the same change in the other instances of the string.

Centralization can have other advantages. For example, if you need to add localization (versions of the user-related content translated into other languages), it is much easier to pass a few files of all strings to a translator, rather than an ad hoc accumulation of points throughout the code.

If you don't write a test code, you probably want to investigate – this is generally considered best practice. If it's a quick personal project, it's okay to choose the acronym for hardcoding – but it's definitely not something that is general recommended,

Refactoring multiple for loops in the same list using different methods

It is not inherently a code smell

There is nothing wrong with having one foreach in several methods, unless You always execute these three methods one after the other. At this point, you can simplify it to accomplish the following:

public void xyz()
{
    foreach(a in alist)
    {
        x(a);
        y(a);
        z(a);
    }  
}

However, if you do not always invoke these methods in the same order (which is probably the case), this does not apply.

I feel like walking through the same object with different methods is a code smell

If you calculated a value based on the same input parameters, I would agree. However, this is not the case here. A separate enumerator is required for each method in order to carry out its own enumeration.

What if you change the name of the alist variable in the future? I would have to change at least 3 positions.

This applies to all variables, methods, class names or namespaces that you would ever use. It would literally make you unable to write any Code that supposedly doesn't smell.

With the correct IDE (or extension), this is not a problem because you can redesign names, which will change all references to that name throughout the code base.

In the case of Visual Studio, press F2 while the cursor is on the name (variable / method / class / …).

Is there any way to extract the foreach to a common location and get the same output you want?

Not that sensible. I mean you could abstract it, but it would add complexity without actually improving performance or maintainability. This would affect readability, not only because complexity increases, but also because you are changing a familiar concept to a homebrew concept that requires additional knowledge that a developer must adhere to when reading the code.

Several disadvantages, no advantages. No need to do this.


The only sensible case for a code smell would not be the enumeration itself, but a preliminary filter logic. For example:

public string x ()
{ 
    foreach(a in alist.Where(a => a.Foo == "Bar"))
    {
        //do something
    }
    return string;
}

// And the same Where() for y() and z()

The filter logic itself can be abstracted here to avoid unnecessary repetitions:

private IEnumerable aBarList => alist.Where(a => a.Foo == "Bar");

public string x ()
{ 
    foreach(a in aBarList)
    {
        //do something
    }
    return string;
}

// And the same Where() for y() and z()

This is just one of many possible implementations. Your vague sample code makes it impossible to judge the best implementation for your current case.

refactoring – Refactor of the method that has the order of the similar looking blocks of code towards (or towards) the design pattern (the design pattern).

I need some help to understand whether the following code can be revised to something less complicated, less repetitive and more in the direction of a suitable pattern.

What I find uncomfortable in the code is the flow of repetitive tasks with the same pattern as:

// Get the result from some operation (API call / or any other operation);
// Check if the result is somehow valid;
// If it is not valid, set the response object accordingly and return early;
// If it is valid, continue with the next step with the overall same logic but different details.

Does the code seem to be able to be revised into (or towards) a design pattern that can be used here?

I would be pleased to receive feedback.

Here is the code:

/**
 * Check if the given email exists in the SendGrid recipients global list
 * and its custom field 'status' has the value 'subscribed'.
 *
 * @param  string  $email The email to check.
 *
 * @return object  (object)('isfound'=>false, 'issubscribed'=>false);
 */
public function getSubscriberStatus(string $email): object
{
    $result = (object) ('isfound' => null, 'issubscribed' => null);

    /**
     * Find the email in the SendGrid global list.
     */
    $endpoint = "contactdb/recipients/search?email=$email";
    $found = $this->callSendGrid('GET', $endpoint);
    if ($found->status !== 200) {
        Log::error(sprintf('(SENDGRID) Error while searching the email: %s in the SendGrid list, status: %s, message: %s', $email, $found->status, $found->message));
        $result->isfound = false;
        $result->issubscribed = false;
        return $result;
    }

    if (!($found->data->recipient_count > 0)) {
        $result->isfound = false;
        $result->issubscribed = false;
        return $result;
    }

    /**
     * Find the recipient with email exactly matching the required one.
     */
    $recipient = collect($found->data->recipients)->first(function ($item) use ($email) {
        return $item->email === $email;
    });

    /**
     * No exactly matching emails.
     */
    if (!$recipient) {
        $result->isfound = false;
        $result->issubscribed = false;
        return $result;
    }

    $result->isfound = true;

    /**
     * Get the status field of the recipient's 'custom_fields' array.
     */
    $status = collect($recipient->custom_fields)->first(function ($item) {
        return $item->name === 'status';
    });

    if ($status->value !== 'subscribed') {
        $result->issubscribed = false;
        return $result;
    }

    $result->issubscribed = true;
    return $result;
}

Refactoring – gateway aggregation only for microservices?

I started working in an existing Angular app that uses a monolithic web API written with .NET CORE. This API has dozens of controllers (routes).

When a particular event occurs on one of the main screens, dozens of information loading requests may be made for that screen. Each request is addressed to different controllers (customer, contract, configurations, etc.). Some requests are nested. Angular receives an ID and sends another request that passes this ID. The performance is poor.

I thought it would be possible to get most of the data in a single request if a kind of gateway was created for the aggregation.

All the articles I found on the Internet are about microservices. I am not planning to use microservices in this project, nor am I planning to scale it. I just want to redesign it to make fewer requests. Example for an article: https://docs.microsoft.com/en-us/azure/architecture/patterns/gateway-aggregation

I could create a controller that would include all the services / repositories that retrieve data for this front-end screen, although the existing GET methods would no longer have to be present in the other controllers.

I could also send HTTP calls to other controllers within this new controller to aggregate data. In this case the request is for localhost because it is the same application / server.

What approach or technology would I use in this situation?

Java – Business Logic code refactoring required

I try to transform PlaylistBusinessBean Class. The goal of this class is to add a song / track at an index to an existing playlist.

After my understanding of the method validateIndexes in the class PlaylistBusinessBean has no meaning there validateIndexes I will never return falsebecause we already update that toIndex Value before the call validateIndexes Method. So my question is that the following lines can be removed from the class because they are unnecessary. Is there anything else that can be done to redesign the class?

                    if (!validateIndexes(toIndex, playList.getNrOfTracks())) {
                        return Collections.EMPTY_LIST;
                    }

Business Class:

public class PlaylistBusinessBean {

    private PlaylistDaoBean playlistDaoBean;

    @Inject
    public PlaylistBusinessBean(PlaylistDaoBean playlistDaoBean) {
        this.playlistDaoBean = playlistDaoBean;
    }

    List addTracks(String uuid, List tracksToAdd, int toIndex) throws PlaylistException {

        if (StringUtils.isNotBlank(uuid) && CollectionUtils.isNotEmpty(tracksToAdd)) {

            try {

                Optional playlistByUUID = Optional.ofNullable(playlistDaoBean.getPlaylistByUUID(uuid));

                if (playlistByUUID.isPresent()) {

                    PlayList playList = playlistByUUID.get();

                    // We do not allow > 500 tracks in new playlists
                    if (playList.getNrOfTracks() + tracksToAdd.size() > 500) {
                        throw new PlaylistException("Playlist cannot have more than " + 500 + " tracks");
                    }

                    // The index is out of bounds, put it in the end of the list.
                    int size = playList.getPlayListTracks() == null ? 0 : playList.getPlayListTracks().size();
                    if (toIndex > size || toIndex == -1) {
                        toIndex = size;
                    }

                    if (!validateIndexes(toIndex, playList.getNrOfTracks())) {
                        return Collections.EMPTY_LIST;
                    }

                    Set originalSet = playList.getPlayListTracks();
                    List original;

                    if (originalSet == null || originalSet.size() == 0)
                        original = new ArrayList();
                    else
                        original = new ArrayList(originalSet);

                    Collections.sort(original);

                    List added = new ArrayList(tracksToAdd.size());

                    for (Track track : tracksToAdd) {
                        PlayListTrack playlistTrack = new PlayListTrack();
                        playlistTrack.setTrack(track);
                        playlistTrack.setTrackPlaylist(playList);
                        playlistTrack.setDateAdded(new Date());
                        playlistTrack.setTrack(track);
                        playList.setDuration(addTrackDurationToPlaylist(playList, track));
                        original.add(toIndex, playlistTrack);
                        added.add(playlistTrack);
                        toIndex++;
                    }

                    int i = 0;
                    for (PlayListTrack track : original) {
                        track.setIndex(i++);
                    }

                    playList.getPlayListTracks().clear();
                    playList.getPlayListTracks().addAll(original);
                    playList.setNrOfTracks(original.size());

                    return added;
                }
                throw new PlaylistException("Playlist with UUID: " + uuid + " not found");
            } catch (Exception e) {
                e.printStackTrace();
                throw new PlaylistException("Generic error");
            }
        } else {
            throw new PlaylistException("Bad input data error");
        }
    }

    private boolean validateIndexes(int toIndex, int length) {
        return toIndex >= 0 && toIndex <= length;
    }

    private float addTrackDurationToPlaylist(PlayList playList, Track track) {
        return (track != null ? track.getDuration() : 0)
                + (playList != null && playList.getDuration() != null ? playList.getDuration() : 0);
    }
}
```