[meheleventyone]

The example you post is out of context and maybe a bit overly verbose but again it’s one guy maintaining it and is perfectly readable. Does anyone really struggle to understand that?

I’m back working on a twenty plus year old codebase (and game) that’s ~3 millions loc and would kill for something simple like that rather than some of the overabstracted crimes lurking within.

[npteljes]

The problem is not with understanding, but with maintainability. If you need, for any reason, to touch that code again, instead of a few changes, you have to be consistently doing a lot of changes. This has a higher chance of manual errors. And it has nothing to do with other bad code, like the one that you encounter. Both can be bad code, and what you inherited sounds worse. I hate it when the badness is compounded with complexity. It feels pretentious to me.

[meheleventyone]

That code is a few lines maintaining a mapping though, changing it is not going to be hard and all the mappings are captured explicitly which makes it even more straightforward versus shaving off lines of code by making some of them implicit (e.g. by seeing a pattern in the mapping and using an if statement to capture them all). I'd really love to know what you think it should be to be more maintainable!

[npteljes]

Something like this would pass a code review, while the original would not:

  function createObject(value, id, nominal)
      return {
          value = value,
          id = id,
          nominal = nominal
      }
  end
  
  local objects = {
      createObject(100, 1, "nominal1"),
      createObject(200, 2, "nominal2"),
      createObject(300, 3, "nominal3"),
      createObject(400, 4, "nominal4")
  }

Still far from ideal, but I'm not that familiar with LUA either. And I do acknowledge that bad code can work, and heck, even I write them sometimes. My original point is that it has downsides and risks, and so I don't consider it fine. I have dreaded to touch such code (mine and others) many times, and made mistakes because of past laziness like that, many times.

[meheleventyone]

This is not equivalent though so you fail my code review. :D

For starters its clear the code in the example expects self.base.value to already hold a value. So constructing an object blind isn't necessarily what's happening here, it's possible an existing object is even being decorated with extra fields. So your suggestion isn't preserving the semantics.

Secondly for the face cards there is an additional field being set which is not happening in your case.

That's what I mean when I said earlier that we can't just look at a piece of code contextless and say it's bad. Your refactor is definitely neater if we're initializing homogenous objects but that's an incorrect assumption on your part. It also comes with trade-offs for example it's not obvious anymore what any of the values relate to without going to the function constructing the object. Not the biggest thing in the world but if this is the only place in the codebase doing this then I'd argue that it probably isn't worth it.

[AnIrishDuck]

Yeah, without context criticism of this code is silly.

What if this wasn't always how cards worked? What if the mapping was (at one point in development) not straightforward? Or localthunk had some ideas and wanted to leave the door open to a more complex mapping scheme?

[npteljes]

Either silly, or 20 years in the business.