chore: refactor server code #9
Conversation
|
@swarit-pandey thanks for this contribution, please resolve the conflicts once. |
@lucifercr07 Sure, will get it done by EOD today. |
config/config.go
Outdated
| @@ -14,7 +14,7 @@ type Config struct { | |||
| } | |||
|
|
|||
| // LoadConfig loads the application configuration from environment variables or defaults | |||
There was a problem hiding this comment.
fn name is self explanatory we can remove or update the comment as required
| // LoadConfig loads the application configuration from environment variables or defaults | |
| // loads the application configuration from environment variables or defaults |
There was a problem hiding this comment.
In Go, if a function is public then it should have a comment starting with its name.
Signed-off-by: Swarit Pandey <[email protected]>
Signed-off-by: Swarit Pandey <[email protected]>
Signed-off-by: Swarit Pandey <[email protected]>
| // ExecuteCommand executes a command based on the input | ||
| func (db *DiceDB) ExecuteCommand(command *cmds.CommandRequest) (interface{}, error) { | ||
| switch command.Cmd { | ||
| case "GET": |
There was a problem hiding this comment.
Please match the behaviour of current changes similar to this.
| @@ -4,3 +4,9 @@ type CommandRequest struct { | |||
| Cmd string `json:"cmd"` | |||
| Args []string | |||
| } | |||
|
|
|||
| type CommandRequestArgs struct { | |||
| @@ -0,0 +1,3 @@ | |||
| package service | |||
|
|
|||
| // TODO: Implement me | |||
| @@ -0,0 +1,14 @@ | |||
| package repository | |||
|
|
|||
| type GetRequest struct { | |||
There was a problem hiding this comment.
We need to keep this generic, not feasible to be done for multiple commands getting generated.
|
Closing the PR, since inactive from sometime. |
|
Hey folks, sorry for not being able to continue working on this. I got occupied with other work related stuff, thanks for taking time to review. |
Purpose: The current code can be potentially improved to use standard REST based structure used in Go projects. Currently there is no scope of dependency injection, seperation of concerned layers which could potentially create issues going ahead (as we add more routes and businees logic).
This PR overhauls a lot of code that was implemented previously (excpet core logic). The new changes introduces:
Potential improvements:
Caveats:
This is not to undermine the work previous contributors did, if the reviewers/maintainers think that this change is 'very drastic', feel free to close the PR. But since a lot of service layer code is still to be implemented we can potentially improve the code.
Thanks.