3 ms·

Some issues with this project: - non static and non namespaced global variable EMOJIS (should be inside the emojize method or at least in the namespace) - lac

by speps 8y ago

Some issues with this project: - non static and non namespaced global variable EMOJIS (should be inside the emojize method or at least in the namespace) - lack of STL use, that std::string replacing code doesn't look very "modern" at all - complete lack of unit tests - README has unsupported emojis under the "supported" header

delvincasper 8y ago
Thanks for the feedback. I will do improvements
mbel 8y ago
> - non static and non namespaced global variable EMOJIS (should be inside the emojize method or at least in the namespace) The implementation honestly should be put in .cpp file, not a header. Currently it will define both the gigantic statically-initialized map and a function in each module where it's used. Compared to consequences of this lack of unit tests looks like nit-picking. And yeah, the logic of the function is easy to follow, so it's definitely not modern C++.
- speps 8y ago
  > And yeah, the logic of the function is easy to follow, so it's definitely not modern C++. It isn't easy to follow at all, count how many "+1" had to be added just to get indices working right, it's a mine field of off by one errors.
  - huhtenberg 8y ago
    > It isn't easy to follow at all You can't be serious.
    - speps 8y ago
      You just commented a suggested improvement which proves that it's not easy to follow code. Your suggestion is very good, you still had to process the previous code to understand and it wasn't clear at first.
userbinator 8y ago
doesn't look very "modern" at all This is the sort of thing that turns me off C++ completely. It used to be a pragmatic extension to C, now it's about trend-chasing. The code could be simpler (the extra "index" which is essentially a state variable is odd), but IMHO it is otherwise perfectly fine. Useless superficial criticism like this is just another form of cargo-culting.
- jcelerier 8y ago
  > It used to be a pragmatic extension to C, now it's about trend-chasing. no, it's about maximizing performance, because C++ libs are what countless software build upon. Maybe someone will want to use this to display a 4*4k mural screen with 2000000 of those displayed and changing in real-time. Just using a <map> instead of a hash-container (there's a compile-time one just on top of HN right now which would fit the bill perfectly since these are known at compile time) is already a fairly big smell.
yarosv 8y ago
I'm not C++ developer, but isn't str::string STandard Library?

by speps 8y ago

delvincasper 8y ago

Thanks for the feedback. I will do improvements

mbel 8y ago

> - non static and non namespaced global variable EMOJIS (should be inside the emojize method or at least in the namespace) The implementation honestly should be put in .cpp file, not a header. Currently it will define both the gigantic statically-initialized map and a function in each module where it's used. Compared to consequences of this lack of unit tests looks like nit-picking. And yeah, the logic of the function is easy to follow, so it's definitely not modern C++.

speps 8y ago
> And yeah, the logic of the function is easy to follow, so it's definitely not modern C++. It isn't easy to follow at all, count how many "+1" had to be added just to get indices working right, it's a mine field of off by one errors.
- huhtenberg 8y ago
  > It isn't easy to follow at all You can't be serious.
  - speps 8y ago
    You just commented a suggested improvement which proves that it's not easy to follow code. Your suggestion is very good, you still had to process the previous code to understand and it wasn't clear at first.

userbinator 8y ago

doesn't look very "modern" at all This is the sort of thing that turns me off C++ completely. It used to be a pragmatic extension to C, now it's about trend-chasing. The code could be simpler (the extra "index" which is essentially a state variable is odd), but IMHO it is otherwise perfectly fine. Useless superficial criticism like this is just another form of cargo-culting.

jcelerier 8y ago
> It used to be a pragmatic extension to C, now it's about trend-chasing. no, it's about maximizing performance, because C++ libs are what countless software build upon. Maybe someone will want to use this to display a 4*4k mural screen with 2000000 of those displayed and changing in real-time. Just using a <map> instead of a hash-container (there's a compile-time one just on top of HN right now which would fit the bill perfectly since these are known at compile time) is already a fairly big smell.

yarosv 8y ago

I'm not C++ developer, but isn't str::string STandard Library?