maintainability

[xTurinx]

Hi,

at first thank you for making an API for TC.
After I've looked at the code, I saw that you wrote the most of the application logic into routes.php.

At the moment the routes.php has about 1700 lines of code. In my opinion this is bad for the maintainability.

Do you have a specific reason why you did this so?

Would you merge a pull request if I change the structure into multiple controllers?

[FrancescoBorzi]

Hello! The only reason is that this is my first experience with Laravel and I've just started adding contents to routes.php, but I've always kept in mind that I have to split the structure in multiple controllers.... sooner or later!

So if you want to do it, that would be great!

[xTurinx]

Alright. I'll start next weekend :)

[FrancescoBorzi]

@xTurinx any news? how is it going?

[avengerweb]

mhm... project is alive 👍

[FrancescoBorzi]

@avengerweb may you help with this?

[xTurinx]

Oh sorry guys I forgot you :/
Got a lot of work in the last weeks.

@avengerweb are you doing this now? If yes I would continue https://github.com/xTurinx/TC-PHP-Framework ^^

Otherwise I would start tomorrow

[xTurinx]

@avengerweb I've seen your commit 9056188

In general your work is good. In my opinion the restructuring of the API will be a lot of work but in the end it will be worth it.

If you want I will help you with this. But if we do this together we will have to discuss how to realize it.

At first we need consistent routenames, namespace, classnames and methodnames. This is the most important part. In the example I've commited ( xTurinx@c66b38d ) I've defined two major methods:

1. find => the find method gets a search pattern and returns all results containing this pattern
2. get => the get method gets the primary key of an entity and returns a single row

With these methods we can provide search functionality and the possibility to get a specific entity.

Our first step on implementing the new structure could be to create for each entity a controller class which contains the methods find and get (maybe some others). Then we just copy the code of the anonymous functions, written in the routes.php, into these controller methods.

Just let me know if you want to do this with me together.

[avengerweb]

@xTurinx, isn`t laravel style. In ur structure u have 100500+ controllers.

[FrancescoBorzi]

Please avoid renaming routes in order to not break compatibility with those
applications which are already using the API

On Fri, 1 Apr 2016, 00:27 Vadim, notifications@github.com wrote:

@xTurinx https://github.com/xTurinx, isn`t laravel style. In ur
structure u have 100500+ controllers.

—
You are receiving this because you commented.

Reply to this email directly or view it on GitHub
#11 (comment)

[avengerweb]

First, old can be optimized and rewrtied with using laravel things.
Second, we need make it with right structure, for support automatic API documentation generation
Third, old code has some place where we can do SQL injections, that one of the first reasons for rewrite old and not save it.

About structure: https://laravel.com/docs/5.2/eloquent - one table - one model (Entity) in next that can be easy adding relations with locales for example.

If all going good for search we can use something like elasticsearch.

[FrancescoBorzi]

👍

[xTurinx]

@xTurinx, isn`t laravel style. In ur structure u have 100500+ controllers.

No we would have controllers as much we have entities in the databases.

Next question is if we should realize a restful API.
On the main README.md we can see, that this WebAPI should be a restful implementation, but it hasn't implemented http request methods like patch or delete ( https://en.wikipedia.org/wiki/Representational_state_transfer#Applied_to_web_services )

Should we change the TC-JSON-API to restful?

In general yes we have to use the Laravel ORM to avoid security issues. Building queries is too unsecure.

We also have to create for each entity a controller (Item is an entity, Achievement is an entity etc).

Then we should provide a consistent structure for routing. To avoid compatibility issues we can also implement the old routes.

In my example I did it like this:

https://github.com/xTurinx/TC-JSON-API/blob/master/app/Http/routes.php << this are the new routes
https://github.com/xTurinx/TC-JSON-API/blob/master/app/Http/deprecatedRoutes.php << this are the deprecated routes

[avengerweb]

Current project is not RESTfull, currently that something like database viewer.
But ur structure not RESTfull too.

[xTurinx]

I haven't said, that my structure is restful.