3
\$\begingroup\$

I have a simple task.

  1. Array of people object comes in, up to a thousand.
  2. Need to iterate over it, and create a custom payload for external service.

I have created a working solution, but I would like to see if I am totally off course, and what changes should I make.

I have reduced non-important details from the code (errors, and similar), so it might not work. But the point is to show how it works, and hopefully get some advice.

User struct {
   Name        string  `json:"name"`
   Lastname    string  `json:"lastname"`
 }

func HandleLambda(event events.SQSEvent) {
 var userRecords []map[string]interface{}

 for idx := range event.Records {
  record := event.Records[idx]
  ev, err := e.NewEvent(record.Body)
  
  if err != nil {
    continue
  }

  u, err := repository.NewUser(ev)

  if err != nil {
        continue
  }

  user := make(map[string]interface{})
  user["1"] = u.Name
  user["2"] = u.Lastname

  userRecords = append(userRecords, user)
  }

 uJSON, _ := json.Marshal(userRecords)
 // I send this to external service

 }

External service is using numbers as keys, that's why I need "1" and "2". I do not have control over it. That's the way it works.

\$\endgroup\$
2
  • \$\begingroup\$ The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?. \$\endgroup\$ – BCdotWEB Feb 25 at 11:23
  • 2
    \$\begingroup\$ Welcome to Code Review! You mentioned you removed non-important details, but it would actually be really good to see the fully, working code, you might never know what review comments you get about those non-essentials. Enjoy your stay! \$\endgroup\$ – ferada Feb 25 at 12:49
2
\$\begingroup\$

I guess I might as well then. To prefix that with: It looks fine as is, there's a few details that could be improved, although those also depend on your team and style guide. Also read it in the context of a bigger project, not everything always applies, especially if it's one-off code, etc.

I'm gonna assume (since the formatting is a bit off) that you're using gofmt or goimports already, if you don't, check them out, it helps immensely.

For the User struct, even if the JSON name is lastname, consider the Go-side name to be LastName to keep in line with normal style guides. Depending on whether you need it the JSON annotation could also use an omitempty, but that depends on your external service of course.

For the HandleLambda loop a few things stand out:

  • for idx := range event.Records might as well be for idx, record := event.Records, no need have an extra line to get the actual item. And if you're never using it, for _, record := event.Records even.
  • Errors are ignored? Consider logging them if don't error our immediately.
  • user := make(map ...) I'd usually make that a literal to avoid repeating myself even more.
  • userRecords could perhaps already be pre-allocated to the right size. Then there'd be no need for append at all. Or, like here, if you still need to use continue, pre-allocating the maximum length it could take, so it definitely wouldn't have to be resized might also be a good idea. Usually this also doesn't matter that much if you don't have a lot of entries, but it's good to keep in mind for situations where it does matter.
  • Also consider making the to-map conversion a new method on User, or a helper function, you might want to test it or reuse in other places.
  • repository looks like a global variable, that's usually not the best idea. Consider making that a parameter for HandleLambda, or making HandleLambda a method and put the repository as one of its members. It will help with testability in the end to not have dependencies scattered around like that.
type User struct {
    Name     string `json:"name"`
    LastName string `json:"lastname"`
}

func (u *User) toMap() map[string]interface{} {
    return map[string]interface{}{
        "1": u.Name,
        "2": u.LastName,
    }
}

func HandleLambda(event events.SQSEvent) {
    userRecords := make([]map[string]interface{}, 0, len(event.Records))

    for _, record := range event.Records {
        ev, err := event.NewEvent(record.Body)
        if err != nil {
            continue
        }

        user, err := repository.NewUser(ev)
        if err != nil {
            continue
        }

        userRecords = append(userRecords, user.toMap())
    }

    uJSON, _ := json.Marshal(userRecords)
    // I send this to external service
}
\$\endgroup\$
8
  • 2
    \$\begingroup\$ Awesome. I learned new things from your answer! Thank you. \$\endgroup\$ – Wexoni Feb 26 at 10:54
  • 1
    \$\begingroup\$ I have one question. You mentioned: "userRecords could already be pre-allocated to the right size, then there'd be no need for append,". But append is still used? \$\endgroup\$ – Wexoni Feb 26 at 19:28
  • 1
    \$\begingroup\$ Not many errors will be skipped. This comes form AWS SQS. I think I will keep only one error check, the first one that checks the record.body, Second one is my mistake and it should not be there at all. \$\endgroup\$ – Wexoni Feb 28 at 7:05
  • 1
    \$\begingroup\$ On a side note, when you declared that slice. I was able to find and read about it. But that zero is confusing me. What is it? \$\endgroup\$ – Wexoni Feb 28 at 7:57
  • 1
    \$\begingroup\$ I tried to implement the suggestions, but I run in a problem. The to Map() function, in my IDE (Goland) it has many issues, it seems it is a Syntax Error that creates them all. I tried, but I cant fix it. Do you maybe have an idea? \$\endgroup\$ – Wexoni Mar 1 at 8:53

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.