Re-implementation of `std::tuple` for MCUs Background
I'm writing code for an 8-bit MCU with very limited RAM (1KiB) and program flash space (64KiB). I have a C++14 capable compiler with no standard library implementation available. This is a good enough excuse for me to do some recreational template programming. :)
In the project where I intend to use this I need to be able to create a densely packed, complex, configurable tuple of varying types and values. And I need this data structure to be stored in program space, so I need it to be usable as constexpr.
About the code
In the code below I have used uint8_t for the size type because the MCU has 8-bit words and an int (which has to be 16 bits to be standard compliant) thus is "extended precision" for my MCU and incurs speed and program size overhead. And frankly, with 1K of ram I'm not expecting to make tuples with more than 255 elements.
The code is put into a single file for your reviewing pleasure and uses std::forward which is technically not available on my target, but I intend to re-implement it as the next step. This code is written for PC first to be able to debug and test it.
I'm happy for any comments on suggestions on the code, but please bear in mind the target architecture.
tuple_test.cpp
#include <iostream>
#include <type_traits>

//#define PACKED_TUPLES

#ifdef PACKED_TUPLES
#define ATTRIBUTE_PACKED __attribute__((__packed__))
#else
#define ATTRIBUTE_PACKED
#endif

namespace xtd {
  namespace detail {

    template <typename...>
    struct ATTRIBUTE_PACKED pack;

    // Helper class to get a value from a pack.
    // This is required as template functions cannot be
    // partially specialised.
    template <int, typename...>
    struct pack_get;

    template <typename... types>
    struct pack_get<0, types...> {
      static auto get(const pack<types...>& pack) { return pack.value; }
    };

    template <int i, typename first_type, typename second_type, typename... types>
    struct pack_get<i, first_type, second_type, types...> {
      static auto get(const pack<first_type, second_type, types...>& pack) {
        return pack_get<i - 1, second_type, types...>::get(pack.next);
      }
    };

    template <typename type>
    struct ATTRIBUTE_PACKED pack<type> {
      constexpr static int size() { return 1; }

      constexpr pack() = default;
      constexpr pack(type arg) : value(arg) {}

    private:
      const type value;

      template <int, typename...>
      friend struct pack_get;
    };

    template <typename type, typename... types>
    struct ATTRIBUTE_PACKED pack<type, types...> {
      using next_pack = pack<types...>;

      constexpr pack() = default;
      constexpr pack(const type& arg, types&&... args)
          : value(arg), next(std::forward<types>(args)...) {}

      constexpr static int size() { return 1 + next_pack::size(); }

    private:
      const type value;
      const next_pack next;

      template <int, typename...>
      friend struct pack_get;
    };
  }  // namespace detail

  template <typename... types>
  using tuple = detail::pack<types...>;

  template <int i, typename... types>
  auto get(const tuple<types...>& tuple) {
    return detail::pack_get<i, types...>().get(tuple);
  }

  template <typename... types>
  constexpr auto make_tuple(types&&... args) {
    return tuple<types...>(std::forward<types>(args)...);
  }
}  // namespace xtd

extern constexpr auto p = xtd::make_tuple(3, 3.14, "fooo");

int main(int, char**) {
  std::cout << sizeof(p) << std::endl;
  std::cout << p.size() << std::endl;
  auto x = xtd::get<2>(p);
  std::cout << x << std::endl;
}

Good stuff!
improvements:
Your constructors should move the value.
constexpr pack(type arg) : value(arg) {}

vs
constexpr pack(type arg) : value(std::move(arg)) {}

Inconsistent constructor parameter:
Your pack<...> template's constructor's first argument is by reference, whereas your root template takes it by value. Be consistent, pick one. (the one by value with a move is the correct choice here)
This is not actually forwarding:
constexpr pack(const type& arg, types&&... args)
          : value(arg), next(std::forward<types>(args)...) {}

Forwarding is needed when the template deduction can deduce the type of a parameter to be a reference. I.E. the arguments themselves need to be templated. That's not the case here.
You can simply use:
constexpr pack(type arg, types... args)
          : value(std::move(arg)), next(std::move(args)...) {}

Note that your make_tuple() function DOES perform forwarding, so that one is fine.
Get by reference, please.
Your get function returns by value, when a reference would make more sense:
auto get(const tuple<types...>& tuple) {}

vs
const auto& get(const tuple<types...>& tuple) {}
auto& get(tuple<types...>& tuple) {}

make_tuple() should decay the type of its arguments.
Specifically, you should apply std::decay on a per-arg basis so that make_tuple(int&, const float&) returns a tuple<int, float>.
No EBO
Since you seem to care about packing, you should really be performing empty base class optimisation to get the maximum oomph out of your tuple class. This does make the code a lot more complicated though, so if you know for a fact that you'll never pack 0-sized types in a tuple, then I wouldn't bother.
A note on PACKED_TUPLES:
Making this a compile-time switch like this is pretty dicey, as you can eventually run into some nasty binary incompatibilities in the future. I would rather make a complete separate class called packed_tuple<>, and use the compile-time switch in higher-level code so that symbols related to tuple<> and packed_tuple<> remain consistent between binaries. It also allows you to mix packed and unpacked tuples in the same binary which is potentially convenient, since __attribute__((__packed__)) can have a substantial hit on performance.


Remove (not extract) timestamp from HTML string I've got strings coming from irregularly and ugly formatted HTML sites, that contain a timestamp. I am interested in removing the timestamp entirely and get all the rest.

from bs4 import BeautifulSoup

date1 = '
10:15 AM  ARVIND KRISHNAMURTHY, Northwestern University
'
date2 = """

 8:45 a.m.<o:p></o:p>

RICARDO  CABALLERO, MIT and NBER<o:p></o:p>

"""

soup1 = BeautifulSoup(date1)
print repr(soup1.text.strip())
# "u'10:15 AM  ARVIND KRISHNAMURTHY, Northwestern University'"
soup2 = BeautifulSoup(date2)
print repr(soup2.text.strip())
# "u'8:45 a.m.RICARDO  CABALLERO, MIT and NBER'"

Now, to get the text following the timestamp, I split along whitespaces to join all elements, except the first two:

def remove_date(aString):
    cleaned = aString.replace("\t", " ").replace(".m.", " ").strip()
    return " ".join(cleaned.split(" ")[ 2:]).strip()

string1 = remove_date(soup1.text.strip())
print repr(string1)
# u'ARVIND KRISHNAMURTHY, Northwestern University'
string2 = remove_date(soup2.text.strip())
print repr(string2)
# u'RICARDO  CABALLERO, MIT and NBER'

It delivers the desired result, but this is arguably very ugly. Is there something better? Maybe something that works like dateutil.parser.parse(), but in reverse?

You're right that it's ugly, I thought it didn't work at first until I realised how you're splitting to remove the first two whitespace separated strings. It's very unclear and very specific to these formats. If you're going to be format specific, then shouldn't you just use a regex?

r'^\d+:\d+\s+(am|pm|a\.m\.|p\.m\.)' will match the format in both the provided cases.

A brief breakdown is that it will match digits, then a colon, then digits again, then whitespace followed by either am, pm, a.m. or p.m.. You can make it case insensitive to avoid manually marking upper cases too. Also the ^ at the start means it will only work on strings that have this pattern at the beginning, so it wouldn't affect strings that happen to include similar formats as part of the text. I used this site for figuring out this regex and it's very helpful for stuff like this.

You can compile this regex simply with this:

import re

pattern = re.compile(ur'^\d+:\d+\s+(am|pm|a\.m\.|p\.m\.)', re.IGNORECASE)

Then just call it on your stripped soup text:

string1 = re.sub(pattern, "", soup1.text.strip())
print repr(string1)
string2 = re.sub(pattern, "", soup2.text.strip())
print repr(string2)


User session helper class I write this session helper class to use it inside my projects for managing the $_SESSION variables setup after an user login or logout. It's very simple and after some tests it seems to work smoothly and fine.

The class doesn't have a constructor, this because the needed parameters that are the username and the user id are passed directly to the setSession method.

The sessionCode method is instead only a code who is used to check if the user is logged in or not, this to limit the access to certain pages if needed.

<?php
namespace library;

class SessionHelper{

private $username;
private $id;
private $ip;

public function setSession(string $email,int $id){
    session_regenerate_id();
    $_SESSION['session_code'] = $this->sessionCode();
    $_SESSION['user_ip'] = $_SERVER['REMOTE_ADDR'];
    $_SESSION['user_id'] = $id;
    $_SESSION['username'] = $email;

    return true;
}

public function sessionStatus(){
    if(isset($_SESSION['session_code'])){
        if(hash_equals($_SESSION['session_code'], $this->sessionCode())){
          return true;
        } else {
          return false;
        }
    }
}

public function unsetSession(){
    session_unset();
    session_destroy();
    return true;
}

private function sessionCode(){
    return hash('sha256', session_id());
}

}

?>

USAGE EXAMPLE AFTER A LOGIN SCRIPT:

<?php
require_once 'SessionHelper.php';

use library\SessionHelper as SessionHelper;

$session = new SessionHelper;

$session->setSession('user1', '4');

?>

USAGE ON RESTRICTED ACCESS PAGES

<?php
session_start();

require_once 'library/Autoloader.php';

use library\SessionHelper as SessionHelper;

$session = new SessionHelper;

if($session->sessionStatus() != true){
  header('Location: index');
  die();
}
?>

This is not a SessionHelper but a UserSessionHelper in the first place. A session is a container that could contain anything, not only user credentials. A shopping cart items for example. Therefore you have to either extend its functionality or at least rename a class.

In 2018 you are obligatory supposed to utilize a PSR-4: Autoloader so not to include your class definitions manually.

What is most important, I don't see any getSession() method. Are you checking $_SESSION array directly? If so, that makes you class essentially incomplete. When writing OOP, you must encapsulate the full functionality, leaving something outside just makes no sense.

Given there is no constructor, consider using static methods, it will save you the very absent constructor call.


Train reservation system in Rust as my practice Abstract

I am a beginner of Rust language. As my practice I want to create simple train reservation system.

I know little about the "best practice" in Rust, so I want some advises in my code. In terms of the function there are much luck to improve, but I want advises about how to write codes "in Rust".

Code structure

reservation
├── Cargo.lock
├── Cargo.toml
├── src
│   ├── lib.rs
│   ├── main.rs
│   └── reserve_request
│       └── mod.rs
└── target

My codes

lib.rs

pub mod reserve_request;

reserve_request/mod.rs

use std::collections::HashMap;

#[derive(PartialEq, Eq, Hash)]
pub struct Stations<'a> {
    pub start: &'a str,
    pub destination: &'a str,
}

pub struct Request<'a> {
    pub start: &'a str,
    pub destination: &'a str,
    pub time: &'a str,
    pub time_kind: &'a str,
}

impl<'a> Request<'a> {
    pub fn reserve(&self, timetable: HashMap<Stations, &[&str]>) -> bool {
        let st = Stations {
            start: self.start,
            destination: self.destination,
        };

        let times = timetable.get(&st);
        let default: &[&str] = &[];
        let result = match times {
            Some(r) => r,
            None => default,
        };

        for t in result {
            if *t == self.time {
                return true;
            }
        }

        false
    }

    pub fn is_valid(&self) -> bool {
        if self.start == "" {
            println!("You need to determine start point");
            return false;
        } else if self.destination == "" {
            println!("You need to determine destination");
            return false;
        } else if self.start == self.destination {
            println!("Invalid. The start point and destication is same");
        } else if self.time == "" && self.time_kind != "" {
            println!("Invalid time specification");
            return false;
        }

        true
    }
}

main.rs

use reserve::reserve_request;
use std::collections::HashMap;

fn main() {
    //TODO: save time table in the json file
    let time: &[&str] = &["12:00", "14:00", "18:00", "19:00"];
    let mut stations = HashMap::new();
    stations.insert(
        reserve_request::Stations {
            start: "Tokyo",
            destination: "Kyoto",
        },
        time,
    );

    let request = reserve_request::Request {
        start: "Tokyo",
        destination: "Kyoto",
        time: "18:00",
        time_kind: "start",
    };

    if request.is_valid() && request.reserve(stations) {
        println!("Succeed in reserving that train");
    } else {
        println!("Failed to reserved that train");
    }
}

What I want to know especially

In main.rs, to make a value of the HashMap (type &[&str]), I define time variable and insert into HashMap. I feel it is redundant but I do not know how to deal with.
Similar to above, in mod.rs, I make default variable for using in match clause only.

let time: &[&str] = &["12:00", "14:00", "18:00", "19:00"];
let mut stations = HashMap::new();
stations.insert(
    reserve_request::Stations {
        start: "Tokyo",
        destination: "Kyoto",
    },
    time,
);

Ok, I assume you tried putting the value assigned to time directly in the expression, but were unable to resolve the resulting error message. The problem is that expression has the type [&str; 4], an array. But you want a slice: &[&str] instead. There a few ways to resolve this:

Firstly, you can specify the type of the HashMap:

let mut stations: HashMap<reserve_request::Stations, &[&str]> = HashMap::new();
stations.insert(
    reserve_request::Stations {
        start: "Tokyo",
        destination: "Kyoto",
    },
    &["12:00", "14:00", "18:00", "19:00"]
);

This way, Rust will infer what type you actually wanted at convert the reference to an array into a slice.

Alternatively, you can manually request a slice by called .as_ref() on the array:

let mut stations = HashMap::new();
stations.insert(
    reserve_request::Stations {
        start: "Tokyo",
        destination: "Kyoto",
    },
    ["12:00", "14:00", "18:00", "19:00"].as_ref()
);

However, a better approach would be to not use arrays at all. Rust arrays are really just not that useful, and you almost always should prefer to use a Vec. Furthermore, if you implement loading from JSON as your comment suggests, then you'll certainly get a Vec and not a slice.

let mut stations = HashMap::new();
stations.insert(
    reserve_request::Stations {
        start: "Tokyo",
        destination: "Kyoto",
    },
    vec!["12:00", "14:00", "18:00", "19:00"]
);

Moving on the second part where you had an issue:

    let times = timetable.get(&st);
    let default: &[&str] = &[];

Firstly, we have the same problem as before with a (in this case empty) array. It can be solved in the same way as above.

    let result = match times {
        Some(r) => r,
        None => default,
    };

Option has a method, unwrap_or which does the same thing:

    let result = timetable.get(&st).unwrap_or([].as_slice());

However, a better approach would be use an if let statement.

    if let Some(times) = timetable.get(&st) {
        for t in times {
            if *t == self.time {
                return true;
            }
        }
    }

This way the loop over times only runs if it was present and you don't need to create and iterate over an empty slice.

However, if you use Vec instead of slices as I suggest, the inner loop can be written as a call to the contains function:

        if times.contains(&self.time) {
            return true;
        }

Or you can replace the whole function with:

        timetable
            .get(&Stations {
                start: self.start,
                destination: self.destination
            })
            .map_or(false, |times| times.contains(&self.time))

map_or returns false if the time table was not found, otherwise it calls the closure which checks if times contains the appropriate time.


Timeout Error in Fraudulent Activity Notification HackerRank I am solving this problem: Fraudulent Activity Notifications on HackerRank. I am done with my code and is working, but it is inefficient as well for very large inputs.

  I don't know but after all my efforts, I am able to give out good solution to a problem of a MEDIUM LEVEL but this timeout error happens every time for very large inputs. I have tried optimizing my code and still I get timeout errors.
  My agendas for this question and upcoming questions are:

  How to put efficiency for very large inputs. What kind of intellect it requires.
  How to reach to that level. What should I prepare for this.
  Code optimization

I am open to learning, and I am really desperate to learn how to write a more advanced and optimized code to make myself better. I am open to do hard work.

My Algorithm:

  For this problem we must go from incrementing variable i till len(givenArray)-d
  Take a variable for the next variable to be compared, my case iterate is the variable
  Pass the values to the particular array to the method for counting countFraud()
  Add it to the count variable
  Increment iterate variable

Code:

# this is for counting the trailing array
def countFraud(arr, nextNum):
    count = 0
    median = 0.0
    d = len(arr)
    #for calculating the median correctly
    arr.sort()
    if d%2 != 0:
        median = arr[int(d/2)]
    else:
        n = int(d/2)
        median = (arr[n] + arr[n-1]) / 2

    #now the opeartion for count from the array
    if nextNum >= 2*median: count += 1
    return count

# Complete the activityNotifications function below.
def activityNotifications(expenditure, d):
    count = 0
    iterate = d
    # it will go upto the len of array - d, so that it will go upto the d length
    # leaving the last element everytime for the comparision
    for i in range(len(expenditure)-d):
        count += countFraud(expenditure[i:iterate], expenditure[iterate])
        iterate += 1
    return count

Now previously I was doing two loops, adding the items to the new_array and passing it to the the countFraud(). But now I have optimized it and made it sort of O(N).

I don't know but this code is not getting submitted for all TCs due to Timeout Error. There is no problem in operation part. It is just with the efficiency of the code.

Timeout Error Input Example:

200000 10000

Link for input - Input Data

Expected Output:

633

I have read upon this article: HackerRank Environment to learn about the timing issue. For Python/Python 3 it is 10 seconds. My code is definitely taking more than that for values greater than 10^3 or 4.

My code has successfully passed 3 TCs though.

If this wasn't a programming challenge with restricted modules, I would suggest using pandas. It has a rolling windowed median implemented, so it is quite straight forward (just have to be careful not to have an off-by-one error with the shift):

import pandas as pd

def activityNotifications(expenditure, d):
    df = pd.DataFrame(expenditure)
    return (df.shift(-1) > 2 * df.rolling(d).median())[0].sum()

For the example input you gave, this runs in about 210 ms ± 3.71 ms on my machine. This is very hard to beat, both in runtime and readability.

Unfortunately, Hackerrank does not have pandas available (or even numpy for that matter). So, we need another implementation of a windowed running median. There are multiple ways to implement it, but here is one. You keep track of two collections, one is the window you are looking at and one is a sorted version of it. The former tells you which value drops out when you add a new one, and the latter allows you to easily get the median without having to sort the window each time, if you insert elements at the right place. For this you can use the bisect modules insort (which is \$\mathcal{O}(n)\$, instead of \$\mathcal{O}(n\log n)\$ for sorting each time).

I used the RunningMedian function from here, and modified it a bit to directly count instead of collecting the medians. It is not the most beautiful code to look at, I might clean it up a bit later.

from itertools import islice
from collections import deque
from bisect import insort, bisect_left

def activityNotifications(seq, M):
    """
     Purpose: Find the median for the points in a sliding window (odd number in size)
              as it is moved from left to right by one point at a time.
      Inputs:
            seq -- list containing items for which a running median (in a sliding window)
                   is to be calculated
              M -- number of items in window (window size) -- must be an integer > 1
      Otputs:
         medians -- list of medians with size N - M + 1
       Note:
         1. The median of a finite list of numbers is the "center" value when this list
            is sorted in ascending order.
         2. If M is an even number the two elements in the window that
            are close to the center are averaged to give the median (this
            is not by definition)
    """
    seq = iter(seq)
    s = []
    m = M // 2

    # Set up list s (to be sorted) and load deque with first window of seq
    s = [item for item in islice(seq, M)]
    d = deque(s)

    # Simple lambda function to handle even/odd window sizes
    median = lambda : s[m] if bool(M&1) else (s[m-1]+s[m])*0.5

    # Sort it in increasing order and extract the median ("center" of the sorted window)
    s.sort()

    # Now slide the window by one point to the right for each new position (each pass through
    # the loop). Stop when the item in the right end of the deque contains the last item in seq
    count = 0
    for item in seq:
        count += item > 2 * median()
        old = d.popleft()          # pop oldest from left
        d.append(item)             # push newest in from right
        del s[bisect_left(s, old)] # locate insertion point and then remove old
        insort(s, item)            # insert newest such that new sort is not required
    return count

This code takes about 871 ms ± 14.5 ms on my machine for the given testcase. It passes all testcases on Hackerrank as well. It is slower than the pandas one, but that is to be expected. Both functions give the same result.

Some additional comments about your code itself:

if nextNum >= 2*median: count += 1 could just be return nextNum >= 2*median. Summing bools is perfectly fine, since they are just ints underneath.
if d%2 != 0: can be if d % 2, because non-zero integers are truthy.
In actual code please follow Python's official style-guide, PEP8. It recommends naming your function activity_notifications.
You should add a docstring to explain what your function does.


Random Access Iterator Implementation (This is the Non-const version, I have to implement the const one too).
Could someone please review this implementation? This is made for std::vector
I'm unsure whether I respected all requirements for LegacyRandomAccessIterator; so if I'm missing something, please do let me know.
namespace random_access
{
    template<typename Type>
    class iterator
    {
    private:
        Type* m_iterator;

    public:
        using value_type = Type;
        using reference = value_type&;
        using pointer = value_type*;
        using iterator_category = std::random_access_iterator_tag;
        using difference_type = std::ptrdiff_t;
        //using iterator_concept = std::contiguous_iterator_tag;

        constexpr iterator(Type* iter = nullptr) : m_iterator{ iter } {}

        constexpr bool operator==(const iterator& other) const noexcept { return m_iterator == other.m_iterator; }
        constexpr bool operator!=(const iterator& other) const noexcept { return m_iterator != other.m_iterator; }
        constexpr reference operator*() const noexcept { return *m_iterator; }
        constexpr pointer operator->() const noexcept { return m_iterator; }
        constexpr iterator& operator++() noexcept { ++m_iterator; return *this; }
        constexpr iterator operator++(int) noexcept { iterator tmp(*this); ++(*this); return tmp; }
        constexpr iterator& operator--() noexcept { --m_iterator; return *this; }
        constexpr iterator operator--(int) noexcept { iterator tmp(*this); --(*this); return tmp; }
        constexpr iterator& operator+=(const difference_type other) noexcept { m_iterator += other; return *this; }
        constexpr iterator& operator-=(const difference_type other) noexcept { m_iterator -= other; return *this; }
        constexpr iterator operator+(const difference_type other) const noexcept { return iterator(m_iterator + other); }
        constexpr iterator operator-(const difference_type other) const noexcept { return iterator(m_iterator - other); }
        constexpr iterator operator+(const iterator& other) const noexcept { return iterator(*this + other.m_iterator); }
        constexpr difference_type operator-(const iterator& other) const noexcept { return std::distance(m_iterator, other.m_iterator); }
        constexpr reference operator[](std::size_t index) const { return m_iterator[index]; }
        constexpr bool operator<(const iterator& other) const noexcept { return m_iterator < other.m_iterator; }
        constexpr bool operator>(const iterator& other) const noexcept { return m_iterator > other.m_iterator; }
        constexpr bool operator<=(const iterator& other) const noexcept { return m_iterator <= other.m_iterator; }
        constexpr bool operator>=(const iterator& other) const noexcept { return m_iterator >= other.m_iterator; }
    };
}

Thanks !

As pointed out by @Martin York in the comments that your class is just a wrapper for a pointer, I don't have much to say. I only have one suggestion

Better Formatting

Good formatting IMO plays a major role in how readable your code is. In this case, I find it impossible to navigate through the functions because they all look extremely cramped.
constexpr pointer operator->() const noexcept { return m_iterator; }
constexpr iterator& operator++() noexcept { ++m_iterator; return *this; }

Compare that to
constexpr pointer operator->() const noexcept {
    return m_iterator;
}

constexpr iterator& operator++() noexcept {
    ++m_iterator;
    return *this;
}


Better way to un-define a set of defines based on version numbers in C++? In a header I have something like:

#define OPENGL_TARGET_VERSION_MAJOR 4
#define OPENGL_TARGET_VERSION_MINOR 0

#define OPENGL_TARGET_VERSION ( OPENGL_TARGET_VERSION_MAJOR * 10 \
                              + OPENGL_TARGET_VERSION_MINOR * 1 )

And then a lot of checks like:

#if OPENGL_TARGET_VERSION < 44
    #ifdef GL_VERSION_4_4
        #undef GL_VERSION_4_4
    #endif
#endif

#if OPENGL_TARGET_VERSION < 43
    #ifdef GL_VERSION_4_3
        #undef GL_VERSION_4_3
    #endif
#endif

#if OPENGL_TARGET_VERSION < 42
    #ifdef GL_VERSION_4_2
        #undef GL_VERSION_4_2
    #endif
#endif

#if OPENGL_TARGET_VERSION < 41
    #ifdef GL_VERSION_4_1
        #undef GL_VERSION_4_1
    #endif
#endif

...

Is there any way I could avoid repeating these checks for every version-number?

While it's somewhat uncertain whether you'll like it (some compilers warn about doing so) it's entirely permissible to #undef a value that wasn't defined to start with, so you can consolidate these down to something like:

#if OPENGL_TARGET_VERSION < 44
    #undef GL_VERSION_4_4
#endif

#if OPENGL_TARGET_VERSION < 43
    #undef GL_VERSION_4_3
#endif

#if OPENGL_TARGET_VERSION < 42
    #undef GL_VERSION_4_2
#endif

#if OPENGL_TARGET_VERSION < 41
    #undef GL_VERSION_4_1
#endif

While this does (or at least sort of might) answer the question you asked, from a code review standpoint, I'd probably prefer the code as it's written in the question. It's longer, but:

it's almost certainly hidden in a header almost nobody really reads through very often
it's simple repetitions of a simple pattern, so reading it isn't much work,
eliminating the #ifdef will give warnings with some compilers

While I think that's a pretty stupid warning, eliminating the warning is still worth more than reducing the line count.


A Concise Quick Sort This is an implementation of Quick Sort in JS. Can this implantation of Quick Sort be more concise and efficient while still remaining expressive?

function quickSort(list){
   if( list.length < 2 ) {
    return list;
}
var leftList = [];
var rightList = [];
for( var i = 1; i < list.length; i++ ){
    if(list[i] >= list[0]){
       rightList.push(list[i]) ;
    }else{
       leftList.push(list[i]);
    }
}

var sortedLeft = quickSort(leftList);
var sortedRight = quickSort(rightList);
var combined = sortedLeft.concat(list[0]).concat(sortedRight);
console.log(combined.join(' '));
return combined;
}

var array=[9,8,7,6,5,4,3,2,1];
quickSort(array);

Example 2 works with randomly selected pivots, filters, and recursion. This example is more efficient than the first while still remaining as just as expressive. Unfortunately, both perform horribly compared to in place and nonrecursive approaches.

function quicksort(arr,len=arr.length) {
    if (len <= 1) return arr;
        else
        var randomIndex = Math.floor(Math.random() * len),
        pivot = arr.splice(randomIndex, 1),
        lesserHalf = arr.filter(el =>el <= pivot)
        biggerHalf = arr.filter((el)=> el > pivot)
        return ((lesserHalf, pivot, biggerHalf) =>
        [].concat(quicksort(lesserHalf), pivot, quicksort(biggerHalf)))
        (lesserHalf, pivot, biggerHalf)
}

var arr =[9,8,7,6,5,4,3,2,1];
console.log(quicksort(arr));

Example 3 is the shortest I've seen using spread and inspired by Haskel

const quickSort = ([x = [], ...xs]) => {
  if (x.length === 0) return [];
  return [
    ...quickSort(xs.filter(y => y <= x)),
    x,
    ...quickSort(xs.filter(y => y > x))
  ];
}

var arr= [9,8,7,6,5,4,3,2,1];

console.log(quickSort(arr))

Before even thinking about performance, concision, etc., run JSLint or similar over these methods and fix the indentation, other whitespace, and bugs. As it is the lack of consistency and poor indentation makes it hard to read and hides things like a missing comma which is leading to implicit semicolon insertion:

        var randomIndex = Math.floor(Math.random() * len),
        pivot = arr.splice(randomIndex, 1),
        lesserHalf = arr.filter(el =>el <= pivot)
        biggerHalf = arr.filter((el)=> el > pivot)

These four lines also look buggy because pivot is an array and yet it's being compared to an element with <= and >.

        var randomIndex = Math.floor(Math.random() * len),
            pivot = arr.splice(randomIndex, 1)[0],
            lesserHalf = arr.filter(el => el <= pivot),
            biggerHalf = arr.filter(el => el > pivot);

is more consistent, more obviously correct, and fixes at least one bug.

Example 3 also looks buggy.

const quickSort = ([x = [], ...xs]) => {
  if (x.length === 0) return [];

How will this sort ["", "a", "b"]? Answer: it will return [].


Class for sending emails I'm 100% sure improvements can be made. Any help is greatly appreciated. I should add that I am new to OOP and am not sure if I'm using this the right way.

<?php
defined('_VALID') or die('Restricted Access!');

class email
{
public function addtemplate($html, $public)
{
    global $conn;

    $sql = "INSERT INTO emailtemplate SET id = '', parentid = '" . $_SESSION['uid'] . "', html = '" . mysql_escape_string($html) . "', public = '" . $public . "', added = '" . time() . "'";
    $conn->execute($sql);

    if ($conn->affected_rows() == 1) {
        return true;
    } else {
        return false;
    }
}

public function addtext($subject, $text)
{
    global $conn;

    $sql = "INSERT INTO emailtext SET id = '', parentid = '" . $_SESSION['uid'] . "', subject = '" . mysql_escape_string($subject) . "', text = '" . mysql_escape_string($text) . "', added = '" . time() . "'";
    $conn->execute($sql);

    if ($conn->affected_rows() == 1) {
        return $conn->insert_id();
    } else {
        return false;
    }
}

public function addqueue($to, $text, $template)
{
    global $conn;

    $sql = "INSERT INTO emailqueue SET eid = '', parentid = '" . $_SESSION['uid'] . "', eto = '" . $to . "', etext = '" . $text . "', etemplate = '" . $template . "', added = '" . time() . "', status = 'pending'";
    $conn->execute($sql);

    if ($conn->affected_rows() == 1) {
        return true;
    } else {
        return false;
    }
}

public function delete($eid)
{
    global $conn;

    $sql = "DELETE FROM emailqueue WHERE eid = '" . $eid . "' AND parentid = '" . $_SESSION['uid'] . "' AND status = 'pending'";
    $conn->execute($sql);

    if ($conn->affected_rows() == 1) {
        return true;
    } else {
        return false;
    }
}

public function generate($eid)
{
    global $conn;

    $sql   = "SELECT q.*, x.*, t.*, c.id, c.fname, c.email, p.UID, p.company, p.hash FROM emailqueue AS q, emailtext AS x, emailtemplate AS t, clients AS c,
                signup AS p WHERE q.eid = '" . $eid . "' AND q.etemplate = t.id AND q.etext = x.id AND c.id = q.eto AND p.UID = q.parentid";
    $rs    = $conn->execute($sql);
    $email = $rs->getrows();
    $email = $email[0];

    $find    = array(
        '{name}',
        '{email}',
        '{company}',
        '{hash}'
    );
    $replace = array(
        $email['fname'],
        $email['email'],
        $email['company'],
        $email['hash']
    );

    $body  = str_replace('{bodytext}', $email['text'], stripslashes($email['html']));
    $final = str_replace($find, $replace, $body);

    $array['body'] = stripslashes($final);
    $array['subject'] = $email['subject'];
    $array['to'] = $email['email'];

    return $array;
}

public function from($eid)
{
    global $conn;

    $sql   = "SELECT q.*, s.UID, s.email, s.company FROM emailqueue AS q, signup AS s WHERE q.eid = '" . $eid . "' AND q.parentid = s.UID";
    $rs    = $conn->execute($sql);
    $email = $rs->getrows();
    $email = $email[0];

    $array['company'] = $email['company'];
    $array['email']   = $email['email'];

    return $array;
}

public function send($eid)
{
    $msg      = $this->generate($eid);
    $from    = $this->from($eid);

    $headers = "From: " . $from['company'] . " <noreply@emailsmsmarketing.com>\r\n";
    $headers .= "Reply-To: " . $from['email'] . "\r\n";
    $headers .= "MIME-Version: 1.0\r\n";
    $headers .= "Content-type: text/html; \r\n";

    if (mail($msg['to'], $msg['subject'], $msg['body'], $headers)) {
        return true;
    } else {
        return false;
    }
    }
}
?>

Not a PHP expert, but things like

if ($conn->affected_rows() == 1) {
    return true;
} else {
    return false;
}

should be

return $conn->affected_rows() == 1;


POSIX shell script using a string to store command arguments Background

I’ve started putting all my configuration files (which include many shell files) under version control.  In the Git repository, I’ve configured a pre-commit hook that runs the checkbashisms utility from Debian’s devscripts package on each sh file in the repository as a simple sanity check to ensure that I don’t inadvertently introduce Bash-specific syntax into scripts that should be POSIX compatible.

Reason for using POSIX sh

I’m using /bin/sh as the interpreter for this pre-commit hook as it’s quicker for systems where /bin/sh is a link to dash. Committing files shouldn’t take a noticeable amount of time.

I don’t want to run checkbashisms if the only files staged for committing are non-shell files and for performance, I only want to execute checkbashisms command once and supply as arguments the names of all the relevant files that should be checked.

Arguments that may contain spaces

While I don’t normally create files with names containing unusual characters, I tried to write this script to be reasonably robust (without becoming overly complex, harder to read and prone to error on various inputs).

In a more fully-featured shell such as Bash, I’d process a list of file name arguments using an array. However, a plain POSIX shell (such as dash) doesn’t support array variables.

My solution

When using a POSIX shell, the positional parameters can often be used as a substitute but in this case, I couldn’t see how I could easily modify them to only add the filenames of shell scripts.  The technique I used was to split fields using newlines (not spaces) and to use a string to store the arguments to be passed to checkbashisms.

I’ve verified that it works with filenames containing spaces and single quotes – but due to to how the shell represents names containing other characters, it doesn’t work with filenames containing double quotes, tabs, etc. This is fine as I really don’t expect to have to process such filenames.

However, it feels hacky and I don’t know if I’m introducing the potential for other errors (“unknown unknowns”). Modifying IFS is explicitly not recommended by Greg’s Wiki. I thought I’d post here in the hope that the robustness and/or maintainability of the code can be improved.

Script

#!/bin/sh

# Check shell scripts for Bashisms if the `checkbashisms` utility is installed.
# Called by "git commit" with no arguments.  This hook should exit with a
# non-zero status after issuing an appropriate message if it wants to stop the
# commit.
# Allows for files with spaces or single quotes in their name – but not any
# other unusual characters.

set -u  # -o nounset – exit with failure if an unset shell variable is referenced.
set -e  # -o errexit – exit script if any command fails.
set -o noclobber  # prevent shell redirection from over-writing files.

if ! command -v checkbashisms >/dev/null 2>&1; then
  echo "‘checkbashisms’ is not available; it can be installed from the ‘devscripts’ package."
else
  echo "Checking for Bashisms in shell scripts..."

  # Use only newline characters (not spaces) to split filenames.
  IFS="
"  # POSIX way to set IFS to newline (be careful with indentation).
  set -f  # Disable globbing of pathnames.
  # Arrays are not defined by POSIX so store arguments in a string variable.
  filenames=""

  # Get list of filenames that have been staged for committing.
  for file in $(git diff --cached --name-only); do
    # Only process shell scripts.
    case "$file" in *.sh|shell/*.sh)
      # Add file to list of arguments (separated by newlines).
      filenames="$filenames
$file"  # again, be careful with indentation of the code.
    esac
  done;

  # If any shell files have been staged, check their syntax for Bashisms.
  if [ "$filenames" ]; then
    if checkbashisms $filenames; then   # no quotes for field-splitting.
      printf "(No Bashisms found)"
    else
      exit 1
    fi
  fi

  # Restore file globbing and IFS.
  unset IFS
  set +f
fi

# Further code to check for other issues ...

First thing, you could turn the check if the checkbashisms tool is installed into a guard clause, shaving off one level of indentation:

if ! command -v checkbashisms >/dev/null 2>&1; then
    echo "‘checkbashisms’ is not available; it can be installed from the ‘devscripts’ package."
    exit 1
fi

The remaining logic can be simplified a lot with the xargs tool.

if git diff --cached --name-only | grep "\\.sh$" | xargs checkbashisms; then
    printf "(No Bashisms found)"
else
    exit 1
fi

I understand that the code you showed it only part of a bigger script and you still want to use the generated list of files. However, calling git diff repeatedly will have negligible overhead and you can wrap the code in a shell function like this:

staged_scripts() {
    git diff --cached --name-only | grep "\\.sh$"
}

staged_scripts | xargs checkbashisms

Or even

check_staged_scripts() {
    git diff --cached --name-only | grep "\\.sh$" | xargs $1
}

check_staged_scripts "checkbashisms"


Find floor and ceiling in the BST Find ceiling and floor in the BinarySearchTree. Looking for code-review, optmizations and best practices.

public class FloorCeiling {

    private TreeNode root;

    public FloorCeiling(List<Integer> items) {
        create(items);
    }

    private void create (List<Integer> items) {
        if (items.isEmpty())  {
            throw new NullPointerException("The items array is empty.");
        }

        root = new TreeNode(items.get(0));

        final Queue<TreeNode> queue = new LinkedList<TreeNode>();
        queue.add(root);

        final int half = items.size() / 2;

        for (int i = 0; i < half; i++) {
            if (items.get(i) != null) {
                final TreeNode current = queue.poll();
                final int left = 2 * i + 1;
                final int right = 2 * i + 2;

                if (items.get(left) != null) {
                    current.left = new TreeNode(items.get(left));
                    queue.add(current.left);
                }
                if (right < items.size() && items.get(right) != null) {
                    current.right = new TreeNode(items.get(right));
                    queue.add(current.right);
                }
            }
        }
    }

    private static class TreeNode {
        private TreeNode left;
        private int item;
        private TreeNode right;

        TreeNode(int item) {
            this.item = item;
        }
    }

    private static class IntegerObj {
        Integer obj = null;
    }

    public int ceiling (int val) {
        IntegerObj iobj = new IntegerObj();
        recurseCeiling(root, iobj, val);
        return iobj.obj;
    }

    public int floor (int val) {
        IntegerObj iobj = new IntegerObj();
        recurseFloor(root, iobj, val);
        return iobj.obj;
    }

    private void recurseCeiling (TreeNode node,  IntegerObj iobj, int value) {
        if (node == null) {
            return;
        }
        if (value <= node.item) {
            iobj.obj = node.item;
            recurseCeiling(node.left, iobj, value);
        } else {
            recurseCeiling(node.right, iobj, value);
        }
    }

    private void recurseFloor (TreeNode node, IntegerObj iobj, int value) {
        if (node == null) {
            return;
        }
        if (value < node.item) {
            recurseFloor (node.left, iobj, value);
        } else {
            iobj.obj = node.item;
            recurseFloor (node.right, iobj, value);
        }
    }
}

public class FloorCeilingTest {

    @Test
    public void test1() {
        FloorCeiling fc1 = new FloorCeiling(Arrays.asList(100, 50, 150, 25, 75, 125, 175));
        assertEquals(25,  fc1.ceiling(20));
        assertEquals(50,  fc1.ceiling(30));
        assertEquals(75,  fc1.ceiling(70));
        assertEquals(100, fc1.ceiling(90));
        assertEquals(125, fc1.ceiling(120));
        assertEquals(150, fc1.ceiling(145));
        assertEquals(175, fc1.ceiling(160));
    }

    @Test
    public void test2() {
        FloorCeiling fc2 = new FloorCeiling(Arrays.asList(100, 50, 150, 25, 75, 125, 175));
        assertEquals(25,  fc2.floor(27));
        assertEquals(50,  fc2.floor(55));
        assertEquals(75,  fc2.floor(78));
        assertEquals(100, fc2.floor(110));
        assertEquals(125, fc2.floor(128));
        assertEquals(150, fc2.floor(160));
        assertEquals(175, fc2.floor(180));
    }

}

This:

private static class IntegerObj {
    Integer obj = null;
}

really made me wonder. You're threading this object through recursive calls to hold the answer.

Instead you can remove this class and change your methods to return an int:

public int ceiling(int val) {
  return recurseCeiling(root, null, val);
}

public int floor(int val) {
  return recurseFloor(root, null, val);
}

private int recurseCeiling(TreeNode node, Integer ceil, int value) {
  if (node == null) {
    return ceil;
  }
  if (value <= node.item) {
    return recurseCeiling(node.left, node.item, value);
  }
  return recurseCeiling(node.right, ceil, value);
}

private int recurseFloor(TreeNode node, Integer floor, int value) {
  if (node == null) {
    return floor;
  }
  if (value < node.item) {
    return recurseFloor(node.left, floor, value);
  }
  return recurseFloor(node.right, node.item, value);
}

We can now see that these methods can be written iteratively

private int recurseCeiling(TreeNode node, Integer ceil, int value) {
  while (node != null) {
    if (value <= node.item) {
      ceil = node.item;
      node = node.left;
    } else {
      node = node.right;
    }
  }
  return ceil;
}

private int recurseFloor(TreeNode node, Integer floor, int value) {
  while (node != null) {
    if (value < node.item) {
      node = node.left;
    } else {
      floor = node.item;
      node = node.right;
    }
  }
  return floor;
}


Rock-Paper-Scissors engine Looking over Rock, Paper, Scissors. C++ from a beginning programmer prompted me to think about how I'd program this simple game in C++.  The RockPaperScissors class contains the essential features of printing and evaluating each round of the game, as well as generation of randomly selected moves.  I didn't encode a player input because that didn't seem too interesting; instead the sample program just plays 20 rounds against itself and prints the result of each.  No statistics are gathered or stored.

One feature that does exist is that there is a #define to select whether the classic game or the enhanced "Rock-Paper-Scissors-Lizard-Spock" version is used.  (With a bit more work, which I chose not to invest, one could allow for a fairly generic \$n\$-ary relation as the basis of the game.)  The other feature worth noting is that the operator< is deliberately private because the \$<\$ operation implemented here does not represent a partially ordered set because the operation is not transitive.  In simple terms, we would normally expect that \$(a < b) \land (b < c) \implies (a < c)\$ but this is expressly not true here.

I'd be interested in comments on those or any other design decisions or implementation details.

#include <iostream>
#include <string_view>
#include <array>
#include <stdexcept>
#include <random>

// define ENHANCED to non-zero for enhanced version with Spock, Lizard
#ifndef ENHANCED
#define ENHANCED 0
#endif

class RockPaperScissors
{
public:
    RockPaperScissors(std::size_t num) {
        if (num >= words.size()) {
            throw std::range_error("invalid choice");
        }
        choice = num;
    }
    static RockPaperScissors random();
    const std::string_view& vs(const RockPaperScissors &b) const;
    friend std::ostream& operator<<(std::ostream& out, const RockPaperScissors &rps);
private:
    bool operator==(const RockPaperScissors &b) const;
    bool operator<(const RockPaperScissors &b) const;
#if ENHANCED
    static constexpr std::array<std::string_view, 5> words{ "Rock", "Paper", "Scissors", "Spock", "Lizard", };
#else
    static constexpr std::array<std::string_view, 3> words{ "Rock", "Paper", "Scissors" };
#endif
    static constexpr std::array<std::string_view, 3> results{"LOSE", "TIE", "WIN" };
    std::size_t choice;
};

std::ostream& operator<<(std::ostream& out, const RockPaperScissors &rps) {
    return out << rps.words[rps.choice];
}

RockPaperScissors RockPaperScissors::random() {
    static std::random_device rd;
    static std::mt19937 gen{rd()};
    static std::uniform_int_distribution<> dis(0, RockPaperScissors::words.size()-1);
    std::size_t which{static_cast<std::size_t>(dis(gen))};
    return RockPaperScissors{which};
}

bool RockPaperScissors::operator==(const RockPaperScissors &b) const {
    return choice == b.choice;
}

bool RockPaperScissors::operator<(const RockPaperScissors &b) const {
    return (choice + 1) % words.size() == b.choice || (choice + 3) % words.size() == b.choice;
}

const std::string_view& RockPaperScissors::vs(const RockPaperScissors &b) const {
    if (*this == b) {
        return results[1];
    } else if (*this < b) {
        return results[0];
    }
    return results[2];
}

int main()
{
    for (int trials = 20; trials; --trials) {
        auto a = RockPaperScissors::random();
        auto b = RockPaperScissors::random();
        std::cout << a << " vs. " << b << ": " << a << " " << a.vs(b) << "S!\n";
    }
}

After preparing this recent answer it occurred to me that I'd already written something like it.  Here's my review of this code.

Make the class more useful

Right now the only thing we can do with the RockPaperScissors class is to get a new one via random or print the results of a contest.  If we wanted to do something else, such as collect some statistics, there are pieces missing, such as the ability to get a printable version of the class.  Here's one way to add that:

friend std::ostream& operator<<(std::ostream& out, const RockPaperScissors &rps);

Now we can gather some statistics like this:

int main()
{
    std::unordered_map<std::string_view, unsigned> responses;
    std::unordered_map<std::string_view, unsigned> results;
    constexpr unsigned max_trials{300'000'000};
    for (unsigned trials = max_trials; trials; --trials) {
        auto a = RockPaperScissors::random();
        auto b = RockPaperScissors::random();
        ++responses[static_cast<std::string_view>(a)];
        ++responses[static_cast<std::string_view>(b)];
        ++results[a.vs(b)];
    }
    for (const auto& resp: responses) {
        std::cout << std::setw(16) << resp.first << '\t' << resp.second
            << '\t' << static_cast<double>(resp.second)/max_trials/2
            << '\n';
    }
    for (const auto& resp: results) {
        std::cout << std::setw(16) << resp.first << '\t' << resp.second
            << '\t' << static_cast<double>(resp.second)/max_trials
            << '\n';
    }
}

This gives a report something like this:

       Paper    200002179   0.333337
    Scissors    199987250   0.333312
        Rock    200010571   0.333351
         WIN    66662424    0.222208
         TIE    100003629   0.333345
        LOSE    133333947   0.444446

Prefer open functions to class functions

The current code has this member function:

RockPaperScissors RockPaperScissors::random() {
    static std::random_device rd;
    static std::mt19937 gen{rd()};
    static std::uniform_int_distribution<> dis(0, RockPaperScissors::words.size()-1);
    std::size_t which{static_cast<std::size_t>(dis(gen))};
    return RockPaperScissors{which};
}

However, selecting a single item at random from within a collection is a very common need.  A more generally useful way to write this might be this:

template <typename Iter>
Iter random_one(Iter begin, const Iter& end) {
    static std::random_device rd;
    static std::mt19937 gen{rd()};
    static std::uniform_int_distribution<> dis(0, std::distance(begin, end) - 1);
    std::advance(begin, dis(gen));
    return begin;
}

Now we can get an iterator that points to a particular item in any collection.  (To make this template more durable, we'd actually probably want to do some validation of both the Iter type and the values passed, but this conveys the idea.) We could use that with no further modifications as in this:

RockPaperScissors RockPaperScissors::random() {
    auto which = std::distance(words.begin(), random_one(words.begin(), words.end()));
    return RockPaperScissors{which};
}

But that's a bit odd because we are converting an iterator to an ordinal.  That leads to the next suggestion.

Store an iterator within the class

We can simplify some of the code for this implementation by changing the definition of choice within the class to be an iterator.

using iter = decltype(words.begin());
iter choice;

Now the constructor can use the templated function above very simply:

RockPaperScissors::RockPaperScissors() : choice{random_one(words.begin(), words.end())}
{
}

A few other changes are needed, but mostly they simplify the code.

Also note that now the random function is no longer needed.


Return a list of values from a single column in a DataTable I have this code that I was asked to maintain, it is a notification system for a third party application event system (a lot more involved than that but for the sake of this question...)

The Code uses a class DataTable Variable with 4 columns, it takes one of the columns and takes the distinct values and outputs them to a list, but the person that wrote the code made it no so dynamic values are static.

What I am looking for is a way that the underlying DataTable will determine the List<string> instead of having to go through all these if statements that have the wonderful magic strings.

private List<string> GetBaseTypes()
{
    List<string> baseTypes = new List<string>();
    var result = (
       from myRow in _allEventsList.AsEnumerable()
       select myRow.Field<string>("BaseTypeDesc"))
       .Distinct();

    var item = result
       .SingleOrDefault(e => e == "Order");
    if (item != null)
        baseTypes.Add(item);
    item = result
       .SingleOrDefault(e => e == "Motion");
    if (item != null)
        baseTypes.Add(item);
    item = result
       .SingleOrDefault(e => e == "Service");
    if (item != null)
        baseTypes.Add(item);
    item = result
       .SingleOrDefault(e => e == "Original Filing Document");
    if (item != null)
        baseTypes.Add(item);
    item = result
       .SingleOrDefault(e => e == "Amended Filing Documents");
    if (item != null)
        baseTypes.Add(item);
    item = result
       .SingleOrDefault(e => e == "Case Event Text");
    if (item != null)
        baseTypes.Add(item);
    item = result
       .SingleOrDefault(e => e == "Other Event Types");
    if (item != null)
        baseTypes.Add(item);
    item = result
       .SingleOrDefault(e => e == "Attorney Assignment");
    if (item != null)
        baseTypes.Add(item);
    item = result
       .SingleOrDefault(e => e == "Court Administration");
    if (item != null)
        baseTypes.Add(item);
    item = result
       .SingleOrDefault(e => e == "Physical Filing Location");
    if (item != null)
        baseTypes.Add(item);

    return baseTypes;
}

Does anyone see something that sticks out where I could simplify this code?

I have been tasked with changing the data going into the DataTable and I don't want to enter in new entries every time something is added to the "BaseTypeDesc" .

Two code-style issues are immediately obvious:

Use braces for 1-liners
only split lines where it overflows

Code like:

var item = result
   .SingleOrDefault(e => e == "Order");
if (item != null)
    baseTypes.Add(item);
item = result
   .SingleOrDefault(e => e == "Motion");

Should be

var item = result.SingleOrDefault(e => e == "Order");
if (item != null)
{
    baseTypes.Add(item);
}
item = result.SingleOrDefault(e => e == "Motion");

Then, the logical thing to do to simplify those cascading statements, is to create an array of keys to look for (perhaps by loading from a configuration file, or as a static constant array...):

string[] keys = {"Order", "Motion", .....};

then loop through them like:

foreach (string key in keys)
{
    var item = result.SingleOrDefault(e => e == key);
    if (item != null)
    {
        baseTypes.Add(item);
    }
}


General review of Robot class I'm working alone on my code, trying to learn Ruby as I go.

Class Robot is supposed to be instantiated with the position on a map, and whether it is facing toward a side of the map. It has a couple of moves: left or right, where the face of the robot changes direction, and a move action, one step forward.

class Robot

  def initialize(pos_X, pos_Y, facing)
    @pos_X, @pos_Y, @facing = pos_X, pos_Y, facing
  end

  def move
    world_switch(Proc.new { @pos_X += 1}, Proc.new { @pos_X -= 1},
                 Proc.new { @pos_Y += 1}, Proc.new { @pos_Y -= 1})
  end

  def left
    world_switch(Proc.new {@facing = 'WEST'}, Proc.new {@facing = 'EAST'},
                Proc.new {@facing = 'NORTH'}, Proc.new {@facing = 'SOUTH'})
  end

  def right
    world_switch(Proc.new {@facing = 'EAST'}, Proc.new {@facing = 'WEST'},
                Proc.new {@facing = 'SOUTH'}, Proc.new {@facing = 'NORTH'})
  end

  def report
    puts "Output: " <<  @pos_X.to_s << ',' << @pos_Y.to_s << ',' << @facing
  end

  def world_switch(do_on_north, do_on_south, do_on_east, do_on_west)
    case @facing
    when 'NORTH'
      do_on_north.call
    when 'SOUTH'
      do_on_south.call
    when 'EAST'
      do_on_east.call
    when 'WEST'
      do_on_west.call
    end
  end
end

Looks like you over-thought it a little bit. Callbacks are indeed a powerful abstraction, but it looks overkill in this case. Some notes:

Use symbols to codify @facing: :east, :north, ...
a hint to write move without callbacks:

increments = {:north => [0, +1], :east => [+1, 0], :south => [0, -1], :west => [-1, 0]}

A hint to write left without callbacks:

new_facing = {:north => :west, :east => :north, :south => :east, :west => :south}

This way you describe what the robot does not with code (callbacks) but with data structures (which can be as simple as a hash).

As requested: the complete implementation of left:

def left
  new_facing = {:north => :west, :east => :north, :south => :east, :west => :south}
  @facing = new_facing[@facing]
end


Database migration script I'm working on a database migration script written in python, which will take data from a MySQL database and insert them into a PostgreSQL database with a different Schema (different table structures, different datatypes and so on).

Performances does matter, since I will have to handle with sizable databases.

I use mysql.connector and psycopg2 adapters in order to make python talks with the two databases.

My problem is that I need performance, but also the power of modifying/converting mysql data before inserting rows in the new fresh database. I will give you an example in order to show why there actually is a conflict of interest.

Following is a snippet of code without performance optimization, but at least, where transformation/conversion of data was possible:

cur_msql.execute("SELECT customer_id, customer_name, contact_name, address, city, postal_code, country FROM mysqlcustomers")

for row in cur_msql:

  # /!\ checking if customer_name is NULL (NOT NULL field in destination database)
  if row['customer_name'] == NULL:
    row['customer_name'] = row['contact_name']

  try:
    cur_psql.execute("INSERT INTO psqlcustomers (customer_id, customer_name, contact_name, address, city, postal_code, country) \
                      VALUES (%(customer_id)s, %(customer_name)s, %(contact_name)s, %(address)s, %(city)s, %(postal_code)s, %(country)s)", row)
  except psycopg2.Error as e:
    print "cannot execute that query", e.pgerror
    sys.exit("leaving early the script")

As you can see here I have the possibility to make an if statement for each row retrieved from mysql, to e.g. put the value of a field inside another field (or casting field datatype and so on...). Which is something that I really need to do in my scenario (elaboration of db row).

Meanwhile, I have found out that performing many insert statement as above (each mysql row is inserted separately) is killing the performance.

So I'm thinking to switch to something like that, which prepares all the blocks of statements, and then insert it all in one block:

cur_msql.execute("SELECT customer_id, customer_name, contact_name, address, city, postal_code, country FROM mysqlcustomers")

args_str = ','.join(cur_psql.mogrify("(%(customer_id)s, %(customer_name)s, %(contact_name)s, %(address)s, %(city)s, %(postal_code)s, %(country)s)", x) for x in cur_msql)

try:
  cur_psql.execute("INSERT INTO psqlcustomers (customer_id, customer_name, contact_name, address, city, postal_code, country) \
                    VALUES " + args_str)
except psycopg2.Error as e:
  print "cannot execute that query", e.pgerror
  sys.exit("leaving early the script")

With this approach the script can be about 100x faster! But the problem with this is, that I can no more elaborate the data (transformation/conversion/casting...) because all the rows get inserted into an object and I'm not able to iterate through it anymore...

Is it possible to approach the problem in a way where I can:

Not kill the performance
Still elaborate/transform the data of each row?

There's no reason why you can't modify the data retrived from MySQL and still insert multiple rows at once into PostgreSQL, for example something like this:

fields = '''
customer_id customer_name contact_name address city postal_code country
'''.split()

cur_msql.execute("SELECT {} FROM mysqlcustomers".format(','.join(fields)))

rows = []
for row in cur_msql:
    if row['customer_name'] == NULL:
        row['customer_name'] = row['contact_name']
    rows.append(row)

params = '({})'.format(','.join('%s' for _ in fields))
query = "INSERT INTO psqlcustomers ({}) VALUES {}".format(
    ','.join(fields), ','.join(params for _ in rows))

cur_psql.execute(query, rows)


Checking some rules before a Telegram bot replies to a message So far, I've created a couple of programs, and the issue that comes up is how ugly and non-scalable they look.

For example, currently I'm working on a telegram bot. Before replying to a message I need to check that the message follows a couple of rules. In order to minimize the number of if/elif statements. I've even created a couple of functions that return True or False if my condition is met, but the code still looks very ugly:

# only if messages are not empty and description has a hyphen
if channel and (description and description[0] == '-'):

    if is_channel(channel) and is_length(description):

        if channel_is_size(channel):
            bot.reply_to(message, replies.success_add.format(channel))
            write_results(' '.join(message_words[1:]))
        else:
            bot.reply_to(message, replies.small_chan.format(min_channel_size))

    else:
        bot.reply_to(message, replies.enter_chan)

elif not description and channel:
    bot.reply_to(message, replies.enter_desc_error)
else:
     bot.reply_to(message, replies.enter_addmessage_error) # if empty

As you can see, these are the conditions for which the user message will be accepted into the database (it's a text file), and this can be seen right under the triple nested if statements.
I've tried to make this code look nicer many times over, but now it seems very messy and if I would want to add more conditions, which I am planning to do, these nested statements will grow out of proportion.

If you want to get rid of your nested code then you should make a function and use guard statements.

If we simplify your code a bit, we can have:

if C and D and D[0] == '-':
    return a  # Ignoring other code, for simplicity
elif C and not D:
    return b
else:
    return c

Instead you can have:

if C:
    if not D:
        return b
    elif D[0] == '-':
        return a
return c

If you use the guard clause then you get:

if not C:
    return c
if not D:
    return b
if D[0] != '-':
    return c
return a

Using the same logic, you can expand all your ifs to guard clauses. And the rest is easier than the above to change.
Which can leave you with:

def gen_response(channel, description, replies, message_words):
    if not channel:
        return replies.enter_addmessage_error

    if not description:
        return replies.enter_desc_error

    if description[0] != '-':
        return replies.enter_addmessage_error

    if not (is_channel(channel) and is_length(description)):
        return replies.enter_chan

    if not channel_is_size(channel):
        return replies.small_chan.format(min_channel_size)

    write_results(' '.join(message_words[1:]))
    return replies.success_add.format(channel)

bot.reply_to(message, gen_response(channel, description, replies, message_words))


Swapping every letter with every other letter in a string I am doing a project and I have come up with some code to swap every letter with every other letter in a string, only the project was to be "efficient as possible".

Example:

"rya" = yra, ray, ayr.. etc

I understand exactly how the code works. I would just like to make it efficient as possible.

public String testSwap(String word) {

char[] charArr = word.toCharArray();
char[] charArr2 = new char[word.length()];
String newString = "";

for(int i = 0; i < word.length(); i++) {
    for(int j = 0; j < word.length(); j++) {
        int count = 0;

        for(char c: charArr) {
            charArr2[count] = c;
            count++;
        }

        if(charArr2[i] != charArr[j]) {
            charArr2[i] = charArr[j];
            charArr2[j] = charArr[i];

            newString = new String(charArr2);

            if(graph.keySet().contains(newString)) {
                return newString;
            } else {
                newString = "";
            }
        }
    }
}
return newString;
}

NOTE: The if statement inside is for the current project I am doing.

to swap every letter with every other letter in a string

In other words you need all permutations of a string. Ok not all: no need in null set.

  I would just like to make it efficient as possible.

Just wanted to show you another solution. A crazy one with a recursion:
https://stackoverflow.com/questions/4240080/generating-all-permutations-of-a-given-string

public static void permutation(String str) {
    permutation("", str);
}

private static void permutation(String prefix, String str) {
    int n = str.length();
    if (n == 0) System.out.println(prefix);
    else {
        for (int i = 0; i < n; i++)
            permutation(prefix + str.charAt(i), str.substring(0, i) + str.substring(i+1, n));
    }
}

Quick solution to get it in one line with commas:

public static String permutation(String str) {
        String res = permutation("", str);
        return res.substring(0, res.length()-2);
    }

private static String permutation(String prefix, String str) {
    String res = "";
    int n = str.length();
    if (n == 0) res = res + prefix + ", ";
    else {
         for (int i = 0; i < n; i++)
             res = res + permutation(prefix + str.charAt(i), str.substring(0, i) + str.substring(i+1, n));
    }
    return res;
 }

Output:
rya, ray, yra, yar, ary, ayr

Less code, but definitely not the most efficient: calling charAt and substring a lot of time - not a good way.

In that link there are alot of other examples how to do it.


Shifting records in SQL Database while sorting with algorithm I have a table in a SQL Server database, which holds information of some images, and the relevant gallery of them. The columns are like:

ImageId, GalleryId, Order

I have a unique key on GalleryId-Order columns, that is, each image should have a unique order in its gallery.

A business requirement is to change the place (order) of an image, inside the gallery. For example, image at the order of 16, might be replaced at the new order 7. However, all the images should shift, if necessary, to provide space for this sorting operation. In this example, images with the order 7, 8, 9, ..., 15 should shift forward one level, so that the place 7 would become empty for the mentioned image.

I've written a procedure for this:

create procedure MoveImage(@imageId int, @newOrder int)
as
begin
    -- Finding the related gallery
    declare @galleryId int;
    select @galleryId = GalleryId
    from Images
    where ImageId = @imageId

    -- Finding the count of images inside this gallery
    declare @mediaCount int;
    select @mediaCount = COUNT(ImageId)
    from Images
    where GalleryId = @galleryId

    -- Finding the current order of the image
    declare @currentOrder int;
    select @currentOrder = [Order]
    from Images
    where ImageId = @imageId

    -- Temporary shifting the current media to one order above all, to bypass the 'Each Media Should Have a Unique Order in its Gallery' unique index
    update Images
    set [Order] = @mediaCount + 1
    where ImageId = @imageId

    -- Shifting all media of the gallery between the current order and the new order, one order forward, or backward
    if @newOrder < @currentOrder
    begin
        update Images
        set [Order] = [Order] + 1
        where GalleryId = @galleryId and [Order] >= @newOrder and [Order] <= @currentOrder
    end
    else
    begin
        update Images
        set [Order] = [Order] - 1
        where GalleryId = @galleryId and [Order] <= @newOrder and [Order] >= @currentOrder
    end

    -- Inserting the image at the specified order
    print @newOrder
    update Images
    set [Order] = @newOrder
    where ImageId = @imageId
end
go

It works, just fine (at least as much as I've tested it). However, I don't feel Okay with this procedure, I think it might become easier and more effective. Can anyone improve this T-SQL code?

PS: parameters are checked before being passed to this procedure, so presume that there is nothing wrong with parameters, like negative @newOrder, etc.

There is a much, much, much easier way to do this. You can use real values for order, so when you want to move the row from position 16 to position 7 you give it an order value which is the average between the order values of rows 6 and 7.

The precision of real numbers is huge, and I presume that the re-orderings of images in your database will be happening at human reaction speeds, so you probably won't run into precision problems during the remainder of the expected lifetime of the universe. But if you are really insecure about the possibility of precision problems, you can rectify your table every once in a while, by reading the rows sorted by their order, and writing them into a new table, re-assigning the order values by copying them from an integer identity field. Then you delete the old table and rename the new table to the old name.


Checking custom URLs and user IDs Is there a better way to do this? I'm kinda ending up having a lot of if statements.

if params[:custom_url] || params[:user_id]
    if !params[:user_id].blank?
      @user = User.find(params[:user_id]).first
      @url = @user.custom_url
    else
      @url = params[:custom_url]
      @user = User.where(:custom_url => @url).first
    end
end

Not a big fan of case..when:

@user = User.find_first(params[:user_id]) if params[:user_id]
@user ||= User.find_by_custom_url(@url)

@url = params[:custom_url] || @user.custom_url


Prime Number Generator algorithm optimization I've implemented a simple program that finds all prime numbers between two integer inputs using Sieve of Eratosthenes, but the online judge is still complaining that the time limit is exceeding. Maybe I'm doing something wrong, but how can I optimize (or debug if necessary) my code?

#include <iostream>
#include <cmath>

using namespace std;

int main()
{
    int t;
    unsigned int i, j, k;
    cin >> t;
    int m, n;
    cin >> m >> n;
    if (n - m <= 100000 && m <= n && 1 <= m && n <= 1000000000)
    {
        int a[n - m + 1];
        for (j = 0; j < n - m + 1; ++j)
        {
            a[j] = m + j;
        }
        for (j = 2; j <= sqrt (n); ++j)
        {
            for (k = 0; k < n - m + 1; ++k)
            {
                if (a[k] % j == 0 && a[k] != 0)
                {
                    break;
                }
            }
            for (; k < n - m + 1; k = k + j)
            {
                if (a[k] != j)
                {
                    a[k] = 0;
                }
            }
        }
        if (a[0] == 1)
        {
            a[0] = 0;
        }
        for (j = 0; j < n - m + 1; ++j)
        {
            if (a[j])
            {
                cout << a[j] << endl;
            }
        }
        cout << endl;
    }
    return 0;
}

You say you have implemented a Sieve of Eratosthenes but the code doesn't do
what I understand from the Wiki page.  Maybe you have changed it now, as your
comment to Loki's comment implies.

Here are a few extra comments to add to those from @LokiAstari

Use of unsigned is not necessary or desirable. Just use int unless there
is good reason for an unsigned (e.g. when you have a bitmap or similar)
You repeat the expression n - m + 1 several times. Although the compiler
will optimise this away, the reader has to read it each time.  Best to put
such an expression into a constant, e.g.:

 const int range = n - m + 1;

Variables i and t are functionally unused.
main takes argc/argv parameters. Also, it is normal not to do all the work
in main, although for a small program like this that is perhaps irrelevant.
Your initial if clause might be better inverted to avoid the indentation
caused.  Also note that it is easy to mistake the precedence of operators, so
brackets are safer:

if ((n - m > 10000000) || (m >= n) || (m < 0) || (n > 1000000000)) {
    // print an error
    exit(1);
}

In general it is better to use functions to aeparate parts of a program into
small units that are easily tested or otherwise verified.  The overhead of a
function call is often zero (the compiler can optimise the call away). So
some of your loops could be extracted into functions - but only if using a
function is 'makes sense' (which takes experience to decide).
The Sieve of Eratosthenes doesn't really need any division.  It is probably
better to compute all primes from 2 upwards and then just print the ones
requested rather than trying to start at the specified minimum bound.
Nested loops are in my opinion best avoided.
You say you used short names to keep the program short.
But even your short names are not well
considered.  For example, the first loop uses j as a loop variable to
initialise the sieve: access to the sieve uses a[j].  The following loop
uses j as a loop variable but it is never used to access the sieve -
instead the inner loop uses k as a variable: if (a[k] % j == 0.....
Maybe this is nit-picking but consistency is an important aspect of
programming.  Note also that loop variables are often best defined in the
loop:

for (int i = 0; i < range; ++i) {
    etc
}

Here is my attempt at the sieve, for what it is worth:

#include <stdio.h>
#define SIZE 1000000

static void prime_set(char *sieve, int n, size_t size)
{
    for (int i = n + n; i < (int) size; i += n) {
        sieve[i] = 1;
    }
}

static int prime_next(char *sieve, int n, size_t size)
{
    for (int i = n + 1; i < (int) size; ++i) {
        if (!sieve[i])
            return i;
    }
    return 0;
}

static void prime_print(const char *sieve, int min, int max)
{
    for (int i = min; i < max; ++i) {
        if (!sieve[i]) {
            printf("%d\n", i);
        }
    }
}

int main(int argc, char **argv)
{
    const int min = 1;
    const int max = SIZE;
    int prime = 1;
    char sieve[SIZE] = {0};

    while ((prime = prime_next(sieve, prime, SIZE)) != 0) {
        prime_set(sieve, prime, SIZE);
    }
    /* remaining zeros in sieve are primes */
    prime_print(sieve, min, max);
    return 0;
}


Clean and tidy Python code for dictionary look up and API interactions I am new to programming and entirely self-taught. Below is a program I wrote to retrieve text content via an API and scan the text for important words. If the word is found URI is returned. The API returns data in JSON. The important words and URIs are linked via a dictionary. It works. I'm at a point in my programming education where I need to start focusing on writing clean code. I have a book about it, but there's something about peer review that is so much more helpful. Here is the code with commenting.
Any ideas on how I can make this cleaner?

#!/usr/bin/env python
# coding: utf-8
from __future__ import division
import urllib, json

#bio_dict is the dictionary used to assign DBpedia URIs. When a specific key is
#found in the text, the value URI is returned. Any dictionary can be used here.

bio_dict = eval(open('biodict.txt').read())

in_file_name = "text_object_id1.txt"
in_file = open(in_file_name, 'r')
out_file_name = "all_uri.txt"
out_file = open(out_file_name, 'w')

#At this point, you have imported all the modules you need, read the dictionary
#you will use to the assign the URIs and opened all the files you will be using. The
#InFile should be a list of DataObject IDs. The OutFile will hold your results.

def data_object_url(line):
    return 'http://eol.org/api/data_objects/1.0/' + line.strip() + '.json'

#data_object_url is a function that will create the URL needed to query the API
#for each data object ID in the list

for line in in_file:
    results = urllib.urlopen(data_object_url(line)).read()
    print line
    print len(results)
    data = json.loads(results)
    descriptions = data ['dataObjects'][0]['description'].lower()
    for k, v in bio_dict.iteritems():
        if k in descriptions:
            out_file.write(','.join([line.strip(),k,v + '\n']))
in_file.close()
out_file.close()

#This code goes over each line in the in_file (a list of text object IDs), retrieves
#them from the API in json format, pulls out the description text and then iterates over
#each key in the dictionary. If the key is found the value is returned and the system
#moves on to the next key. If the key is not found, the system returns nothing and moves
#on to the next key. Line 34 controls the format of the output. Lines 28 and 29 serve to
#show progress as the program runs.

I have a few pointers, a lot of them coming from Python's offical style guide:

ALWAYS use with syntax when dealing with files:

with open('some_file.txt', 'r') as file:
    # Do stuff

# file will be closed by now
foo = 'Hello World!'

This syntax prevents any errant bugs like forgetting to close a file, which you do here:

bio_dict = eval(open('biodict.txt').read())

Use format to create strings with variables. This is the easiest way to create strings without having to use ugly string concatenation:

# This functions...
def data_object_url(line):
    return 'http://eol.org/api/data_objects/1.0/' + line.strip() + '.json'

# But this is better, more Pythonic
def data_object_url(line):
    return 'http://eol.org/api/data_objects/1.0/{}.json'.format(line.strip())

Different module imports should not go on the same line:

import urllib, json

They should have their own line. If you are importing submodules, then you can have multiple imports on the same line:

from my_module import some_func, other_func

Pay attention to function and variable names; make them descriptive.

Function names should start with a verb and describe what they do: instead of data_object_url use something like get_object_url.

Also, make variable names descriptive; erring on the side of being too wordy than not wordy enough: instead of for k, v in bio_dict.iteritems() use for key, value in bio_dict.iteritems().
When writing comments, only say what needs to be explained. Comments should not explain the obvious but instead should explain the complex or the reasons behind the code. A lot of the comments you have would actually be more fitting as docstrings.
Look into list comprehensions. In my code below I was able to take your for loop into a single statement. This also made writing to the output file much more Pythonic.

Here is what your code looks like with my suggestions implemented:

#!/usr/bin/env python
# coding: utf-8

import urllib
import json

with open('biodict.txt', 'r') as file:
    bio_dict = eval(file.read())

in_file_name = "text_object_id1.txt"
out_file_name = "all_uri.txt"

def get_object_url(line):
    return 'http://eol.org/api/data_objects/1.0/{}.json'.format(line)

out_file_text = []
with open(in_file_name, 'r') as file:
    for line in in_file:
        line = line.strip()
        results = urllib.urlopen(get_object_url(line)).read()
        data = json.loads(results)

        descriptions = data['dataObjects'][0]['description'].lower()
        out_file_text += [','.join(line, key, bio_dict[key])
                             for key in bio_dict if key in descriptions]

with open(out_file_name, 'w') as file:
    file.write('\n'.join(out_file_text))


Is there any performance increase converting search term to lower case before LINQ Please have a look at the following 2 snippets:

var query = _ctx.Notes.Where(...);
var searchLower = searchTerms.ToLower();
query = query.Where(a => a.Comment.ToLower().Contains(searchLower))

vs

var query = _ctx.Notes.Where(...);
query = query.Where(a => a.Comment.ToLower().Contains(searchTerms.ToLower()))

Is there any peformance increase by converting the search terms before carrying out the LINQ query?

Instead of Contains(), use IndexOf() which takes a comparer:

var query = _ctx.Notes.Where(...);
query = query.Where(a => a.Comment.IndexOf(searchTerms, StringComparison.OrdinalIgnoreCase) >= 0)


String Zigzag Conversion Overview

The string 0123456789 is written in a zigzag pattern (following the path going all the way down, then diagonally moving towards the top right) using a given number of rows. Like this example in the case of 3 rows.

0   4   8
1 3 5 7 9
2   6

This string is then read line by line and converted into a new string. In this case this new string from the zigzag will be 0481357926.

I'd like to understand any optimizations I can make using C++17 features, edge cases I may gave missed, possible overflows errors, etc.

#include <cmath>
#include <iostream>
#include <string>

class Solution {
    public:
        std::string convert(const std::string& s, const int numRows) {
            if (numRows == 1) { return s; }
            const int size = s.size();
            std::string converted = "";
            const int num  = numRows + (numRows - 2);
            int row = 0;
            int i   = 0;
            int sub = 0;
            bool useDiff = true;
            while (row < numRows) {
                converted += s[row];
                if (row == 0 || row == numRows - 1) {
                    i = row + num;
                    while (i < size) {
                        std::string str(1, s[i]);
                        converted.append(str);
                        i += num;
                    }
                } else {
                    int diff = std::abs(num - sub);
                    i = diff;
                    while (i < size && row + i < size) {
                        if (useDiff) {
                            std::string str(1, s[row + i]);
                            converted.append(str);
                            useDiff = false;
                            i += sub;
                        } else {
                            std::string str(1, s[row + i]);
                            converted.append(str);
                            useDiff = true;
                            i += diff;
                        }
                    }
                }
                useDiff = true;
                sub += 2;
                row++;
            }

            return converted;
        }
};

int main()
{
    Solution solution;
    std::string s = "0123456789";
    int numRows = 3;
    std::cout << s << '\n';
    s = solution.convert(s, numRows);
    std::cout << s << '\n';
}

Don't use an alibi-class. It doesn't make the code any more OO, even in the cases it would be desirable, it just adds to the clutter.
Solution and convert are such generic names. Naming is hard, but good names really help.
You know an int need not be positive? An unsigned type may be more appropriate for a count, but even then you should handle zero.
Only if you need a null-terminated string should you consider a parameter of type std::string const&. In all other cases, using std::string_view is more efficient.
While creating a temporary string of length 1 probably doesn't cause dynamic allocation due to small-string-optimisation (or allocation ellision), doing so just to add a character to the result-string is cumbersome and potentially quite inefficient (in theory, the compiler could optimise it all out). Just append it directly.
If you assign directly, diff is gone.
useDiff doesn't make a difference, so remove it.
Consider adding additional test-cases. For that, you should rewrite main().

See the modified code live on coliru:

#include <string>
#include <string_view>
#include <stdexcept>

auto zigzag(std::string_view s, std::size_t cRows) {
    if (!cRows)
        throw std::domain_error("Must output at least one row.");
    if (!cRows || cRows >= s.size())
        return std::string(s);
    std::string r;
    r.reserve(s.size());
    const auto period = 2 * cRows - 2;
    for (auto row = 0 * cRows; row < cRows; ++row) {
        auto delta = 2 * row;
        auto delta2 = period - delta;
        if (!delta) delta = period;
        if (!delta2) delta2 = period;
        for (auto i = row; i < s.size(); i += delta) {
            r += s[i];
            std::swap(delta, delta2);
        }
    }
    return r;
}

#include <iostream>

struct test {
    std::string_view in;
    std::size_t n;
    std::string_view out;
};

int main() {
    constexpr test cases[]{
        "0123456789", 3, "0481357926",
        "", 1, "",
        "0123456789", 10, "0123456789",
        "0123456789", 2, "0246813579",
    };
    for (auto&& x : cases) {
        std::cout << "zigzag(\"" << x.in << "\", " << x.n << ") == \"" << x.out;
        auto r = zigzag(x.in, x.n);
        if (r == x.out)
            std::cout << "\": PASSED\n";
        else
            std::cout << "\": FAILED, got \"" << r << "\"\n";
    }
}


Getting/setting default values from my App.config Imagine that I need a color palette for my Winforms application to have a consistent look.

What I did was create a static helper class and helper methods that I can call from anywhere in my code, and invoke what I need from the App.settings file.

Here for example, I am getting the school name from the App.config file, so I can sell this application to other schools with minimal changes on my part.

<?xml version="1.0" encoding="utf-8" ?>
<configuration>
  <appSettings>
    <add key="schoolName" value="Uboldi"/>
  </appSettings>
</configuration>

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Configuration;

namespace Uboldi.Helpers
{
    public static class CustomizationHelper
    {
        public static string GetSchoolName()
        {
            return ConfigurationManager.AppSettings["schoolName"];
        }
    }
}

Usage:

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Windows.Forms;
using Uboldi.Helpers;

namespace Uboldi
{
    public partial class MainForm : Form
    {
        public MainForm()
        {
            InitializeComponent();
            LoadFormTitle();
        }

        private void LoadFormTitle()
        {
            var schoolName = CustomizationHelper.GetSchoolName();
            this.Text = String.Format("Sistema {0} - Pagina Principal", schoolName);
        }
    }
}

Are there any glaring mistakes I'm making by choosing this type of architecture?

While somewhat of a tangent from your question but you may find it helpful nonetheless.

I would recommend looking at a custom ConfigurationSection which allows you to define more complex configuration hierarchies that are strongly-typed.  I find it much easier to use a custom configuration section than having to remember a bunch of magic strings in the appSettings element and also allows you to specific which values are required, what the defaults values are, etc.

Using a custom configuration section you could create a configuration type like:

public class UboldiConfigurationSection : System.Configuration.ConfigurationSection {
    [ConfigurationProperty("schoolName")]
    public string SchoolName {
        get { return (string)this["schoolName"]; }
        set { this["schoolName"] = value; }
    }
}

Then to load that configuration type:

public static class UboldiApplication {
    public static UboldiConfigurationSection Config { get; internal set; }

    public static void Initialize() {
        Config = ConfigurationManager.GetSection("uboldi") as UboldiConfigurationSection;
    }
}

The app.config then would look something like this:

<configuration>
    <configSections>
        <section name="uboldi" type="Uboldi.UboldiConfigurationSection, Uboldi" />
    </configSections>
    <uboldi schoolName="Fillmore Central" />
</configuration>

Lastly, you use the configuration by:

public void Test() {
    //This only needs to be done once, presumably in your Program.Main method
    UboldiApplication.Initialize();

    var name = UboldiApplication.Config.SchoolName;
}

A couple of notes:

You'll need to reference the System.Configuration assembly as it's not usually referenced in VS by default.
The ConfigurationManager.GetSection("uboldi") is expecting the name of the section in the app.config file.  You'll note that this matches in the example above.
The section element in the app.config file uses the standard .Net type name convention to locate the specified configuration section.  In this example I am assuming that the UboldiConfigurationSection type is the Uboldi namespace and in an Uboldi assembly (dll or exe).
You can add hierarchy by creating ConfigurationElement sub classes and using them as properties in your configuration section and elements.
The link above is for a Web.config, but the same thing is possible in an app.config file.


A function that implements a query I have some parameterized queries in Access 2010.
They are often used in VBA functions having the same parameters.
In such cases, I give the query and function the same name.
The following is such a function (from my actual project):

Public Function get_assignments(e_id As Long, yr As Integer, wk As Integer) As DAO.Recordset
    Dim db As DAO.Database
    Set db = CurrentDb
    Dim qd As DAO.QueryDef
    Set qd = db.QueryDefs!get_assignments
    qd.Parameters![e_id] = e_id
    qd.Parameters![year] = yr
    qd.Parameters![week] = wk
    Set get_assignments = qd.OpenRecordset
    qd.Close
End Function

Both the query and function are named get_assignments and things work fine.

Am I asking for trouble? While I don't usually add "type" suffixes to names, I could rename the query as get_assignments_qry or even rename the function as get_assignments_fn. What's best?

Feel free to give feedback on any aspects of this code.

Namespace scope and pollution is often a problem in programming, and when reading code, if there are two usages of a particular identifier, a reader may not understand exactly which one you mean, and the complier may have different scope rules than you expect, so it is worthwhile to not re-use names.

In this particular case, consider using query instead of get in the public API

Public Function query_assignments(e_id As Long, yr As Integer, wk As Integer) As DAO.Recordset


Class / Superclass / Specific class matching condition, can it be simpler? I recently built a way to automagically dismiss firstResponders for an app I'm building. Is there a more efficient way to approach this?

- (void)resignFieldStackRespondersExcept:(id)fieldException
{
    for (id field in self.fieldStack) {
        if ([field isKindOfClass:[fieldException class]] ||
            ([[field class] isSubclassOfClass:[fieldException class]] || [[fieldException class] isSubclassOfClass:[field class]]) ||
            (([[field class] isSubclassOfClass:[UITextField class]] && [[fieldException class] isSubclassOfClass:[UITextView class]]) ||
             ([[field class] isSubclassOfClass:[UITextView class]] && [[fieldException class] isSubclassOfClass:[UITextField class]]))) {
            continue;
        }

        [self resignField:field];
    }
}

- (void)resignField:(id)field
{
    if ([field isKindOfClass:[AccessoryPicker class]]) {
        [field closePicker:YES];
    }

    if ([field isKindOfClass:[TagsTextView class]]) {
        [field saveTrailingTag];
    }

    if ([field isKindOfClass:[UITextField class]] || [field isKindOfClass:[UITextView class]]) {
        [field resignFirstResponder];
    }
}

There's not really enough information or context here to make any sort of comment on the efficiency of this pair of methods, but it seems like this could simply be narrowed down into a single method.

We're iterating through a collection so we can call resignField: on specific classes only, and all of the logic in resignField: already works on specific classes only.  The only thing that's left is the check for whatever this exception is:

- (void)resignFieldStackRespondersExcept:(id)fieldException {
    for (id field in self.fieldStack) {
        if ([field isKindOfClass:[AccessoryPicker class]]) {
            [field closePicker:YES];
        } else if ([field isKindOfClass:[TagsTextView class]]) {
            [field saveTrailingTag];
        } else if ([field isKindOfClass:[UITextField class]]
                   && ![[fieldException class] isSubclassOfClass:[UITextView class]]) {
            [field resignFirstResponder];
        } else if ([field isKindOfClass:[UITextView class]]
                   && ![[fieldException class] isSubclassOfClass:[UITextField class]]) {
            [field resignFirstResponder];
        }
    }
}

Now we can eliminate the second method entirely.  The first method is more readable and unnecessary checks have been removed.