std::vector for pods I am learning c++ and read why we can't use realloc() in std::vector, but i think we can use realloc() for pods, so here is my attempt to write std::vector like class template specialized for PODs

#include <malloc.h>
#include <stdexcept>
#include <type_traits>

namespace gupta {
namespace detail {
namespace podvector {
template <typename T> inline auto malloc(size_t elm_count) {
  return ::malloc(elm_count * sizeof(T));
}

template <typename T> inline auto realloc(void *old_block, size_t elm_count) {
  return ::realloc(old_block, elm_count * sizeof(T));
}
} // namespace podvector
} // namespace detail

template <typename PodType,
          typename = std::enable_if_t<std::is_pod<PodType>::value>>
class podvector {
public:
  using value_type = PodType;
  using size_type = size_t;
  using pointer = value_type *;
  using const_value_pointer = const value_type *;

  ~podvector() {
    if (m_memory)
      free(m_memory);
  }

  podvector(size_type initial_size = 0) {
    alloc(initial_size);
    m_size = initial_size;
  }

  podvector(size_type initial_size, const value_type &value)
      : podvector(initial_size) {
    for (auto &v : *this)
      v = value;
  }

  podvector(const podvector &other) : podvector(other.m_size) {
    for (size_type i = 0; i < m_size; i++)
      m_memory[i] = other.m_memory[i];
  }

  podvector(const podvector &&other)
      : m_memory{std::move(other.m_memory)},
        m_capacity{std::move(other.m_capacity)}, m_size{
                                                     std::move(other.m_size)} {
    // other.m_memory = nullptr;
    other.alloc(0);
  }

  podvector &operator=(const podvector &rhs) {
    if (this != &rhs) {
      resize(rhs.m_size);
      for (size_type i = 0; i < rhs.m_size; i++)
        m_memory[i] = rhs[i];
    }
    return *this;
  }

  podvector &operator=(podvector &&rhs) {
    if (this != &rhs) {
      this->~podvector();
      m_memory = std::move(rhs.m_memory);
      m_capacity = std::move(rhs.m_capacity);
      m_size = std::move(rhs.m_size);
      // rhs.m_memory = nullptr;
      rhs.alloc(0);
    }
    return *this;
  }

  void resize(size_type new_size) {
    if (new_size > m_size)
      change_capacity(new_size);
    m_size = new_size;
  }

  void reserve(size_type new_capacity) {
    if (m_capacity < new_capacity)
      change_capacity(new_capacity);
  }

  void push_back(const value_type &new_elm) {
    if (m_size + 1 > m_capacity)
      change_capacity(std::min(m_capacity * 2, 1));
    m_memory[m_size++] = new_elm;
  }

  void pop_back() { m_size--; }

  auto size() const { return m_size; }
  auto capacity() const { return m_capacity; }
  pointer begin() { return m_memory; }
  const_value_pointer begin() const { return m_memory; }
  pointer end() { return m_memory + m_size; }
  const_value_pointer end() const { return m_memory + m_size; }
  value_type &operator[](size_type pos) { return m_memory[pos]; }
  const value_type &operator[](size_type pos) const { return m_memory[pos]; }

private:
  pointer m_memory;
  size_type m_size, m_capacity;
  void alloc(size_type capacity) {
    m_capacity = capacity;
    m_size = 0;
    m_memory =
        static_cast<pointer>(detail::podvector::malloc<value_type>(capacity));
    if (!m_memory)
      throw std::bad_alloc{};
  }
  void change_capacity(size_type new_capacity) {
    m_capacity = new_capacity;
    void *new_memory =
        detail::podvector::realloc<value_type>(m_memory, new_capacity);
    if (!new_memory)
      throw std::bad_alloc{};
    m_memory = static_cast<pointer>(new_memory);
  }
};

} // namespace gupta

malloc and realloc live in cstdlib. malloc.h is not a standard header, neither in C++ nor in C, and should not be relied on in any case.
Also, you're missing #include <utility> for std::move.
Legacy C functionality, as everything else from the standard library, lives in the std namespace, if you include the corresponding <cheader> files. While you can also include the C standard headers via <header.h>, this is highly discouraged. With the first option, names may also exist in the global namespace, but this is not guaranteed and should not be relied on.
Your malloc and realloc functions need not be inline. In fact, inline does nothing here; remove it.
You don't need to check for null pointers when freeing memory. Passing a null pointer to free is always well defined and does nothing.
You don't need to move out all the data members from other in your move constructor. All of those members are trivial to copy, and have no special move behavior.
The move assignment operator invokes undefined behavior. You cannot just call the destructor of an object and then write to its members. Calling the destructor completely ends the lifetime of the object; afterwards, there's nothing there to write to. Furthermore, this is a kind of anti-pattern; in almost all cases, the move assignment operator should be implemented via the copy-and-swap idiom.
push_back is broken. You call change_capacity with the result of std::min(m_capacity * 2, 1), which is either 0 or 1, in any case. Most likely, you'd want to use std::max instead.
if (m_size + 1 > m_capacity) looks strange to my eyes. I would just write if (m_size >= m_capacity).
pop_back should definitely return a value. It's not very useful otherwise.
Since you're going for what seems to be a std-like container interface, you should add a little more functionality. For one, you're missing some of the usual type definitions, such as iterator and const_iterator. Furthermore, some sort of emplace mechanism would be appropriate, as well as some way to clear the container and some functionality to shrink it, as well as some other things.
You should make sure that memory is properly initialized. As a user, I would expect that it's safe to access every element in bounds; however, you are not initializing memory at all, leaving that job to the user. I'd expect memory to be default- or zero-initialized; making use of std::calloc should do the trick (alternatively, you can also just std::memset the area).
I would make your malloc and realloc functions a little bit more useful. As of right now, the onus of checking whether allocation succeeded is put on the user of those functions, but in my opinion it would be much cleaner and safer to lift those functions to a "C++-safe" level, i.e. make them check the return value of the underlying allocation call directly and convert that into an exception directly.
std::is_pod is being deprecated, in accordance with the standard library moving away from the term POD as a whole. Also, the requirement that T must be a POD type is too strict; your vector will still work correctly if T is trivial.


Singleton handler for reading and writing configuration.json In a current project of I have multiple modules that need to read from and write to a config.json file, where the settings are saved as nested dictionaries. I chose json here, because I want the config file to be human-readable.
Simplified example config.json:
{
  "server_data": {
    "email_address": "something@somewhere.com",
    "server_address": "imap.server.com",
    "server_port": 993
  },
  "handle_emails": {
    "output_directory": "C:\\Users\\riskypenguin\\some_directory",
    "mark_read": false,
    "write_to_file": true,
    "folders": ["inbox"]
  },
  "output": {
    "output_directory": "C:\\Users\\riskypenguin\\some_other_directory",
    "print_timeinfo": true,
    "print_form_counts": true
  }
}

My config_handler.py provides standardised access to the file via get_setting and set_setting. For different use cases throughout the project I need to be able to access the configuration settings at varying depths. For example: Some boolean settings are read and written directly, but the dict at server_data will be read and processed as a whole. Concurrency is not relevant for this project, so there are no collisions regarding read and write access.
config_handler.py
import os
import json
from functools import reduce

from definitions import CONFIG_DIR, CONFIG_FILE, DEFAULT_CONFIG_FILE

__all__ = ["get_setting", "set_setting"]

# Helper functions

def _save_config_data(content):
    if not os.path.exists(CONFIG_DIR):
        os.makedirs(CONFIG_DIR)

    with open(CONFIG_FILE, 'w') as f:
        json.dump(obj=content, fp=f, indent=2)

def _load_config_data():
    file = CONFIG_FILE if os.path.exists(CONFIG_FILE) else DEFAULT_CONFIG_FILE

    with open(file, 'r') as f:
        return json.load(f)

_config_data = _load_config_data()

# Exported functions

# Old version of get_setting, currently not in use
def get_setting_old(*args, default=None):
    current_value = _config_data

    for key in args:
        if isinstance(current_value, dict):
            current_value = current_value.get(key, default)
        else:
            return default

    return current_value

def get_setting(*config_keys, default=None):
    drill_down = lambda x, y: dict.get(x, y, default) if isinstance(x, dict) else default
    return reduce(drill_down, config_keys, _config_data)

def set_setting(*config_keys, value):
    current_value = _config_data

    for key in config_keys[:-1]:
        if isinstance(current_value, dict):
            current_value = current_value.get(key)
        else:
            return False

    last_key = config_keys[-1]
    if isinstance(current_value, dict):
        current_value[last_key] = value
    else:
        return False

    _save_config_data(_config_data)

    return True

As this is my first shot at handling configurations in a central way like this I am grateful for all feedback. I'm particularly interested in feedback about:

General approach
Comparison between the haskell-like approach of get_setting vs. the simpler and more readable approach of get_setting_old
set_setting seems rather unelegant to me. As the types of a setting can be either mutable or immutable I don't see a better solution though. I've thought about wrapper classes for primitive types, but that feels rather unpythonic and creates some unnecessary overhead when reading from and before writing to json. Maybe I'm missing something here.
I've been thinking about creating a simple class for a path of config_keys, to be able to pass a ConfigKeyPath object instead of a bunch of separated string arguments. I'm not sure which of the two approaches is better.

Rather than os and os.path use pathlib.Path.
def _save_config_data(content):
    CONFIG.parent.mkdir(parents=True, exist_ok=True)
    with open(CONFIG, 'w') as f:
        json.dump(obj=content, fp=f, indent=2)

def _load_config_data():
    file = CONFIG if CONFIG.exists() else DEFAULT_CONFIG
    with open(file, 'r') as f:
        return json.load(f)

Your code doesn't follow idiomatic Python.
Python uses errors for control flow so returning True/False in set_setting is not idiomatic.

You should make a walk function which you call from both get_setting and set_setting.

Reduce is generally not a good solution.
I would say in the specific example of your code, get_setting_old has far superior readability to get_setting.

import os
import json
from functools import reduce

from definitions import CONFIG, DEFAULT_CONFIG

__all__ = [
    "get_setting",
    "set_setting",
]

# Helper functions

def _save_config_data(content):
    CONFIG.parent.mkdir(parents=True, exist_ok=True)
    with open(CONFIG, 'w') as f:
        json.dump(obj=content, fp=f, indent=2)

def _load_config_data():
    file = CONFIG if CONFIG.exists() else DEFAULT_CONFIG
    with open(file, 'r') as f:
        return json.load(f)

def _walk(obj, path):
    for segment in path:
        obj = obj[segment]
    return obj

def get_setting(*args, default=_SENTINEL):
    try:
        return _walk(_CONFIG_DATA, args)
    except LookupError:
        if default is _SENTINEL:
            raise LookupError(f"cannot walk path; {args}") from None
        else:
            return default

def set_setting(value, *args):
    *args, segment = args
    try:
        node = _walk(_CONFIG_DATA, args)
        node[segment] = value
    except LookupError:
        raise LookupError(f"cannot set path; {args}") from None

_SENTINEL = object()
_CONFIG_DATA = _load_config_data()

I think using a class would be better as then you can define __getitem__ and __setitem__ for a potentially nicer interface.

class Walker:
    def __init__(self, data):
        self._data = data

    def __getitem__(self, args):
        if not isinstance(args, tuple):
            args = (args,)
        return self.get(*args)

    def __setitem__(self, args, value):
        if not isinstance(args, tuple):
            args = (args,)
        self.set(value, *args)

    def _walk(self, obj, path):
        for segment in path:
            obj = obj[segment]
        return obj

    def get(self, *args, default=_SENTINEL):
        try:
            return _walk(_CONFIG_DATA, args)
        except LookupError:
            if default is _SENTINEL:
                raise LookupError(f"cannot walk path; {args}") from None
            else:
                return default

    def set(self, value, *args):
        *args, segment = args
        try:
            node = _walk(_CONFIG_DATA, args)
            node[segment] = value
        except LookupError:
            raise LookupError(f"cannot set path; {args}") from None


C++: Generating similar methods with macros I am currently working on a project that involves Lua. For convenience, I created a wrapper class for the Lua state structure. However, I also added some methods for getting globals and table fields. Since these methods are very similar, I decided do turn them into macros.

Do you consider this a good solution? Would you have done it another way?

#include <lua/lua.hpp>

#include <util/Exception.h>
#include "LuaState.h"

namespace gnaar {

LuaState::LuaState() : ls(luaL_newstate())
{
    luaL_openlibs(ls);
}

void LuaState::doFile(const char* file)
{
    if (luaL_dofile(ls, file) != 0)
    {
        throwException(lua_tostring(ls, -1));
    }
}

lua_State* LuaState::operator*()
{
    return ls;
}

LuaState::~LuaState()
{
    lua_close(ls);
}

//==================== ugly macro stuff below, beware ====================

#define _getGlobal(_lua, _type, _check, _conversion)                    \
    template <>                                                         \
    _type LuaState::getGlobal<_type>(const char* name, _type dfault)    \
    {                                                                   \
        lua_getglobal(_lua, name);                                      \
        if (_check(_lua, -1))                                           \
        {                                                               \
            return (_type) _conversion(_lua, -1);                       \
        }                                                               \
        else                                                            \
        {                                                               \
            return dfault;                                              \
        }                                                               \
    }

#define _getTableField(_lua, _type, _check, _conversion)    \
    template <> _type LuaState::getTableField<_type>        \
    (const char* table, const char* key, _type def)         \
    {                                                       \
        lua_getglobal(_lua, table);                         \
        if (!lua_istable(_lua, -1))                         \
        {                                                   \
            return def;                                     \
        }                                                   \
        _type result;                                       \
        lua_pushstring(_lua, key);                          \
        lua_gettable(_lua, -2);                             \
        if (!_check(_lua, -1))                              \
        {                                                   \
            result = def;                                   \
        }                                                   \
        else                                                \
        {                                                   \
            result = (_type) _conversion(_lua, -1);         \
        }                                                   \
        lua_pop(_lua, 1);                                   \
        return result;                                      \
    }

_getGlobal(ls, int, lua_isnumber, lua_tonumber)

_getGlobal(ls, const char*, lua_isstring, lua_tostring)

_getGlobal(ls, bool, lua_isboolean, lua_toboolean)

_getTableField(ls, int, lua_isnumber, lua_tonumber)

_getTableField(ls, const char*, lua_isstring, lua_tostring)

_getTableField(ls, bool, lua_isboolean, lua_toboolean)

} // namespace gnaar

Do you consider this a good solution?

No.

  Would you have done it another way?

Yes.

Macros are never the solution. There is always a better tool. Macros are designed to deal with Hardware/OS/Compiler variations. You should limit their use to this situaation.

It looks like your macros are used to manually insert functions for specific situations. It seems like these could be deduced using template specializations. It is hard to provide a better solution without more code. But let my give it a go (this will probably be wrong as I don't have enough information).

template<typename Type>
TypeOfLuaObject LuaState::lua;

template<typename Type>
struct DoCheck;
template<typename Type>
struct DoConversion;

// Provide a specialization of these two macros for each
// type you are using.
template<>
struct DoCheck<int>
{
    bool operator()(TypeOfLuaObject& lauObjec, int val) const {return lua_isnumber(lauObjec, val);}
};
template<>
struct DoConversion<int>
{
    int operator()(TypeOfLuaObject& lauObjec, int val) const {return lua_tonumber(lauObjec, val);}
};

// The main function can then be left in its generalized form
// It will pick up the specialization of the above macros.
// Type will be deduced automatically from `def` parameter and the other two
// template parameters will be deduced from `Type` so you do not need to manually
// specify the type when calling.
template<typename Type, typename Check=DoCheck<Type>, Conversion=DoConversion<T>>
Type LuaState::getTableField(const char* table, const char* key, Type def)
{
    lua_getglobal(lua, table);
    if (!lua_istable(lua, -1))
    {
        return def;
    }
    Type result;
    lua_pushstring(lua, key);
    lua_gettable(lua, -2);
    Check doCheckObj;
    if (!checkObj(lua, -1))
    {
        result = def;
    }
    else
    {
        Conversion doConversionObj;
        result = doConversionObj(lua, -1);
    }
    lua_pop(lua, 1);
    return result;
}

Other comments:

When you use macros. Make sure they are all uppercase.

Macros do not obey scope boundaries. So we reserve all uppercase letters for macros to prevent accidental smashing of normal variables.

Don't use '_' as a prefix for identifiers.
Most people don't know the actual rules for this. So even if you do it will confuse them.

But you are breaking the rule with your macros _getGlobal and _getTableField. As they are macros they are not bound by the scope of your namespace and thus could potentially be trampling on some system macro.

See the artile I wrote on SO: https://stackoverflow.com/questions/228783/what-are-the-rules-about-using-an-underscore-in-a-c-identifier/228797#228797


Slaying a dragon, the old-school way Is the below program written in the most efficient way possible? I am new to Java Script and am just trying to make something basic so I can practice as I learn more.

The way I wrote it the program does work (except it keeps wanting to run again after it's over?) but is there a better way I could have accomplished this?

var charName = prompt("Hello Lost Stranger, Tell us your name!");
alert("Welcome " + charName + " we have been waiting for someone who can challenge this dragon!");
alert("A rather small but still intimidating dragon appears; it does not look happy.");

var charHP = 3000;
var mobHP = 3000;
var userAttack = "What type of attack will you use?(Choices: Sword, Charge, Spell)";

var diceOne = Math.floor(Math.random() * 100) + 1;
var diceTwo = Math.floor(Math.random() * 100) + 1;
var diceThree = Math.floor(Math.random() * 100) + 1;

function report() {
    alert("You have " + charHP + " remaining and the dragon has " + mobHP + " remaining.");
}

function attack() {
    function report() {
        alert("You have " + charHP + " remaining and the dragon has " + mobHP + " remaining.");
    }
    var attackType = prompt(userAttack);
    function evaluateAttack() {
        if (attackType.toLowerCase() === "sword") {
            mobHP = mobHP - diceOne - diceTwo - diceThree;
            alert("You slash at the dragon three times drawing blood!");
        } else if (attackType.toLowerCase() === "charge") {
            mobHP = mobHP - (3 * diceOne);
            alert("You charge the dragon at full speed drawing blood!");
        } else if (attackType.toLowerCase() === "spell") {
            mobHP = mobHP - (4 * diceTwo);
            alert("You close your eyes and summon the fires of hell on the dragon");
        }
    }
    if (mobHP > 0) {
        report();
        evaluateAttack();
    } else {
        alert("The dragon is dead! You have won!");
    }
}

do {
    attack();
} while (mobHP > 0);

Ok, the following isn't necessarily about making your code more efficient (in the sense it executes faster), just a couple of quick observations/pointers.

Duplicate code like your two identical report() functions should
be avoided.
Your if..else if..else if statement may be better suited as a switch()/case.  Under some conditions, switch() can be more efficient than if/else-if/.. (depending on the javascript engine, etc.)   I should note that developers have been fighting the "switch-vs-if-else-if war for a while now.  https://stackoverflow.com/questions/2922948/javascript-switch-vs-if-else-if-else
Your dice values appear fixed (ie. they do not change).  Perhaps this is as intended, but I would suggest a rollDice() function is missing.
Closures (nested functions, or "functions within functions" if you like) may be a difficult concept for beginners...  It can introduce problems with variable scope that can be difficult to understand/deal with.   Disclaimer:  I am not saying closures are bad, they are powerful - but perhaps overkill for your current purposes, that's all.


Reducing code duplication for my C# math class I have a library with a lot of duplicate code to accommodate different collection types, and its starting to get a bit tedious, is there a better way to do this to reduce the amount of duplication going on ? Its only going to get worse the more functions i add...

This is my current library script:

using System.Collections.Generic;
using UnityEngine;

namespace Math {
    public static class Geometry
    {
        public static float Area(List<Vector2> points)
        {
            float a = 0;
            for (int i = 0; i < points.Count; i++)
            {
                var p1 = points[i];
                var p2 = points[(i + 1) % points.Count];

                a += p1.x * p2.y - p2.x * p1.y;
            }

            return a * 0.5f;
        }

        public static float Area(List<Vector3> points)
        {
            float a = 0;
            for (int i = 0; i < points.Count; i++)
            {
                var p1 = points[i];
                var p2 = points[(i + 1) % points.Count];

                a += p1.x * p2.z - p2.x * p1.z;
            }
            return a * 0.5f;
        }

        public static float Area(Vector3[] points)
        {
            float a = 0;
            for (int i = 0; i < points.Length; i++)
            {
                var p1 = points[i];
                var p2 = points[(i + 1) % points.Length];

                a += p1.x * p2.z - p2.x * p1.z;
            }
            return a * 0.5f;
        }

        public static float Area(Vector2[] points)
        {
            float a = 0;
            for (int i = 0; i < points.Length; i++)
            {
                var p1 = points[i];
                var p2 = points[(i + 1) % points.Length];

                a += p1.x * p2.y - p2.x * p1.y;
            }
            return a * 0.5f;
        }

        public static bool IsClockwise(List<Vector3> points)
        {
            float sum = 0;

            for (int i = 0; i < points.Count - 1; i++)
            {
                var point1 = points[i];
                var point2 = points[i + 1];

                sum += (point2.x - point1.x) * (point2.z + point1.z);
            }

            return sum > 0;
        }
        public static bool IsClockwise(Vector3[] points)
        {
            float sum = 0;

            for (int i = 0; i < points.Length - 1; i++)
            {
                var point1 = points[i];
                var point2 = points[i + 1];

                sum += (point2.x - point1.x) * (point2.z + point1.z);
            }

            return sum > 0;
        }
        public static bool IsClockwise(List<Vector2> points)
        {
            float sum = 0;

            for (int i = 0; i < points.Count - 1; i++)
            {
                var point1 = points[i];
                var point2 = points[i + 1];

                sum += (point2.x - point1.x) * (point2.y + point1.y);
            }
            return sum > 0;
        }
        public static bool IsClockwise(Vector2[] points)
        {
            float sum = 0;

            for (int i = 0; i < points.Length - 1; i++)
            {
                var point1 = points[i];
                var point2 = points[i + 1];

                sum += (point2.x - point1.x) * (point2.y + point1.y);
            }

            return sum > 0;
        }
    }
}

Depending on how many points you'll be dealing with, you could go one step further.  Since you're only using 2 points from the Vector3, it's fairly simple to cast the Vector3's into Vector2's.

public static float Area(List<Vector2> points)
{
    float a = 0;
    for (int i = 0; i < points.Count; i++)
    {
        var p1 = points[i];
        var p2 = points[(i + 1) % points.Count];

        a += p1.x * p2.y - p2.x * p1.y;
    }

    return a * 0.5f;
}

public static float Area(List<Vector3> points)
{

    return Area(points.Select(x => new Vector2(x.x,x.z)).ToList());
}

public static float Area(Vector3[] points)
{
    return Area(points.ToList());
}

public static float Area(Vector2[] points)
{
    return Area(points.ToList());
}

Upon further reflection the code reduction can be taken a step further by using IEnumerable<T>.  Now it becomes:

public static float Area(IEnumerable<Vector2> points)
{
    float a = 0;
    var first = points.First();
    var prev = first;
    foreach (var curr in points.Skip(1))
    {
        a += prev.x * curr.y - curr.x * prev.y;
        prev = curr;

    }
    a += prev.x * first.y - first.x * prev.y;
    return a * 0.5;
}

public static float Area(IEnumerable<Vector3> points)
{

    return Area(points.Select(x => new Vector2(x.x,x.z)));
}


Query for your Answer Progress I was inspired to write a query that produced a graph mapping the perceived quality of your Stack Exchange answers over time.... in other words, "Are your answers getting better, or worse?"

Because answer scores are very 'spiky', it seems natural to calculate a sliding average with a configurable window, to show trends.

The result I have is:

And this is the SEDE query.

with Rolling as (
    Select Rank() over (order by CreationDate Asc) as Answer,
           CreationDate,
           Convert(float, Score) as Score
    from Posts
    where PostTypeId = 2
      and OwnerUserId = ##UserId:int##
)
select Master.CreationDate, Master.Score as [Score], Avg(Slave.Score) as [RollingAvg]
from Rolling as Master,
     Rolling as Slave
where Slave.Answer  > Master.Answer - ##RollingAvg:int?10##
  and Slave.Answer <= Master.Answer
group by Master.CreationDate, Master.Score
order by Master.CreationDate

I am particularly interested to know whether the rolling average can be computed in a more SQL Server friendly way. There does not appear to be a performance problem, but the CTE and self join seem.... ugly.

Any other observations?

There is a simple way to calculate rolling averages in SQL, with a caveat.

SELECT CreationDate
     , Score
     , avg(CAST(Score AS FLOAT)) OVER (
         ORDER BY CreationDate
         ROWS BETWEEN ##RollingAvg:INT?10## PRECEDING AND CURRENT ROW
       ) AS RollingAvg
    FROM Posts
    WHERE PostTypeId = 2 AND OwnerUserId = ##UserId:INT##;

The problem with that is that a window that includes up to n preceding rows is one row larger than the window in your original query.  Unfortunately, SQL Server requires the window frame bound to be specified using an unsigned integer literal — it cannot be an expression.

That turns out to be the most annoying off-by-one error to fix:

DECLARE @UserId AS INT = ##UserId##;
DECLARE @RollingAvg AS INT = ##RollingAvg:INT?10##;

DECLARE @Sql AS NVARCHAR(1000) = 'SELECT CreationDate
     , Score
     , avg(CAST(Score AS FLOAT)) OVER (
         ORDER BY CreationDate
         ROWS BETWEEN ' + CAST(@RollingAvg - 1 AS VARCHAR) + ' PRECEDING AND CURRENT ROW
       ) AS RollingAvg
    FROM Posts
    WHERE PostTypeId = 2 AND OwnerUserId = ' + CAST(@UserId AS VARCHAR);

EXEC sp_executesql @Sql;

It's arguable that your original formulation, calculating the window average the "hard" way, is better than constructing the SQL dynamically to be EXECuted.

Of course, if you are willing to redefine the meaning of the window size parameter, then the window aggregate function would be the way to go.


Asynchronous dictionary in Python So I am working on writing a discord bot with discord.py.

I decided, rather than making a serie of if/elif, to map the messages to the functions in a dictionnary, like so:

my_dict = { "!trigger": func }

I then wanted to parse this dictionnary into an async for loop, but ran into some problems. To face those problems, I decided to write a dict() subclass, which I called AsyncDict. I just want to know if this is safe to work like this, or not? The main purpose of this little class is to avoid errors like "an async for loop needs an item that defines the function __aiter__"

AsyncDict class:

class AsyncDict(dict):
    def __init__(self, *args, **kwargs):
        super(AsyncDict, self).__init__(*args, **kwargs)

    async def items(self):
        items = super().items()
        for item in items:
            yield item

    async def keys(self):
        keys = super().keys()
        for key in keys:
            yield key

    async def values(self):
        values = super().values()
        for value in values:
            yield

Once this is done, I just create my dictionary "as usual":
calls = AsyncDict([("!ping", ping)])

Here is the relevant code in my main.py:

import discord
import asyncio

async def ping(message):
    await client.send_message(message.channel, 'pong')

calls = AsyncDict([("!ping", ping)])

@client.event
async def on_message(message):
    async for k, v in calls.items():
        if message.content.startswith(k):
            await v(message)

I just want to know if it's "safe" to work like this (cf.: AsyncDict)? I'm quite new to asynchronous development and I don't know if I'm missing something.

I saw your post on python-ideas. I think you have a misconception about native coroutines (i.e. async def). For example, this method in AsyncDict:

async def items(self):
    items = super().items()
    for item in items:
        yield item

This method does not have any await expressions, therefore the method itself should not be async def. It should just be a regular def. You may not realize it, but an async def function can still call regular def functions, and it can use regular with and for blocks.

In your use case, you should use a regular dictionary and a regular for loop:

calls = {"!ping": ping}

@client.event
async def on_message(message):
    for k, v in calls.items():
        if message.content.startswith(k):
            await v(message)

This is perfectly legal, and in fact better than trying to create "Async" variants of common data structures that don't need it.


Converting A Map[String, Seq[String]] To A Seq[(String, String)] in Scala My current code:

val headers : Map[String, Seq[String]] = ...
//NOTE:  ("key" -> Seq()) is  valid entry
headers.toList flatMap {x => x._2 map { y => (x._1 -> y) }}

This feels clumsy...is there a better way to do this?

Goal

I have a Map[String, Seq[String]] representing all the headers in an HTTP request.   That needs to be translated to a Seq[(String, String)] in order to be passed to a method that expects that headers to be stored in that format.

The code is supposed to convert to this format.   The pair "something" -> Seq("val1", "val2", "val3") would be translated to 3 (String,String)'s.   "something" -> Seq() would be translated to 0.

Concern

I'm still getting the hang of some aspects of the functional approach.  In a lot of cases, this one included, it feels like I'm building hard-to read, overly complex code.  I get that it's just a tolist + two maps, but this seems way less clear than some of the imperative alternatives, for example:

val ret = mutable.Seq[(String,String]()
for (kv <- headers; value <- kv._2) ret :+ (kv._1, value)

or even:

Set[Tuple2[String,String]] ret = new HashSet[Tuple2[String,String]]()
for (key : headers.keySet) {
  for (value : headers.get(key) {
     ret.add(new Tuple2(key, value))
  }
}

I don't see how the functional code looks clumsy or difficult to read. If you have problems reading functional code, than don't write it - the imperative code is entirely fine, as long as the side effects remain local to a single function.

A lot of readability can be gained by giving your intermediate values useful names and to apply a different formatting:

headers.toSeq flatMap {
  case (parameter, values) =>
     values map (parameter -> _)
}

No need to squash everything into a single line or to use tuples ugly _X accessors.

Another way to write your code is:

for ((parameter, values) <- m.toSeq; v <- values)
  yield parameter -> v

which looks very similar to the imperative solution but is exactly the same as the code before.

Write code that belongs to your skill level. If you don't feel fine with shorter and more functional code, then let it longer and more imperative. Improve your skills during the next months and refactor the code once you come back and see the need for it.


Retrocomputing scanline function I need to refactor this code, targeted to Google Chrome (current+) to make it manageable going forward.

Here is a working JSBin of the idea.

The messiness comes from determining the accurate number of lines of text, and the real heights of elements. I know I am passing this information around too much, without enough structure, and recalculating too many things. I also know it is messy, especially inside scal_el and run_scan.

function scan_el(el_fin_callback,el,mask,sub_mask,real_height,real_width,push,lines,line) {
        line = line || 0;
        if(line >= lines) {
                el_fin_callback();
                return; // scan finished
        }
        var current_line = real_height*line;
        var line_os = $(el).offset();
        line_os.top += current_line;
        if(lines > 1) {
                $(mask)[0].style.height = real_height+'px';
        }
        $(sub_mask).css( {
                'height' : $(el).height()-real_height-current_line } );
        $(mask).offset(line_os);
        line_os.top += real_height;
        var length = Math.ceil($(el).text().length*real_width)+push;
        if(lines > 1) {
                length = $(el).width();
        }
        $(sub_mask).offset(line_os);
        function next_line() { // callback after scan complete
                scan_el(el_fin_callback,el,mask,sub_mask,real_height,real_width,push,lines,line+1);
        }
        run_line_scan(length,mask,next_line);
}

There are only two things I can think of.

Sometimes it's best to pass objects when there are more than 4 parameters for a method call.

Instead of this

function scan_el(el_fin_callback, el, mask, sub_mask, real_height, real_width, push, lines, line) {

Try this.

function scan_el(el_fin_callback, els, metrics, lines, line) {

els and metrics would look something like this.

els = {
    el : jQuery,
    mask : jQuery,
    submask : jQuery
};
metrics = {
    real_width : Number,
    real_height : Number,
    push : Number
}

Try to make functions no longer than 8-12 lines.

You could extract the real_* variables into a function and return a object instead of a list of variables.

function getRealMetrics(el){
      // doesn't work completely
    var real = {};
    real.box = window.getComputedStyle(el);
    real.width = parseFloat(real.box.width);
    real.height = parseFloat(real.box.height);
    real.el = el.getBoundingClientRect();
    real.el_style = window.getComputedStyle(el);
    real.el_padding = parseFloat(real.el_style.padding);
    real.el_padding_left = parseFloat(real.el_style.paddingLeft);
    real.el_margin_left = parseFloat(real.el_style.marginLeft);
    real.el_align = real.el_style.textAlign;

    if (!isNaN(real.el_padding)) {
        real.el_height -= real.el_padding * 2.0;
    }
    var lines = Math.ceil(real.el_height / real.height);
    if (lines > 1) {
        length = real.el.width;
    }
    real.el_push = real.el_margin_left + real.el_padding_left + (real.el.width - length) / 2.0;
    if (real.el_align !== 'center') {
        real.el_push = real.el_margin_left + real.el_padding;
    }
    return result;
}


Logic Gate Simulator - Proper Encapsulation So I was checking out the courses on InteractivePython, and I came across this example to demonstrate inheritance.

My focus is on the Connector class and how it interacts with BinaryGate and UnaryGate via the setNextPin method.

class LogicGate:

    def __init__(self,n):
        self.name = n
        self.output = None

    def getName(self):
        return self.name

    def getOutput(self):
        self.output = self.performGateLogic()
        return self.output

class BinaryGate(LogicGate):

    def __init__(self,n):
        LogicGate.__init__(self,n)

        self.pinA = None
        self.pinB = None

    def getPinA(self):
        if self.pinA == None:
            return int(input("Enter Pin A input for gate "+self.getName()+"-->"))
        else:
            return self.pinA.getFrom().getOutput()

    def getPinB(self):
        if self.pinB == None:
            return int(input("Enter Pin B input for gate "+self.getName()+"-->"))
        else:
            return self.pinB.getFrom().getOutput()

    def setNextPin(self,source):
        if self.pinA == None:
            self.pinA = source
        else:
            if self.pinB == None:
                self.pinB = source
            else:
                print("Cannot Connect: NO EMPTY PINS on this gate")

class AndGate(BinaryGate):

    def __init__(self,n):
        BinaryGate.__init__(self,n)

    def performGateLogic(self):

        a = self.getPinA()
        b = self.getPinB()
        if a==1 and b==1:
            return 1
        else:
            return 0

class OrGate(BinaryGate):

    def __init__(self,n):
        BinaryGate.__init__(self,n)

    def performGateLogic(self):

        a = self.getPinA()
        b = self.getPinB()
        if a ==1 or b==1:
            return 1
        else:
            return 0

class UnaryGate(LogicGate):

    def __init__(self,n):
        LogicGate.__init__(self,n)

        self.pin = None

    def getPin(self):
        if self.pin == None:
            return int(input("Enter Pin input for gate "+self.getName()+"-->"))
        else:
            return self.pin.getFrom().getOutput()

    def setNextPin(self,source):
        if self.pin == None:
            self.pin = source
        else:
            print("Cannot Connect: NO EMPTY PINS on this gate")

class NotGate(UnaryGate):

    def __init__(self,n):
        UnaryGate.__init__(self,n)

    def performGateLogic(self):
        if self.getPin():
            return 0
        else:
            return 1

class Connector:

    def __init__(self, fgate, tgate):
        self.fromgate = fgate
        self.togate = tgate

        tgate.setNextPin(self)

    def getFrom(self):
        return self.fromgate

    def getTo(self):
        return self.togate

def main():
   g1 = AndGate("G1")
   g2 = AndGate("G2")
   g3 = OrGate("G3")
   g4 = NotGate("G4")
   c1 = Connector(g1,g3)
   c2 = Connector(g2,g3)
   c3 = Connector(g3,g4)
   print(g4.getOutput())

main()

And so the output might look like this:

Input Pin A for gate G1 --> 0
Input Pin B for gate G1 --> 0
Input Pin A for gate G2 --> 1
Input Pin B for gate G2 --> 0
1

So everything is working as intended, but my concern is the "two-way access" between, for example, BinaryGate and Connector.

It's bothering me that BinaryGate.getPinA can call (Connector).getFrom().getOutput() in the line return self.pinA.getFrom().getOutput(), while Connector.__init__() calls BinaryGate.setNextPin

In my mind, there should only be one direction of "visibility" between the two objects - either a *Gate can access members of a Connector, or vice-versa, but not both ways at once. Is this a flawed assumption?

Does the code as-is break encapsulation? If so, how can the code be refactored to have proper encapsulation? If not, why not?

Edit:

Just to be clear for future viewers, this is not my code, it's from InteractivePython's Data Structures course.

You are mixing up lots of different responsibilities in your classes. For instance, AndGate has to handle both the And part and all the pin input parts. It is weird that an AndGate doesn't actually take references to its input pins. Furthermore, how in this model do we connect gates to each other? By your design, a "pin" is a value comparable to an integer. What if I wanted to model something like (picking a random circuit from google):

How would I do that in your system? The Connectors confuse the logic, since in your example you're creating a bunch of gates that are floating around - and then just to see that G1 and G2 go into G3 is split across several lines, rather than ideally:

g3 = OrGate('G3', g1, g2)

As a first go, gates should know their inputs and be able to perform logic to determine their outputs:

class NandGate(LogicGate):
    def __init__(self, name, a, b):
        LogicGate.__init__(self, name)
        self.a = a
        self.b = b

    @property
    def output(self):
        return not (self.a.output and self.b.output)

This separates the concern of input handling (determining the pins) from the concern of actually emulating the gates (determining the outputs) - and is what will let us have NandGates as input to other NandGates:

A = ConstantPin('A', True)
B = ConstantPin('B', False)
N1 = NandGate('N1', A, B)
N2 = NandGate('N2', A, N1)
N3 = NandGate('N3', N1, B)
N4 = NandGate('N4', N2, N3)
N5 = NandGate('N5', N4, N4)
Q = N5.output


Computing the total different between a pixel and its neighborhood I have C++ code which written for mex in MATLAB. It used to compute the total different between a pixel and its neighborhood (8 neighborhood in my code). The code ran and gave me an expected result. However, I think that we can make my code more faster (because of a lot of if statement). I am very happy if someone can help me to speed up it.

#include "mex.h"
#include <vector>
#include <math.h>
#define img(i,j) img[(i) + (j)*M]
void mexFunction( int nlhs, mxArray *plhs[], int nrhs, const mxArray *prhs[] )
{
    double  *img, *G;
    int    N, M, class_count;
    int ndim=3;
    double beta, energy;
    N           = (int) mxGetN(prhs[0]); //cols
    M           = (int) mxGetM(prhs[0]); //rows
    img         = (double *)mxGetData(prhs[0]);
    beta        = (double )*mxGetPr(prhs[1]);
    class_count = (int )*mxGetPr(prhs[2]);
    /* Initial zeros matrix*/
    int dims[3] = {M-2, N-2, class_count};
    plhs[0]     = mxCreateNumericArray(ndim, dims, mxDOUBLE_CLASS, mxREAL);
    G           = (double *) mxGetPr(plhs[0]);
    /////////////Main///////////////
    for (int label=2; label <= class_count+1; label++) {
       for (int i=1; i < M-1; i++) {
         for (int j=1; j < N-1; j++) {
                energy = 0;
                 // North, south, east and west
                if (label == img(i-1,j)) energy = energy-beta;
                else energy = energy+beta;
                if (label == img(i,j+1)) energy = energy-beta;
                else energy = energy+beta;
                if (label == img(i+1,j)) energy = energy-beta;
                else energy = energy+beta;
                if (label == img(i,j-1)) energy = energy-beta;
                else energy = energy+beta;
                if (label == img(i-1,j-1)) energy = energy-beta;
                    else energy = energy+beta;
                if (label == img(i-1,j+1)) energy = energy-beta;
                    else energy = energy+beta;
                if (label == img(i+1,j+1)) energy = energy-beta;
                    else energy = energy+beta;
                if (label == img(i+1,j-1)) energy = energy-beta;
                    else energy = energy+beta;
                G[(i-1) + (j-1)* (M-2) + (label-2) * (N-2) * (M-2)]=energy;
             }
        }
    }
}

Here are some things that may help you improve your code.

Measure your code

Since we don't have your data or your computer or your compiler, we're a little bit hampered in trying to identify things that will speed up your code.  However, you have access to all of those things, so rather than guess that it's the cause of the if-then constructs or anything else, the prudent thing to do would be to measure the code and find out (e.g. with a profiler) where your code is spending most of its time.  Armed with that data, you can begin to effectively target the real bottlenecks.

Think about "locality of reference"

With modern computers, cacheing is often a factor in determining performance.  Generally, data items that are "close together" (that is, items that all fit in the cache at once) will be quicker to access than those that are "far apart" (that is, items that do not all fit in the cache at once).  For this reason, we can think about restructuring the algorithm to try to work on as small a memory footprint as possible.  In this case it's possible you can gain performance by simply rearranging the nested loops.  Right now, it looks like this:

for (int label=2; label <= class_count+1; label++) {
   for (int i=1; i < M-1; i++) {
      for (int j=1; j < N-1; j++) {

But I think I'd try arranging things like this instead:

for (int j=1; j < N-1; j++) {
   for (int i=1; i < M-1; i++) {
      for (int label=2; label <= class_count+1; label++) {

This way, the input data is accessed in a somewhat more sequential manner.  The output data is not, but because it's only accessed once per loop, I'm guessing that it makes less difference.

Simplify your code

One of the things your code does is to either add or subtract an energy value if a particular value is matched.  Because integer math is often faster than floating point, you might benefit from restructuring your code.  Here's one way to do that.  Change the interior of the loop to look like this instead:

int energycount = 8;
if (label == img(i-1,j-1)) energycount -= 2;
if (label == img(i  ,j-1)) energycount -= 2;
if (label == img(i+1,j-1)) energycount -= 2;
if (label == img(i-1,j  )) energycount -= 2;
if (label == img(i+1,j  )) energycount -= 2;
if (label == img(i-1,j+1)) energycount -= 2;
if (label == img(i  ,j+1)) energycount -= 2;
if (label == img(i+1,j+1)) energycount -= 2;
G[(i-1) + (j-1)* (M-2) + (label-2) * (N-2) * (M-2)]=beta*energycount;

Use const where practical

Many of these values are set once and then not changed.  The compiler may be able to find better optimizations if you explicitly tell it which values are const:

const int ndim = 3;
const int N = (int) mxGetN(prhs[0]); //cols
const int M = (int) mxGetM(prhs[0]); //rows
const double *img = (double *)mxGetData(prhs[0]);
const double beta = (double )*mxGetPr(prhs[1]);
const int class_count = (int )*mxGetPr(prhs[2]);
/* Initial zeros matrix*/
const int dims[3] = {M-2, N-2, class_count};
plhs[0] = mxCreateNumericArray(ndim, dims, mxDOUBLE_CLASS, mxREAL);
double *G = (double *) mxGetPr(plhs[0]);

Don't initialize large data items pointlessly

It appears that every member of G is initialized by the loop, so there's no real need to initialize it before that.  For that reason, I'd recommend using mxCreateUninitNumericArray to create an unitialized array rather than mxCreateNumericArray which initializes all values to 0.


Waiting for a file to be accessible with Thread.Sleep() and exception catching I know I've already asked a similar question, but this feels quite different to me.

The main goal is to wait for a file to be accessible (read: waiting until the file lock releases), since it seems like there's no way to do this built into the .NET framework I've written a quick and dirty function to do this.

The requirements which should be kept in mind with this:

The file shouldn't be locked often. Actually, it should be locked quite rarely (figure how often clients manage to still hit that).
There's no need for a limit, if the file is locked for longer then a second, something is fuc^H^H^Hscrewed up.
It should block the main thread until the file is available.

///<summary>
/// Tries to open the specified file over and over again until it is acessible.
///</summary>
public static FileStream WaitForStream(String filename, FileAccess fileAccess, FileShare, fileShare)
{
    while(true) // Gives me the chills
    {
        try
        {
            return new FileStream(filename, FileMode.OpenOrCreate, fileAccess, fileShare);
        }
        catch (IOException ex)
        {
            Logger.LogError(ex); // Information purposes only
            Threading.Thread.Sleep(250); // Just looks wrong
        }
    }

    return null; // Only to keep the compiler happy.
}

The problems I see myself:

while(true)
Thread.Sleep()
It might loop forever, but as I said, if that happens we need to intervene anyway.

Reimplementing this with a similar solution I already used smells like absolute overkill to me, given that this should be built-in functionality anyway. What stuff didn't I think about?

How about making this an asynchrounous operation that only returns if the stream has successfully been opened. you can get rid of the while(true) and the thread sleep plus the new mthod call cn have a retry count.

Here is what it might look like if you use C# 4.0 (not tested).

Task<FileStream> OpenStreamAsync(String filename, FileAccess fileAccess, FileShare fileShare,int retryCount){

    if (retryCount < 1)
        throw new Exception("maximum retry reached");

    return
        Task.Factory.StartNew(() => new FileStream(filename, FileMode.OpenOrCreate, fileAccess, fileShare))
        .ContinueWithTask( task => {
            if (task.IsCompleted)
                return task;

            Logger.LogError(ex); // Information purposes only
            return TaskExtenstion2.Wait(TimeSpan.FromMilliseconds(250))
                .ContinueWithTask(t => OpenStreamAsync(filename, fileAccess, fileShare,retryCount--) );
        });
}

public static class TaskExtenstion2 {

    /// Set the completionSource to the same values as the task
    public static void SetCompletionSource<TResult>(this Task<TResult> task, TaskCompletionSource<TResult> completionSource){
        if (task.IsCompleted){
            completionSource.SetResult(task.Result);
        }else if (task.IsCanceled){
            completionSource.SetCanceled();
        }else if (task.IsFaulted){
            completionSource.SetException(task.Exception);
        }
    }

    /// Continues a task with another task genrated by the specified function
    public static Task<U> ContinueWithTask<T,U>(this Task<T> sourceTask, Func<Task<T>,Task<U>> continuation){
        var completionSource = new TaskCompletionSource<U>();
        sourceTask.ContinueWith(firstTask => {
            var secondTask = continuation(firstTask);
            secondTask.ContinueWith(task => task.SetCompletionSource(completionSource));
        });
        return completionSource.Task;
    }

     /// returns true after a certain amount of time
     public static Task<bool> Wait(TimeSpan span){
         var completionSource = new TaskCompletionSource<bool>();
         Timer timer = null;
         timer = new Timer(_ => {
             using(timer) {
               completionSource.SetResult(true);
             }
         },null,span,TimeSpan.MaxValue);
         return completionSource.Task;
     }
}

if you still want to block the main thread, getting the Result property of the returning task will make sure of that

var stream = OpenStreamAsync("myFile", FileAccess.ReadWrite, FileShare.None,20).Result;


Parsing a TLV string This piece of code is supposed to walk through a TLV string and print out its contents. For this particular example, tag field length is 2, size field length is 3.

As a mostly C programmer, this is what I came up with. I know I can parametrize the tag and length field sizes, but I don't care about that right now. Just interested to know the more pythonic way of parsing the string.

tlv = "01011Lorem ipsum02014dolor sit amet03027consectetur adipiscing elit"

temp = tlv[:]

while len(temp):
  print('tag: {}'.format(temp[0:2]))
  print('length: {}'.format(temp[2:2+3]))
  tam = int(temp[2:2+3])
  print('value: \'{}\'\n'.format(temp[5:5+tam]))
  temp = temp[5+tam:]

Some initial comments:

Good use of string slicing to extract each field, though ideally we should not need to make a copy of the string. I propose a different way of doing this below.
while len(temp): works, but a more Pythonic way of writing this would be while temp: because any object can be tested for a truth value in Python, and non-empty strings evaluate to true.
A popular new way of string interpolation introduced in Python 3.6 is the f-string. So instead of 'tag: {}'.format(temp[0:2]) you could do f'tag: {temp[0:2]}' which I personally find much easier to read.

To avoid copying the string tlv we can instead work with an iterator over the characters in the string, i.e. iter(tlv). Then we can use itertools, a nice built-in library for working with iterators, specifically itertools.islice to extract/consume chunks of arbitrary length from the iterator:

from itertools import islice

TAG_FIELD_LENGTH = 2
LENGTH_FIELD_LENGTH = 3

def tlv_parser(tlv_string):
    it = iter(tlv_string)
    while tag := "".join(islice(it, TAG_FIELD_LENGTH)):
        length = int("".join(islice(it, LENGTH_FIELD_LENGTH)))
        value = "".join(islice(it, length))
        yield (tag, length, value)

Notes on the above:

The strategy of parsing is very similar to yours, with the only difference being that we're working with an iterator of characters, and we're consuming from the iterator chunk by chunk so we don't need to calculate indices for field boundaries.
We're concatenating the characters in each iterator to a string with "".join(...), i.e. str.join.
:= is the "walrus operator" (introduced in Python 3.8) that binds values to variables as part of a larger expression, so we're binding the value of "".join(islice(it, TAG_FIELD_LENGTH)) to tag and at the same time testing its truth value.
The yield keyword makes tlv_parser a generator of 3-tuples of (tag, length, value).

Example usage:

>>> tlv = "01011Lorem ipsum02014dolor sit amet03027consectetur adipiscing elit"

>>> for t in tlv_parser(tlv):
...     print(t)
...
('01', 11, 'Lorem ipsum')
('02', 14, 'dolor sit amet')
('03', 27, 'consectetur adipiscing elit')

>>> for tag, length, value in tlv_parser(tlv):
...     print(f"tag: {tag}")
...     print(f"length: {length}")
...     print(f"value: {value!r}\n")
...
tag: 01
length: 11
value: 'Lorem ipsum'

tag: 02
length: 14
value: 'dolor sit amet'

tag: 03
length: 27
value: 'consectetur adipiscing elit'


Templated byte streaming I often have the need to take some type and stream it to something as a sequence of character byte values, so I came up with this to accomplish the task:

#include <iostream>
#include <iterator>

/// This provides a way of getting the byte representation of an
/// object of underlying type T.
///
/// This is an alternative to memcopying bytes into a string, stream,
/// character array...
template<typename inT, typename outT = char>
struct byte_streamer
{
    byte_streamer(inT ref)
      : data(ref)
    {}
    const inT data;
    outT placeholder;
};

template<typename inT, typename outT>
std::ostream& operator<<(std::ostream& strm, const byte_streamer<inT, outT> val)
{
    std::copy(reinterpret_cast<const char*>(&val.data),
              reinterpret_cast<const char*>(&val.data) + sizeof(inT),
              std::ostream_iterator<decltype(val.placeholder)>(strm, ""));
    return strm;
}

int main()
{
    std::cout << "Here's the bytes of 42 copied directly: " << byte_streamer(42) << std::endl;
    std::cout << "Here's the bytes of 42 copied as their numerical values: "
              << byte_streamer<int, int>(42);
}

You can run it online on Coliru or just look at the output here:

Here's the bytes of 42 copied directly: *
Here's the bytes of 42 copied as their numerical values: 42000

Well, there are a few points:

You make a copy of the object you output. That can be costly, impossible, or have undesirable side-effects. Use a constant reference instead.
There are essentially two modes for your byte_streamer: Output the raw bytes, or output as decimal values for each byte.
The latter is broken as each byte is variable-width (1-3 digits for octets), and there is no separator.
You use the member placeholder only to convey the type outT to other code. Why not use a typedef instead, avoiding inefficiency and painful contortions?
Only use std::endl if you want to flush the stream. Otherwise, using '\n' and avoiding that extra-work is simply far more efficient.
It's really unusual not to end the programs output with a line-break. Wonder where the prompt will end up if called from a terminal...

Imho, the only useful outputs would be hex-dump and raw-bytes.

class dump_hex_t {
    const void* p = nullptr;
    std::size_t s = 0;
public:
    constexpr dump_hex_t() noexcept = default;
    constexpr dump_hex_t(const void* p, std::size_t s) noexcept : p(p), s(s) {}

    template <class CharT, class Traits>
    friend auto& operator<<(std::basic_ostream<CharT, Traits>& os, dump_hex_t x) {
        ... // Dump as hex-digits however you like
    }
};

template <class T>
constexpr dump_hex_t dump_hex(const T& x) noexcept
{ return { std::addressof(x), sizeof x }; }

template <class T>
constexpr std::string_view dump_raw(const T& x) noexcept
{ return { (const char*)std::addressof(x), sizeof x }; }


Sorting by pivot table results I had to create a report for a customer by most sold products in the last month and sorted by parameters they have assigned.

We have three parameters on products: Colour, Type and Collection.

The desired result is to sort products by most sold Collection, then most sold Colour, then most sold Type. The query I have written is monstrous, and I am sure it could be written efficiently.

SELECT a.ID, collection.value as title, colour.value as colour, type.value as type
FROM orderrows oa
JOIN order o ON o.ID = oa.orderID
JOIN products a ON oa.productid = a.ID
JOIN productparameter as collection ON collection.productid = a.ID AND collection.parameterid = 'collection_id'
JOIN productparameter as type ON type.productid = a.ID AND type.parameterid = 'type_id'
JOIN productparameter as colour ON colour.productid = a.ID AND colour.parameterid = 'colour_id'
JOIN
(
    SELECT @rowId:=@rowId+1 AS rowId, collection.title
    FROM
    (
        SELECT collection.value as title
        FROM orderrows oa
        JOIN order o ON o.ID = oa.orderID
        JOIN products a ON oa.productid = a.ID
        JOIN productparameter as collection ON collection.productid = a.ID AND collection.parameterid = 'collection_id'
        WHERE o.orderdate > NOW() - INTERVAL 1 MONTH
        GROUP BY collection.value
        ORDER BY SUM(oa.productprice * oa.productamount) DESC
    ) collection,
    (
        SELECT @rowId:=0
    ) t2
) as collection_sort ON collection.value = collection_sort.title
JOIN
(
    SELECT @rowId:=@rowId+1 AS rowId, colour.title
    FROM
    (
        SELECT colour.value as title
        FROM orderrows oa
        JOIN order o ON o.ID = oa.orderID
        JOIN products a ON oa.productid = a.ID
        JOIN productparameter as colour ON colour.productid = a.ID AND colour.parameterid = 'colour_id'
        WHERE o.orderdate > NOW() - INTERVAL 1 MONTH
        GROUP BY colour.value
        ORDER BY SUM(oa.productprice * oa.productamount) DESC
    ) colour,
    (
        SELECT @rowId:=0
    ) t2
) as colour_sort ON colour.value = colour_sort.title
JOIN
(
    SELECT @rowId:=@rowId+1 AS rowId, type.title
    FROM
    (
        SELECT type.value as title
        FROM orderrows oa
        JOIN order o ON o.ID = oa.orderID
        JOIN products a ON oa.productid = a.ID
        JOIN productparameter as type ON type.productid = a.ID AND type.parameterid = 'type_id'
        WHERE o.orderdate > NOW() - INTERVAL 1 MONTH
        GROUP BY type.value
        ORDER BY SUM(oa.productprice * oa.productamount) DESC
    ) type,
    (
        SELECT @rowId:=0
    ) t2
) as type_sort ON type.value = type_sort.title
WHERE o.orderdate > NOW() - INTERVAL 1 MONTH
GROUP BY a.ID
ORDER BY collection_sort.rowId ASC, colour_sort.rowId ASC, type_sort.rowId ASC

a as alias for products is not the best choice.
You don't use anything from products in your subqueries. So you don't need to join it.
If you only need the productID, you can also skip the products table in the outer query and use oa.productid instead.
You don't need a rowId - You can use the SUM to order the result.

So you query could look like:

SELECT p.ID, collection.value as title, colour.value as colour, type.value as type
FROM orderrows oa
JOIN order o ON o.ID = oa.orderID
JOIN products p ON oa.productid = p.ID
JOIN productparameter as collection ON collection.productid = p.ID AND collection.parameterid = 'collection_id'
JOIN productparameter as type ON type.productid = p.ID AND type.parameterid = 'type_id'
JOIN productparameter as colour ON colour.productid = p.ID AND colour.parameterid = 'colour_id'
JOIN
(
    SELECT collection.value, SUM(oa.productprice * oa.productamount) as total
    FROM orderrows oa
    JOIN order o ON o.ID = oa.orderID
    JOIN productparameter as collection ON collection.productid = oa.productid AND collection.parameterid = 'collection_id'
    WHERE o.orderdate > NOW() - INTERVAL 1 MONTH
    GROUP BY collection.value
) as collection_sort ON collection.value = collection_sort.value
JOIN
(
    SELECT colour.value, SUM(oa.productprice * oa.productamount) as total
    FROM orderrows oa
    JOIN order o ON o.ID = oa.orderID
    JOIN productparameter as colour ON colour.productid = oa.productid AND colour.parameterid = 'colour_id'
    WHERE o.orderdate > NOW() - INTERVAL 1 MONTH
    GROUP BY colour.value
) as colour_sort ON colour.value = colour_sort.value
JOIN
(
    SELECT type.value, SUM(oa.productprice * oa.productamount) as total
    FROM orderrows oa
    JOIN order o ON o.ID = oa.orderID
    JOIN productparameter as type ON type.productid = oa.productid AND type.parameterid = 'type_id'
    WHERE o.orderdate > NOW() - INTERVAL 1 MONTH
    GROUP BY type.value
) as type_sort ON type.value = type_sort.value
WHERE o.orderdate > NOW() - INTERVAL 1 MONTH
GROUP BY a.ID
ORDER BY collection_sort.total DESC, colour_sort.total DESC, type_sort.total DESC

That are 37 lines of code instead of 64.

But there are still three subqueries which are almost equal. You can merge them to this one:

SELECT pp.parameterid, pp.value, SUM(oa.productprice * oa.productamount) as total
FROM orderrows oa
JOIN `order` o ON o.ID = oa.orderID
JOIN productparameter as pp ON pp.productid = oa.productid
WHERE o.orderdate > NOW() - INTERVAL 1 MONTH
  AND collection.parameterid IN ('collection_id', 'colour_id', 'type_id')
GROUP BY pp.parameterid, pp.value

and use conditional aggregation to pivot the result in the outer query:

SELECT
    p.ID,
    MAX(CASE WHEN t.parameterid = 'collection_id' THEN t.value END) as collection,
    MAX(CASE WHEN t.parameterid = 'colour_id'     THEN t.value END) as colour,
    MAX(CASE WHEN t.parameterid = 'type_id'       THEN t.value END) as `type`
FROM (
    SELECT pp.parameterid, pp.value, SUM(oa.productprice * oa.productamount) as total
    FROM orderrows oa
    JOIN `order` o ON o.ID = oa.orderID
    JOIN productparameter as pp ON pp.productid = oa.productid
    WHERE o.orderdate > NOW() - INTERVAL 1 MONTH
      AND collection.parameterid IN ('collection_id', 'colour_id', 'type_id')
    GROUP BY pp.parameterid, pp.value
) t
JOIN productparameter pp ON pp.parameterid = t.parameterid
JOIN products p ON p.ID = pp.productid
JOIN orderrows oa ON oa.productid = p.ID
JOIN `order` o ON o.ID = oa.orderID
WHERE o.orderdate > NOW() - INTERVAL 1 MONTH
GROUP BY p.ID
ORDER BY
    MAX(CASE WHEN t.parameterid = 'collection_id' THEN t.total END) DESC,
    MAX(CASE WHEN t.parameterid = 'colour_id'     THEN t.total END) DESC,
    MAX(CASE WHEN t.parameterid = 'type_id'       THEN t.total END) DESC

(24 lines)


Work with nested task I have Client and Server classes.

The Server return a ResultWithNestedTask instance.

public class ResultWithNestedTask
{
    public Task<String> NestedTask;
    public string Result;
}

The Client displays the data from Result, then waits for the completion of NestedTask and displays the result from NestedTask.

The Server:

Starts a new Task and waits for the result
Starts a nested task and returns the task to the client
Waits for the nested task to complete before doing some work with result.

Server's method DoWorkAsync:

    static async Task<ResultWithNestedTask> DoWorkAsync()
    {
        // start new Task and wait result
        var resultTask = await Task<String>.Factory.StartNew(() =>
        {
            return "Result task.";
        });

        //start neasted task
        var resultNestedTask = Task<String>.Factory.StartNew(() =>
        {
            Task.Delay(TimeSpan.FromSeconds(5)).Wait(); //wait while return data
            return "Result neasted task.";
        });

        //do some work whith nested task
        Task.Factory.StartNew(() =>
            {
                resultNestedTask.Wait();
                var resultFromNestedTask = resultNestedTask.Result;
                resultFromNestedTask = "Updated string";
                Console.WriteLine(resultFromNestedTask);
            });

        //return result string and neasted task to the client
        return new ResultWithNestedTask()
        {
            NestedTask = resultNestedTask,
            Result = resultTask,
        };
    }

Client:

var taskResult = await DoWorkAsync();

// writeline result string
Console.WriteLine(taskResult.Result);

// client wait NestedTask
var nestedTask = await taskResult.NestedTask;

// writeline result string from nested task
Console.WriteLine(nestedTask);

This code should not block the UI.

The server returns the nested task and doesn't wait for the nested task to finish.

 //do some work with the nested task
 Task.Factory.StartNew(() =>
 {
     resultNestedTask.Wait();
     var resultFromNestedTask = resultNestedTask.Result;
     resultFromNestedTask = "Updated string";
     Console.WriteLine(resultFromNestedTask);
 });

I want to return the result before the task completes.

Client and Server are abstractions, and just one class takes data from another class.

Is this normal code? Do you see any problems?

Instead of using Task<string>.Factory.StartNew you can do Task.Factory.StartNew.
However, instead of using Task.Factory.StartNew you can use Task.Run. It is simpler and supports more advanced scenarios. See here.


PHP include page index.php?page=GET safety preg_replace I found a lot of working codes to include page in page on Internet but I could not find a safe code. So I decided to create one myself. The pages will be only stored in folder /pages/ and whitelist seems to be a good option.

Is the follow code safe?

<?php
$unsafe = $_GET['pagename'];
$page = preg_replace('/[^A-Za-z0-9\-]/', '', $unsafe);
if (empty($page)){
include('pages/default.php');
}
$pages = array('default', 'pageone', 'pagetwo', 'another', 'last');
if (isset($pages[$page])) {
    include('pages/' . $page . '.php');
} else {
include('pages/error-404.php');
}
?>

You are checking if a certain key exists in an array: isset($pages[$page]) but you define the $pages array without keys, so they default to numeric ones. A much better approach is to skip the preg_replace thing (it is useless anyway) and create a router array:

$router = array(
    'url' => 'pages/foobar.php',
    'default' => 'pages/default.php'
);

This array maps urls to your internal pages. Then you check or the requested page exists:

if (isset($router[$_GET['page']]) {
    include $router[$_GET['page'];
} else {
    include $router['default'];
}


Calculating the speed of a bicycle given the gear size and cadence The following question was taken from Absolute Java 5th ed. by Walter Savitch:

Bicyclists can calculate their speed if the gear size and cadence is known.  Gear size refers to the effective diameter of the wheel. Gear size multiplied by pi (3.14) gives the distance traveled with one turn of the pedals. Cadence  refers to the number of pedal revolutions per minute (rpm).
The speed in miles per hour is calculated by the following:
Speed(mph)=  Gear Size (inches) * pi * 1(ft)/12 (inches) * 1(mile)/5280 (ft) * Cadence (rpm) * 60 (minutes)/(hour)

This is a program that calculates the speed for a gear size of 100 inches and a  cadence of 90 rpm. This would be considered a high cadence and a maximum gear size for a typical bicycle. In writing your program, don’t forget that the expression 1/12 will result in 0, because both 1 and 12 are integers, and when the integer division is performed, the fractional part is discarded.

This is the code that I have written:
    public class Question6 {

    private static final double PI = 3.14;
    private static final double gearSize = 100;
    private static final double cadence = 90;

    public static void main(String[] args) {
        System.out.println(getSpeed() + " mph");
    }

    private static double distancePerRevolution() {
        return gearSize * PI;
    }

    private static double feetPerInches() {
        return 1.0 / 12;
    }

    private static double milesPerFeet() {
        return 1.0 / 5280;
    }

    private static double minutesPerHour() {
        return 60;
    }

    private static double getSpeed() {
        return distancePerRevolution() * feetPerInches() * milesPerFeet()
                * cadence * minutesPerHour();
    }
}

Your question here is again an improvement over your previous version, but, I was hoping you would take some of the abstraction suggestions and apply them.

As it stands, your program is not a general purpose program, rather it is a complex constant value...

Let's look at the things that are actual constants:

PI - 3.14 (though, for more accuracy, use Math.PI constant).
12 inches in a foot
5280 feet in a mile
60 minutes per hour

Those are your constants, and should be declared as private static final.

The things which are variables, are the cadence, and the 'gear size'.

A general purpose program would have a method that takes those two variables, and returns a speed....:

public static double calculateSpeed(double gearSize, double cadence) {...}

That method would be reusable, and general, and it exposes the right amount of functionality without exposing the inner workings of the class.

In your main method, if you would call:

System.out.println(calculateSpeed(100, 90) + "mph");

or rather, you would get the 100 and the 90 from somewhere else, perhaps a constant for the purpose of this exercise.

Now, how do you implement the calculateSpeed function? That is where the smarts of your program should be, and it should perhaps call other private methods, or do some basic calculations.

There are some style issues I see, but the most significant is that I always recommend using decimal-style typed values when you are using a double literal in your code. You have code like:

private static double feetPerInches() {
    return 1.0 / 12;
}

That produces the right results, but I recommend expressing the 12 as 12.0 to make the implicit conversions absolutely clear. I have seen too many times when bugs were introduced by integer arithmetic when floating-point was expected.

Bottom line, I would expect your program to look like:

private static final double PI = 3.14;
private static final double MINS_PER_HOUR = 60.0;
private static final double FEET_PER_MILE = 5280.0;
private static final double INCHES_PER_FOOT = 12.0;
private static final double INCHES_PER_MILE = FEET_PER_MILE * INCHES_PER_FOOT;

public static final double getSpeed(double gearSize, double cadence) {
    ....
}

public static void main(String[] args) {
    System.out.printf("%.3fmph%n", getSpeed(100.0, 90.0));
}

Note, I have used a printf at the end to simplify the logic of the output. Well, it is simpler for me, but, I thought you should see how printfs are used in Java. What that printf says, is print a floating-point value with 3 decimal places (%.3f), followed by 'mph', followed by a newline %n. The floating value to print is the speed.


SQL query with dynamic unpivot+pivot for cross product In this Data Explorer query I am trying to do the following:

For each tag:

Compute sum of answer scores in this tag ("S")
Compute count of answers in this tag ("A")
For each tag class (Bronze, Silver, Gold)
add two columns:

S divided by this class's score goal
A divided by this class's answer goal

I wanted to do this in the most general way possible, allowing more tag classes/goals to be added later. I came up with this:

-- Predefined tag badge goals
... TagBadges as (
  select * from
    (values
      (1, 'Bronze', 100, 20),
      (2, 'Silver', 400, 80),
      (3, 'Gold', 1000, 200))
    as Badge(Idx, Class, Score, Answers)
),

-- Progress per tag, per badge class
TypeProgress as (
  select
    RawData.TagName,
    format(iif(RawData.Score > TagBadges.Score, 1,
               cast(RawData.Score as float)/TagBadges.Score), '#0.#%') as Score,
    format(iif(RawData.Answers > TagBadges.Answers, 1,
               cast(RawData.Answers as float)/TagBadges.Answers), '#0.#%') as Answers,
    TagBadges.Class
  from RawData cross join TagBadges
),

-- Combine class & type columns
AllProgress as (
  select TagName, Progress, Class+' '+Type as Category
  from TypeProgress
  unpivot (Progress for Type in (Score, Answers)) p
) ...

But in the end I still had to list all the cases (2 × 3 = 6) explicitly:

select *
from AllProgress
pivot (
  max(Progress) for Category in
    ([Bronze Score], [Bronze Answers],
     [Silver Score], [Silver Answers],
     [Gold Score], [Gold Answers])
) q

Is there a better way of doing this?

There is a more general way to do it, but it involves a dynamic query..... which is messy, and may not be better than what you have. Any time you involve a pivot, you are going in to the 'ugly' territory.

First though, some other comments:

you do not include the Rawdata CTE with your question, and it has some small issues I can see in SEDE
You select your data out as formatted strings, and you should only do the formatting as the last step, if possible.

Other than that, I found your query relatively easy to read, and since I understand the SEDE dataset quite well, I could follow the logic nicely.

Basically, your query is good, and I would 'accept it' in a review.

Ugly things of note though are:

max(Progress) is doing character-based max on a single-value set
the order by does a convert-to-float of a character value

I would like those things to be commented on, but, since the data volumes for those operations are small, I think it is OK, given the circumstances.

Out of interest, I processed your query using a more general syntax, that avoids the unpivot, and makes the pivot dynamic. As you will see, it is just as ugly, if not worse.

it does not do the order-by
it does not format the values as string at all.

See it for yourself

Note the multiple temp tables, and the variables for the pivot, and the query. Ugly, but it may give you some ideas.


Text Parsing in C on a microcontroller Microcontroller receive data on uart and call parser_i2c function when user presses enter
e.g. parser_i2c(I2C scan)
I want to parse following input from user communicated through uart
I2C scan
I2C read read_adress
I2C Write Write_address data
I have wrote following code
        void parser_i2c(unsigned char str[20])
    {
       //unsigned char str[20]
       unsigned char s[2] = " ";
       unsigned char *token;
       unsigned char output[10][10] ;
        U8 i_i2c=0;
       /* get the first token */
       token = strtok(str, s);

       /* walk through other tokens */
       while( token != NULL ) {
      //    printf( " %s\n", token );
        strcpy(output[i_i2c],token);

         i_i2c++ ;
          token = strtok(NULL, s);

       }
     if(!(strcmp(output[0],"I2C")))
      {
         if(!(strcmp(output[1],"READ")))
            {
                if(*output[2]!='\0')
                {
                i2c_read(output[2]) ;
                }
                else{

                        printf(COLOR_RED "Bad command" COLOR_RESET);

                }
            }
         else if(!(strcmp(output[1],"WRITE")))
         {
            if(((*output[2])||(*output[3]))!='\0')
            {
            i2c_write(output[2],output[3]);
            }
            else{

                        printf(COLOR_RED "Bad Command" COLOR_RESET);

            }
         }

         else if(!(strcmp(output[1],"SCAN")))
         {
         i2c_scan();
         }
         else{

                        printf(COLOR_RED "Bad Command" COLOR_RESET);

         }

      }
      else {
                            printf(COLOR_RED "Bad Command" COLOR_RESET);

      }
memset(output, '\0', 10*sizeof(arr[0]));

    }

How to improve it ? in terms of reducing code lines and code size and memory utilization ? or any other way to parse ? Is there any basic library available ?

Here are some things that may help you improve your program.
Use consistent formatting
The code as posted has inconsistent indentation which makes it hard to read and understand. Pick a style and apply it consistently.
Provide complete code to reviewers
This is not so much a change to the code as a change in how you present it to other people.  Without the full context of the code and an example of how to use it, it takes more effort for other people to understand your code.  This affects not only code reviews, but also maintenance of the code in the future, by you or by others.  One good way to address that is by the use of comments.  Another good technique is to include test code showing how your code is intended to be used.
Use const where practical
The s string is set and never altered, so it could be const or even better in this case, static const.
Consider re-entrancy
In an embedded system, there are often many things happening at once.  The usual case is that many events are event-driven and so the order in which parts of code are executed is uncertain.  For that reason, you should be wary about using non-reentrant calls such as strtok.  Also, we don't have the complete context, but it may be worth making a copy of a buffered line of data so that subesequent data the comes in does not overwrite a line as it is being parsed.
Consider passing a length
This code makes the tacit assumption that the passed array is a '\0'-terminated text string.  If you can guarantee that is always the case, then the code is fine as it is, but a more robust system might be to explicitly pass both a pointer and a length.
Understand system variations
It is implementation-defined whether char is signed or unsigned.  If it happens to be signed on your platform, you will get warnings about sign mismatches for parameters passed to strtok and strcpy.  If you system uses unsigned chars by default, you will get no such warning, but you should be aware that this is at least a portability concern.
Simplify your algorithm
This could be simpler and more maintainble in the future if it used a state machine for parsing.  For this simple grammar we have the following valid sentences:

I2C SCAN
I2C READ n
I2C WRITE n n

I am assuming the n is a numeric value, but it's not clear from the context.  Here's the corresponding state machine:

Here's one way to code that:
void parser_i2c(mychar str[20])
{
    static const mychar s[2] = " ";
    enum { start, i2c, r1, w1, w2, error, done } state = start;
    mychar *n1 = NULL;
    for (mychar *token = strtok(str, s); state != done; token = strtok(NULL, s)) {
        switch (state) {
            case start:
                if (strcmp(token, "I2C") == 0) {
                    state = i2c;
                } else {
                    state = error;
                }
                break;
            case i2c:
                if (strcmp(token, "READ") == 0) {
                    state = r1;
                } else if (strcmp(token, "WRITE") == 0) {
                    state = w1;
                } else if (strcmp(token, "SCAN") == 0) {
                    i2c_scan();
                    state = done;
                } else {
                    state = error;
                }
                break;
            case r1:
                /* check for number? */
                i2c_read(token);
                state = done;
                break;
            case w1:
                /* check for number? */
                n1 = token;
                state = w2;
                break;
            case w2:
                /* check for number? */
                i2c_write(n1, token);
                state = done;
                break;
            default:
                printf(COLOR_RED "Illegal state" COLOR_RESET);
                state = done;
        }
        if (state == error) {
            printf(COLOR_RED "Bad Command" COLOR_RESET);
            state = done;
        }
    }
}

I've use mychar as a typedef for char on my machine.  On yours, it looks like you could use unsigned char instead.
Note that now, the flow is very easy to follow and it would not be at all difficult to make an alteration to the grammar to accommodate some other command. Because the tokens are handled as they're parsed, there's no need to store any except the first parsed number which we need for i2c_write(). If it gets any more complex than this, one could use a more sophisticated parser or use a tool like flex.


Simplifying VBA Code I wrote a fully working VBA macro that goes through inputed client ID's and returns the specified commission fee for that client.

The code 100% works, however I need to have it so that my boss is able to easily add new clients with their special commission %'s to the list, without having to go through the VBA code.

How can I make a simplified VBA macro or just type an Excel function to do exactly what my code below does, but make it versitile to the special klient table?

The main Excel table looks like this:

The table with the special klients, commission fees, currency, markets, looks like this:

Option Explicit

Sub komisijas_calc_Click()

'Declare the variables
Dim klienta_nr As Long
Dim ISIN As String
Dim Cena As Double
Dim Skaits As Double
Dim Komisija As Double
Dim vk As String
Dim Summa As Double
Dim kSheet As Worksheet

Dim lngFirstRow As Long: lngFirstRow = 2
Dim lngLastRow  As Long
Dim lngCol      As Long: lngCol = 2
Dim lngCounter  As Long
Dim lngKom As Long: lngKom = 11

Set kSheet = ThisWorkbook.Sheets("spec_klienti")

With Worksheets(1)
    lngLastRow = .Cells(.Rows.Count, lngCol).End(xlUp).Row

    For lngCounter = lngFirstRow To lngLastRow

    klienta_nr = Range("B" & lngCounter).Value
    ISIN = Range("E" & lngCounter).Value
    Cena = Range("H" & lngCounter).Value
    Skaits = Range("I" & lngCounter).Value
    vk = Range("D" & lngCounter).Value
    Summa = Cena * Skaits

Select Case klienta_nr

    Case 111111
                '(Vācija, Francija, Nīderlandes, Itālija, Īrija) - 30 EUR MIN
                If klienta_nr = 111111 And (Left(ISIN, 2) = "DE" Or Left(ISIN, 2) = "FR" Or Left(ISIN, 2) = "NL" Or Left(ISIN, 2) = "IT" Or Left(ISIN, 2) = "IE") Then
                    Komisija = Summa * 0.01
                    .Cells(lngCounter, lngKom) = Komisija
                        End If
                If klienta_nr = 111111 And Komisija <= 30 Then
                    .Cells(lngCounter, lngKom) = 30
                        End If

                If klienta_nr = 1111111 And (Left(ISIN, 2) <> "DE" Or Left(ISIN, 2) <> "FR" Or Left(ISIN, 2) <> "NL" Or Left(ISIN, 2) <> "IT" Or Left(ISIN, 2) <> "IE") Then
                    Komisija = Summa * 0.003
                    If Komisija >= 40 Then
                        .Cells(lngCounter, lngKom) = 40
                            End If
                        End If

    Case 2222222
                '(Vācija, Francija, Nīderlandes, Itālija, Īrija) - 30 EUR MIN
                If klienta_nr = 222222 And (Left(ISIN, 2) = "DE" Or Left(ISIN, 2) = "FR" Or Left(ISIN, 2) = "NL" Or Left(ISIN, 2) = "IT" Or Left(ISIN, 2) = "IE") Then
                    Komisija = Summa * 0.01
                    .Cells(lngCounter, lngKom) = Komisija
                        End If
                'Set 30 EUR Min
                If klienta_nr = 2222222 And Komisija <= 30 Then
                    .Cells(lngCounter, lngKom) = 30
                        End If

                If klienta_nr = 2 And (Left(ISIN, 2) <> "DE" Or Left(ISIN, 2) <> "FR" Or Left(ISIN, 2) <> "NL" Or Left(ISIN, 2) <> "IT" Or Left(ISIN, 2) <> "IE") Then
                    GoTo AllElseFails
                        End If

Case Else

AllElseFails:

              'IP2, 0.03% komisija, 40 EUR/USD Max
                If Right(vk, 1) = 1 Or Right(vk, 1) = 8 Then
                    Komisija = Summa * 0.003
                    .Cells(lngCounter, lngKom) = Komisija
                        End If
              'IP1, 0.1% komisija, 40 EUR/USD Max
                If Right(vk, 1) = 7 Then
                    Komisija = Summa * 0.01
                    .Cells(lngCounter, lngKom) = Komisija
                        End If
                'Komisija MAX is 40, so anything >=40 equals 40
                If Komisija >= 40 Then
                    .Cells(lngCounter, lngKom) = 40
                        End If
            'End If
     End Select
   Next lngCounter
  End With
End Sub

Several observations about the code not necessarily related to the (not so) explicit question:

Formatting

Have some consistent indentation of your code, it will make it much easier to read. Rubberduck is a great tool for that, and it will help you find all sorts of spots for other VBA code improvements. (Happy user, haven't yet contributed to the project.)

Remove excessive vertical spacing as it allows you to fit more of this lengthy procedure on the screen.

Variable Naming

Your first set of variables seem reasonably well named (though I don't speak... whatever language you've named them in :) until... we get to vk - This seems to be the commission percentage so why not call it CommissionPercent?

Your second set of variables, however, leave a bit to be desired in the choice of names.

Drop the Hungarian Notation unless you're going to do it usefully. (Most people don't, so you're not alone!)

FirstRow is just as intuitive a name as lngFirstRow and when you're using it, knowing that it's a Long doesn't add value. This applies to all the lng prefixed variables.

lngCol - when I look at that, I'm going to assume that it's something like "the current column I'm processing", but looking through the code, it's actually a pointer to the Client column, and it never changes, so how about Const CLIENT_COL as Long = 2 instead.

The same can be done with lngKom - make it a Const called I'M_NOT_SURE_COL (Col 11 is col K, but that's a somewhat random column in your "Market" section. Currently only set to "EE", but that could certainly change, I'd think).

Clean up the Select statements

In this code:

Select Case klienta_nr
  Case 111111
  '(Vacija, Francija, Niderlandes, Italija, Irija) - 30 EUR MIN
  If klienta_nr = 111111 And (Left(ISIN, 2) = "DE" Or Left(ISIN, 2) = "FR" Or Left(ISIN, 2) = "NL" Or Left(ISIN, 2) = "IT" Or Left(ISIN, 2) = "IE") Then

The If klienta_nr = 111111 And is totally unnecessary. Because you're inside the Select Case and this is Case 111111, klienta_nr will, by definition, be 111111. Remove that part to simplify the code a bit.

Actually, eliminate all the "Magic Numbers" then clean up the Select statements

In your Select you have Case 111111 (6 ones). Inside that Case, you have If 111111 (6 ones), followed by If 111111 (6 ones), followed by If 1111111 (7 ones). This appears, to the innocent bystander, to be a typo on the last If statement. However, since all the rest of the conditions are the same, maybe it's intentional. In either case, it's confusing for the next programmer to come along (where "Next Programmer" is often "Future You"). If you declare some Const ABC_CLIENT_ID as Long = 111111 and Const DEF_CLIENT_ID as Long = 1111111, at least "next programmer" will know that this isn't a typo since you've intentionally declared 2 separate constants for the different values.

Be aware that your code will never fall into the 3rd If statement. By definition, when the code falls into the Case 111111 (6 ones) section of the Select, If klienta_nr = 1111111 (7 ones) will never be True. This is dead code (as written) and should be removed, or the typo fixed to make it do what you intend.

The same can be said for the Case 2222222 (7 twos) section. The only If that will ever execute is the middle one under the 'Set 30 EUR Min comment. The first If only has 6 twos, which will never evaluate to True and the last has only 1 two.

Eliminate GoTo

The good news about that last If statement never executing is that you'll never hit the GoTo AllElseFails jump! If you really need to execute the AllElseFails code at some point in the Case 2222222 section, then refactor your code like this:

Private Sub AllElseFails(ByVal vk As String, ByRef Komisija As Double, ByVal lngCounter As Long, ByVal lngKom As Long, ByVal Summa As Double, ByVal WkSht As Worksheet)

  'IP2, 0.03% komisija, 40 EUR/USD Max
  If Right(vk, 1) = 1 Or Right(vk, 1) = 8 Then
    Komisija = Summa * 0.003
    WkSht.Cells(lngCounter, lngKom) = Komisija
  End If
  'IP1, 0.1% komisija, 40 EUR/USD Max
  If Right(vk, 1) = 7 Then
    Komisija = Summa * 0.01
    WkSht.Cells(lngCounter, lngKom) = Komisija
  End If
  'Komisija MAX is 40, so anything >=40 equals 40
  If Komisija >= 40 Then
    WkSht.Cells(lngCounter, lngKom) = 40
  End If
End Sub

And call AllElseFails when you need that code instead of jumping around in a Select Case structure like this:

      If klienta_nr = 2 And (Left(ISIN, 2) <> "DE" Or Left(ISIN, 2) <> "FR" Or Left(ISIN, 2) <> "NL" Or Left(ISIN, 2) <> "IT" Or Left(ISIN, 2) <> "IE") Then
        AllElseFails vk, Komisija, lngCounter, lngKom, Summa, Worksheet(1)
      End If
    Case Else
      AllElseFails vk, Komisija, lngCounter, lngKom, Summa, Worksheet(1)

That's the way to reuse the code without getting the "next programmer" very confused.

As an added tweak to the refactored Function, you have:

  If Right(vk, 1) = 1 Or Right(vk, 1) = 8 Then

followed by

  If Right(vk, 1) = 7 Then

You should probably change that second If statement to an ElseIf. There's no need to execute the check for 7 if you've already determined it's a 1 or 8.

Refactor to eliminate redundant code:

You have

(Left(ISIN, 2) = "DE" Or Left(ISIN, 2) = "FR" Or Left(ISIN, 2) = "NL" Or Left(ISIN, 2) = "IT" Or Left(ISIN, 2) = "IE")

all over the place. This is a great opportunity to create a Function that does all that testing for you and returns a Boolean:

Private Function IsADesiredISIN(ByVal ISIN As String) As Boolean

    Select Case ISIN
    Case "DE", "FR", "NL", "IT", "IE"
      IsADesiredISIN = True
    Case Else
      IsADesiredISIN = False
    End Select

End Function

Then your if statements become:

If klienta_nr = 111111 And IsADesiredISIN(Left(ISIN, 2)) Then
If klienta_nr = 222222 And IsADesiredISIN(Left(ISIN, 2)) Then

Which should really be

If IsADesiredISIN(Left(ISIN, 2)) Then

since the klienta_nr = 111111 and klienta_nr = 222222 are redundant (as mentioned above).

And, as an added bonus, if you need to include a new country code in the list of ISIN you're processing, you do it in one place and it effects all locations meaning you don't accidentally forget one.

This really hasn't addressed your actual question (I don't think), but it does simplify the code by eliminating some questions that the "next programmer" may have about what the code does and why.


C++ remove from singly linked list I asked a similar question (C++ linked list) but I'd like to have this similar code reviewed. It's not the same code, just slightly different and with bugs fixed.

The code takes the head pointer of a singly linked list and an int element to find and remove from the list:

void removeFromList(node* &head, int toBeRemoved){
    if (head != nullptr){
        node* temp = head;
        if (head->data == toBeRemoved){
            head = head->next;
            delete temp;
        }
        else{
            while (temp->next != nullptr && temp->next->data !=toBeRemoved)
                temp = temp->next;

            if (temp->next == nullptr)
                //some code to be executed when element not found
                else{
                if (temp->next->data == toBeRemoved){
                    node* removeThis = temp->next;
                    temp->next = temp->next->next;
                    delete removeThis;
                }
            }
        }
    }
    else
        //some code to be executed when list is empty
}

I've tested it by outputting messages to the console in case the element is not found or the list is empty, and it seems to be working in every case.

Still, I'd like to know if the code can be improved (not trying to use the C++ list implementation, just to rewrite this in the best way).

Design

You are obviously not using classes (as this is a free function). This means you are exposing the implementation details of the linked list to the user. This is bad as you can not guarantee any of the invariants of the list.

Invariants:

All the nodes were allocated with new.
Does your list use sentinels? (not at the moment see below).
(Sure I could come up with more).

Personally I would wrap this up in a class where the user never sees a node object.

Code Re-Use

You have repeated chunks of code that do the same things.

void removeFromList(node* &head, int toBeRemoved){
    // STUFF
        if (head->data == toBeRemoved){
            head = head->next;
            delete temp;
        }
    // STUFF
                    temp->next = temp->next->next;
                    delete removeThis;
                }

You should isolate code that does the same thing in to its own function/method. Then use a decent function name so the code becomes self documenting.

Sentinel

When writting list it is sometimes useful to have a sentinel node. This is a special node in the list that does not carry any data. But it makes manipulating the list easy as you no longer need to worry about a null value (or empty list). Thus the head no longer becomes a special case.

Example: A sentinel list will always contain one item at the head (this head item contains no data). The rest of the API should be coded with the understanding of the sentinel value. So en empty list contains only the sentinel value.

With this assumption we can simplify the code too:

void removeFromList(node* head, int toBeRemoved){
    node* temp = head;
    while (temp->next != nullptr && temp->next->data !=toBeRemoved) {
        temp = temp->next;
    }
    if (temp->next == nullptr && temp->next->data == toBeRemoved){
        node* removeThis = temp->next;
        temp->next = temp->next->next;
        delete removeThis;
    }
}

Use Braces

You don't consistently use braces. This is going to come back and haunt you one day. Any nested statement should have braces (even if it is a single item). Doing this one thing will save you from so many bugs.

            if (temp->next == nullptr)
                //some code to be executed when element not found
                else{
                }

Just add the braces after the if(). Also align your code better.

            if (temp->next == nullptr) {
                //some code to be executed when element not found
            }
            else {
                // Other stuff.
            }

Types and names

In C++ the most important part of the program is the type information. There is nothing more important than types. Get your types correct and you are 85% there in a C++ program.

As a result it is useful to be able to distinguish types from objects in the code. So a common convention is to use a naming scheme. A very common naming scheme is that user defined types have an initial uppercase letter while objects (variables/methods/functions) have an initial lower case letter.

// So
Node*  item;
// rather than
node*  item;

While we are taking about types. The '*' and the '&' are part of the type information so they go with the type not the object. This is the inverse of how C programmers do it.

// So
Node*&  head
// Rather than
node*   &head

Note: Notice how the SO code block picks up the type. This is a reflection of common conventions used in C++.

Re-Design

class LinkedList
{
    struct Node
    {
        Node* next;
        virtual ~Node(){}
    };
    struct DataNode: public Node
    {
        DataNode(Node* next, int val)
            : Node(next)
            , data(val)
        {}
        int    data;
    }

    Node  sentinel;    // Notice I don't dynamically allocate this.

    public:
        ~LinkedList()
        {
            Node* next;
            for(Node* loop = sentinel.next; loop; loop = next) {
                next = loop->next;
                delete loop;
            }
        }
        LinkedList()
            : sentinel{}  // forces zero-initialization
        {}
        // Delete compiler generated methods.
        LinkedList(LinkedList const&)            = delete;
        LinkedList& operator=(LinkedList const&) = delete;

        void add(int val)
        {
            sentinel.next = new DataNode{sentinel.next, val};
        }
        void del(int val)
        {
            for(Node* loop = &sentinel; loop->next; loop=loop->next) {
                if (dynamic_cast<DataNode*>(loop->next)->data == val) {
                    Node* del = loop->next;
                    loop->next = del->next;
                    delete del;
                    break;
                }
            }
        }
};

To see an example of a fully implemented doubly linked list check out this answer.