Variadic function with restricted types I am designing a class which (among other things) acts as a vertex in an undirected graph.

#include <iterator>
#include <string>

class Node
{
public:
    explicit Node(const std::string& name);
    Node(const Node&);
    Node(Node&&);

    class iterator
        : public std::iterator<std::random_access_iterator_tag, const Node>
    {
        //...
    };
    class container_proxy
    {
    public:
        iterator begin() const;
        iterator end() const;
    private:
        //...
    };

    container_proxy neighbors() const;

    add_link(Node& other);

    //...

private:
    //...
};
// to support: for (const Node& neighbor : node.neighbors())
Node::iterator begin(const Node::container_proxy&);
Node::iterator end(const Node::container_proxy&);

Now I want to define a function link_complete which:

takes any number of arguments
allows non-const Node lvalues (parameter type Node&)
allows non-const Node rvalues (parameter type Node&&)
does not allow any other parameter type.

An example usage:

Node ret_by_value();

void example_test(Node& x, Node&& y)
{
    link_complete(x, ret_by_value(), std::move(y), Node("test"));
}

Here's the solution I came up with:

#include <initializer_list>
#include <type_traits>

class Node {
    //...
public:
    static void link_complete(std::initializer_list<Node*> node_list);
};

template <typename ... Types>
struct template_all; // UNDEFINED

template <>
struct template_all<>
  : public std::true_type {};

template <typename ... Types>
struct template_all<std::false_type, Types...>
  : public std::false_type {};

template <typename ... Types>
struct template_all<std::true_type, Types...>
  : public template_all<Types...>::type {};

template <typename ... Args>
auto link_complete(Args&& ... args) ->
  typename std::enable_if<
    template_all<
      typename std::is_same<
        typename std::remove_reference<Args>::type,
        Node
      >::type ...
    >::value,
    void
  >::type
{
    Node::link_complete( { &args... } );
}

It seems to work as required, but I'm wondering if there was a simpler or "prettier" way to do it, or maybe a way to improve that template_all helper type.  All C++11, TR1, and Boost features are welcome.  I did notice boost::mpl::and_ looks similar to my template_all helper type, but Boost MPL seems like a tricky learning curve and/or overkill for this problem.

struct has public inheritance

By default, struct has public inheritance. Therefore, you don't need to manually specify it every time. You can write shorter (and so more readable) code:

template <>
struct template_all<>
  : std::true_type {};

template <typename ... Types>
struct template_all<std::false_type, Types...>
  : std::false_type {};

template <typename ... Types>
struct template_all<std::true_type, Types...>
  : template_all<Types...>::type {};

Since type traits generally use many templates, you don't want to add even more extra keywords all over the place.

Use alias templates

If you have acces to a compiler that supports some C++14 features, you will probably want to use the alias templates for transformation traits to get rid of many typename and ::type. Unfortunately, the alias templates for the query traits (is_*) have not been accepted, so you have to use std::is_same.

template <typename ... Args>
auto link_complete(Args&& ... args) ->
  std::enable_if_t<
    template_all<
      typename std::is_same<
        std::remove_reference_t<Args>,
        Node
      >...
    >::value,
    void
  >
{
    Node::link_complete( { &args... } );
}

static_assert is great

Instead of using SFINAE for link_complete, you should use static_assert since your function does not have any overload. It will allow you to give a meaningful error message instead of a somewhat obscure SFINAE error message:

template <typename ... Args>
void link_complete(Args&& ... args)
{

    static_assert(
        template_all<
            typename std::is_same<
                std::remove_reference_t<Args>,
                Node
            >::type...
        >::value,
        "link_complete arguments must be of the type Node"
    );

    Node::link_complete( { &args... } );
}

You may want to use std::decay

std::decay calls std::remove_reference and std::remove_cv internally. If needed, it also calls std::remove_extent if T is an array type and calls std::add_pointer if T is a function type. In short:

  This is the type conversion applied to all function arguments when passed by value.

That allows you to check for what we generally think of as "same types":

template <typename ... Args>
void link_complete(Args&& ... args)
{

    static_assert(
        template_all<
            typename std::is_same<
                std::decay_t<Args>,
                Node
            >::type...
        >::value,
        "link_complete arguments must be of the type Node"
    );

    Node::link_complete( { &args... } );
}

Rethink template_all

All in all, your goal is to test boolean conditions on types. Since the query traits have a static constexpr bool value member, you may want to create a template_all that checks for true and false (like std::enable_if or std::conditional) instead of checking for std::true_type and std::false_type. That would make your intent clearer, but it would be more like a all_true template.

template <bool...>
struct all_true; // UNDEFINED

template <>
struct all_true<>
  : std::true_type {};

template <bool... Conds>
struct all_true<false, Conds...>
  : std::false_type {};

template <bool... Conds>
struct all_true<true, Conds...>
  : all_true<Conds...> {};

template <typename ... Args>
void link_complete(Args&& ...)
{
    static_assert(
        all_true<
            std::is_same<
                std::decay_t<Args>,
                Node
            >::value...
        >::value,
        "must have Node types"
    );

    Node::link_complete( { &args... } );
}


Display or hide drawer with onClick The following is my code for my "drawer" to be display or hidden on the basis whether it has been clicked or not. I want to make this better as personally its messy. Any tips?

//Checks if the page loaded is of specific type
if(window.location.href.indexOf("home.php") > -1) {
      //Handles the displaying of the side bar on the home page.
      var button = document.getElementById('navIcon');
      button.onclick = function() {
          var div = document.getElementById('drawer');
          if (div.style.display !== 'block') {
              div.style.display = 'block';
              //If the document is over 900px. Readjusts the grid, Otherwise overlay
              if(document.body.clientWidth > 900) {
                  document.getElementById('resGrid').style.width = 'calc(100% - 240px)';
              }
          }
          else {
              div.style.display = 'none';
              document.getElementById('resGrid').style.width = '100%';
          }
      };
    }

button.onclick = function() {

Use addEventListener instead. That way, you can add as many handlers as you want without replacing any previous ones assigned.

if (div.style.display !== 'block') {

Instead of using inline CSS, put them in stylesheets and define the styles in classes. Then have JS assign/remove classes instead to apply/remove. The problem is that 1) You have CSS in your JS and 2) inline CSS have very high specificity. The only way to override them is to replace the inline style value, or use !important on the stylesheet.

if(document.body.clientWidth > 900) {

If you also use CSS, this can easily be remedied by media queries.

if(window.location.href.indexOf("home.php") > -1) {

You probably want to use a more robust routing library like Page.js to do this.


Interleave two strings This program is designed to interleave two strings, which are input as command line arguments. I'd appreciate any feedback as I am new to C and self-taught.

Edit: Since this is an exercise in becoming more familiar with the intricacies of strings and memory allocation, the output should be stored before being printed.

#include <stdio.h>  // printf
#include <stdlib.h> // malloc, EXIT_FAILURE/SUCCESS
#include <string.h> // strlen

int main(int argc, char const *argv[])
{
  // Expecting: exe name, str_a, str_b
  if (argc != 3)
  {
    return EXIT_FAILURE;
  }

  size_t str_a_len = strlen(argv[1]);
  size_t str_b_len = strlen(argv[2]);
  if (str_a_len != str_b_len)
  {
    return EXIT_FAILURE;
  }

  // +1 for NULL byte
  size_t str_c_len = str_a_len + str_b_len + 1;
  char * str_c = malloc(str_c_len);

   // No free memory
  if (strlen(str_c) == 0)
  {
    return EXIT_FAILURE;
  }

  // Zip letters, AAA BBB => ABABAB
  for (int i = 0, n = str_c_len; i < n; ++i)
  {
    str_c[i] = (i % 2 == 0) ? argv[1][i / 2] : argv[2][i / 2];
  }

  printf("%s\n", str_c);

  return EXIT_SUCCESS;
}

I am compiling with:

gcc *.c -Wall -g -o zip.exe

You allocate a memory block:

char * str_c = malloc(str_c_len);

and then pass it to the strlen():

if (strlen(str_c) == 0)
    ....

Apart from the str_c == NULL condition, described by @sanecz, you can not expect any meaningfull result from that test. It can even cause a crash!
That's because malloc() is not defined to initialize the allocated block of memory in any way. It can contain a zero byte at any position, which will cause strlen() to return any value less than the requested size of the block str_c_len; or it can contain no NUL byte at all, which will cause strlen() to scan past the allocated block and trigger an Undefined Behavior.

Additionally, you do not need the allocated block. If you want only to make an interleaved output, just do that: print characters in appropriate order.

int main(int argc, char const *argv[])
{
    // Expecting: exe name, str_a, str_b
    if (argc != 3)
    {
        return EXIT_FAILURE;
    }

    size_t str_a_len = strlen(argv[1]);
    size_t str_b_len = strlen(argv[2]);
    if (str_a_len != str_b_len)
    {
        return EXIT_FAILURE;
    }

    for (int i = 0; i < str_a_len; ++i)
    {
        putchar( argv[1][i] );
        putchar( argv[2][i] );
    }

    putchar('\n');

    return EXIT_SUCCESS;
}

EDIT

As for the comment 'this was mainly an exercise in string
manipulation': if you want to improve your programming skills, and string
manipulation among them, then I'd recommend to organize your program
into separate functions. For example:

let main() fetch input data and pass it to some processing routine,
then print the output;
let some MakeInterleavedString() validate input and prepare output
buffer;
and let some InterleaveTwoStrings() do actual processing.

For example:

char *InterleaveTwoStrings(const char *l, const char *r, char *sum)
{
    // assume input strings l, r of equal length
    // and output buffer sum twice that long

    char *out = sum;      // the place where output goes
    while(*l != 0)        // input not exhausted yet
    {
        *out ++ = *l ++;  // copy one char and advance input
        *out ++ = *r ++;  // and output positions
    }
    *out = 0;             // terminate the output string

    return sum;           // return the filled buffer
}

char *MakeInterleavedString(const char *l, const char *r)
{
    // test if strings l, r are equal length,
    // prepare a buffer to accomodate l,r contents
    // interleave data into a buffer and return it;
    // return NULL on error

    int llen = strlen(l), rlen = strlen(r);
    if(llen == rlen)
    {
        if(char *sum = malloc(2*llen + 1))    // +1 for terminating NUL char
            return InterleaveTwoStrings(l, r, sum);
    }

    return NULL;   // on (llen != rlen) or malloc() failure
}

int main(int argc, char const *argv[])
{
    // Expecting: exe name, str_a, str_b
    if (argc == 3)
    {
        const char *str_a = argv[1];
        const char *str_b = argv[2];

        if (const char *str_c = MakeInterleavedString(str_a,str_b))
        {
            printf("%s\n", str_c);
            free(str_c);
            return EXIT_SUCCESS;
        }
    }
    return EXIT_FAILURE;
}

Please note the call to free() function – it's not that important in such simple program, as all the allocated memory goes back to the operating system on the program termination. However, in library functions we should always release temporary buffers as soon as they are no longer used. Otherwise we may forget to release them later, and we may cause so called 'memory leaks', when long running process consumes more and more memory for no reason.


Simple DRY Rails - check if a model belongs to a user I can clearly see I'm repeating myself, but I don't know how to refractor this without making it really ugly. What I have:

class ChildModelController < ApplicationController

    def new
        @model = ParentModel.find(params[:id])
        # Does the current user have permissions on the parent model
        if signed_in?
          unless current_user.admin? or current_user.id == @model.user_id
            flash[:error] = t('errors.messages.not_authorized')
            flash.keep
            redirect_to root_path
          end
        else
          unless session and session[:session_id] == @model.author_session_token
            flash[:error] = t('errors.messages.not_authorized')
            flash.keep
            redirect_to root_path
          end
        end
        #do stuff to the model
    end

end

The only way I can think to make it smaller is to make it ugly:

class ChildModelController < ApplicationController

    def new
        @model = ParentModel.find(params[:id])
        # Does the current user have permissions on the parent model
        if (!signed_in? and session and session[:session_id] == @model.author_session_token ) || (signed_in and current_user.admin? or current_user.id == @model.user_id)
            flash[:error] = t('errors.messages.not_authorized')
            flash.keep
            redirect_to root_path
          end

        #do stuff to the model
    end

end

Am I missing an even easier way?

I suggest you extract two methods: authorized? to check if the user is authorized, and not_authorized to handle behaviour. You would then end up with this:

def new
  @model = ParentModel.find(params[:id])
  not_authorized unless authorized?
end

private

def authorized?
  if signed_in?
    # If you have your associations setup correctly you don't have
    # to check equality on id. You can just compare the objects
    current_user.admin? || current_user == @model.user
  else
    # See aside number 2 for why I removed session
    # session[:session_id] will return nil if it's not present
    # which is false in ruby, so it's safe to do this
    session[:session_id] == @model.author_session_token
  end
end

def not_authorized
  flash[:error] = t('errors.messages.not_authorized')
  flash.keep
  redirect_to root_path
end

But I suggest you think about using an authorisation library, or extracting authorisation logic into a policy object. This way you gain the benefit of DRY, and an extensible way to add and reuse permissions.

def new
  @model = ParentModel.find(params[:id])
  not_authorized unless authorized?(current_user, @model)
end

In a separate policy object you could:

# Choose a better name for this
class ModelPolicy
  attr_reader :user, :model

  def initialize(user, model)
    @user, @model = user, model
  end

  def authorized?
    user.admin? || user.id == model.user_id
  end
end

This example illustrates my point but it is not complete. You must wire the policy to the controllers, maybe include it in a concern. But it's not that hard.

There's a great gem called Pundit that gives you similar interface.

Asides

Try not to use unless with || and && conditions because it becomes confusing. It's clearer to use negation: !current_user. Some people are boolean impaired, including myself.
I'm not sure under what circumstances the session hash does not exist. So checking for its presence may be redundant.


How to load and get data from 2 files I want to load 2 files and show this data in 2 strings. I trying to do it like this and it works fine. But I think it is not the best solution... How to do more elegant solution?
var saleNumber1 = ""
var saleNumber2 = ""

override func viewDidLoad() {
        super.viewDidLoad()

        sale1()
        sale2()
}

func sale1() {

        let url = URL(string: "http://example.com/folder/sale1.txt")!

        let task = URLSession.shared.dataTask(with: url) {(data, response, error) in
            guard let data = data else { return }
            saleNumber1 = "\(String(data: data, encoding: .utf8)!)"
        }

        task.resume()

    }

    func sale2() {

        let url = URL(string: "http://example.com/folder/sale2.txt")!

        let task = URLSession.shared.dataTask(with: url) {(data, response, error) in
            guard let data = data else { return }
            saleNumber2 = "\(String(data: data, encoding: .utf8)!)"
        }

        task.resume()

    }

Time to learn how to DRY off your WET code!
(DRY -> "Don't Repeat Yourself"; WET -> "Write Everything Twice")
Note how the only difference between your two functions is a URL and a change in data, effectively "changing input results in changing output." Consider consolidating the two into one generic function that takes a variable url in as input and returns data which can be store in a variable of your choice.
var saleNumber1 = ""
var saleNumber2 = ""

override func viewDidLoad() {
    super.viewDidLoad()

    saleNumber1 = sale("http://example.com/folder/sale1.txt")
    saleNumber2 = sale("http://example.com/folder/sale2.txt")
}

func sale(path: String) -> String {
    let url = URL(string: path)!
    let saleNumber = ""
    let task = URLSession.shared.dataTask(with: url) {(data, response, error) in
        guard let data = data else { return }
        saleNumber = "\(String(data: data, encoding: .utf8)!)"
    }

    task.resume()

    return saleNumber
}


Mixing business logic and presentation layer in enum type Let's assume I have an enum type with currency:

public enum CurrencyType {

    USD("American Dollar", "USD"),
    EUR("Euro", "EUR"),
    JPY("Japan Yen", "JPY");

    private final String fullName;
    private final String shortName; // enum valirables can change in the future, so I do not use Currency.name()

    private CurrencyType(String fullName, String shortName) {
        this.fullName = fullName;
        this.shortName = shortName;
    }

    public String getFullName() {
        return this.fullName;
    }

    public String getFullNameLocalized(ResourceBundle bundle) {
        // returns the currency name using some ResourceBundle with localized strings
    }

    public String getShortName() {
        return this.shortName;
    }
}

Now, somewhere in the presentation layer I say:

CurrencyType curr = CurrencyType.EUR;
// ...
labelCurrencyTypeName.setText(curr.getFullNameLocalized(bundle));
labelCurrencyTypeSymbol.setText(curr.getShortName());

in order to see the "EUR" string in an appropriate place.

Everything is perfect, but such approach mixes the Model and View together, doesn't it? CurrencyType is widely used in business logic and this is it's main goal.

What other approach would you suggest? Or maybe my code is not so wrong as I expect?

If you want to decouple Enum and presentation logic, consider using EnumMap . This type of collection is fast and key-safe.

IMO, there is nothing wrong to store short and full currency names in CurrencyType. But localized name (as well as country's flag and other visual aspects) better be decoupled and put into presentation layer. You can use short names as keys for resource bundles.

Furthermore, if all CurrencyType values reproduce currency short names, you can eliminate using dedicated private field for them and use Enum.name() instead.


Convert camelCase and PascalCase to Title Case I'm trying to take camelCase and PascalCase strings into a function and spit them out as Title Case. This function also needs to be able to handle odd PascalCase strings with capitalized abbreviations such as "CDReceiverBox" and return a readable string - "CD Receiver Box".

My current working solution:

function splitCamelCase(camelCaseString) {
    const result = camelCaseString
        .replace(/([A-Z][a-z])/g, " $1")
        .replace(/([A-Z]+)/g, " $1")
        .replace(/ +/g, " ")
        .replace(/^ +/g, "");

    return result.charAt(0).toUpperCase() + result.slice(1);
}

I would like to condense the amount of replace statements I'm using by at least combining the first two replace statements and the last two together since they are semi similar. The more concise I can make this the better.

CodePen: https://codepen.io/andrewgarrison/pen/dEQrMy

I like your solution quite a bit. It's clear, easy to read and I don't see any bugs.

There are many ways to condense the replace calls as you mention, but I think you're at a point where such changes can easily have a disproportionate impact on readability. That's good--it means the code is already pretty optimal from that standpoint.

For example, here's a one-shot replace using alternation, but its merit is debatable:

const splitCamelCase = s => s.replace(
    /^[a-z]|^([A-Z]+)(?=[A-Z]|$)|([A-Z])+(?=[A-Z]|$)|([A-Z])(?=[a-z]+)/g,
    m => " " + m.toUpperCase()
  ).trim()
;

The idea here is to enumerate each scenario, join the patterns with |s, and provide an arrow function to handle the addition of a space and a capital letter for each match.

With the two extremes in mind, I prefer a balanced approach such as:

const splitCamelCase = s =>
  s.replace(/([A-Z][a-z])/g, " $1")
   .replace(/\s*([A-Z]+)/g, " $1")
   .replace(/./, m => m.toUpperCase())
   .trim()
;

or perhaps

const splitCamelCase = s =>
  s.replace(/([A-Z][a-z]|[A-Z]+(?=[A-Z]|$))/g, " $1")
   .replace(/./, m => m.toUpperCase())
   .trim()
;

These should offer ideas as far as how far you want to go in making the succinctness versus readability tradeoff. But, failing the possibility of a shortcut I might have overlooked, keeping your code basically as-is seems like a fine option to me.

If it's performance you're after in reducing replace calls, there's no guarantee that fewer calls will translate into better performance. Under the hood, the regex engine may make more passes to compensate; you can benchmark and tweak using a debugger like regex101. For performance, it's likely best to avoid regex entirely and write a single-pass loop by hand.

Here's a test runner:

const splitCamelCase = s =>
  s.replace(/([A-Z][a-z]|[A-Z]+(?=[A-Z]|$))/g, " $1")
   .replace(/./, m => m.toUpperCase())
   .trim()
;

[
  "AAABbbbbCcDddEEFffGGHhIiJ",
  "AaBbCcDDEeFGgHHHH",
  "CDBoomBoxAAAABbbbCCC",
  "CDBoomBox",
  "camelCase",
  "camel",
  "Camel",
  "c",
  "C",
  "Aa",
  "AA",
  "aa",
  "AAA",
  "aB",
  "aBC",
  "aBCc",
  "",
].forEach(test =>
  console.log(
    splitCamelCaseOriginal(test) === splitCamelCase(test)
      ? `'${test}' -> '${splitCamelCase(test)}'`
      : "TEST FAILED"
  )
);

function splitCamelCaseOriginal(camelCaseString) {
    const result = camelCaseString
        .replace(/([A-Z][a-z])/g, " $1")
        .replace(/([A-Z]+)/g, " $1")
        .replace(/ +/g, " ")
        .replace(/^ +/g, "");

    return result.charAt(0).toUpperCase() + result.slice(1);
}


Calculate the movement required to get from one angle to another angle on a compass The goal of the function is to find out how to get from one angle to another angle. Two things are involved: the amount to move by and the direction (+ means turn right, - means turn left). The most the angle will have to move by is 180deg. The function is rather short. I wanted to avoid using and trig functions or pi. I want this as fast as I can while maintaining readability.

import pandas as pd
import numpy as np

def get_wind_dir_difd(dir_1, dir_2):
    """
    Figures out the shortest way to get from dir_1 to dir_2. Positive nunber go clockwise, negative numbers go counter clockwise.
    NOTE: The direction 0 and 360 are the same.
    :param dir_1: int. The direction of the first wind.
    :param dir_2: The direction of the second wind.
    :return: int. The what to add (can be a negative number too) to the first wind dir to get to the second wind dir.
    """
    diff = dir_2 - dir_1

    if diff > 180:  # the highest diff can be is 180
        diff = 360 - diff

    elif diff < -180:  # the lowest diff can be is -180
        diff = 360 + diff

    if dir_2 > dir_1 and (dir_2 - dir_1 > 180):  # ensures the correct sign is used when dir_2 is larger and the difference betwene the two is more than 180, EX 10, 310 or 40, 360
        diff = -diff

    return diff

# IGNORE WHAT IS BELOW, THAT IS JUST TO TEST

get_wind_dir_dif_vec = np.vectorize(get_wind_dir_difd)

df = pd.read_csv('test.csv')

df['diff'] = get_wind_dir_dif_vec(df['val1'], df['val2'])

for i in range(len(df)):
    row = df.iloc[i]
    correct_or_not = row['diff'] == row['what_diff_should_be']
    if correct_or_not:
        correct_or_not = ''
    else:
        correct_or_not = 'X'
    df.set_value(i, ' bad_answer ', correct_or_not)

print(df)

To give you a better idea of the program

    val1  val2  diff  what_diff_should_be  bad_answer
0    120    30   -90                  -90
1    340    20    40                   40
2     20   340   -40                  -40
3    310    10    60                   60
4     10   310   -60                  -60
5      0   300   -60                  -60
6    300     0    60                   60
7    190   180   -10                  -10
8    180   190    10                   10
9    200   220    20                   20
10   220   200   -20                  -20
11    10   190   180                  180
12   360     0     0                    0
13     0   360     0                    0
14   340    20    40                   40
15   350     0    10                   10
16   350   360    10                   10
17    40   360   -40                  -40
18   180     0  -180                 -180
19     0   180   180                  180

This is the csv I tested it on.

val1,val2,diff,what_diff_should_be
120,30,,-90
340,20,,40
20,340,,-40
310,10,,60
10,310,,-60
0,300,,-60
300,0,,60
190,180,,-10
180,190,,10
200,220,,20
220,200,,-20
10,190,,180
360,0,,0
0,360,,0
340,20,,40
350,0,,10
350,360,,10
40,360,,-40
180,0,,-180
0,180,,180

The other reviews give some good formatting advice so I'll concentrate solely on the function itself.  I would probably write this instead:

def dir_diff(dir1, dir2):
    diff = (dir2 - dir1) % 360
    return diff if diff <= 180 else diff - 360

The way Python's % operator works, it assures that we always have a positive number from 0 to 360 for diff in the first line.  Then we only need to adjust if the value is greater than 180.  Note also that unlike all of the other versions (including the original), this version will also continue to work properly even if we pass in values such as 900, -92 or 725, 9900.

Better yet, use AJNeufeld's comment or twalberg's equivalent:

return 180 - (dir1 - dir2 + 180) % 360

In another comment, the OP said:

  Later I will use this function to interpolate missing data. So, if dir_1 is lower than dir_2 like it is with (0, 180) then the answer should be positive because 90 (90 b/c +180/2) would be the middle value and not -90. Hope that helps.

This requirement doesn't make sense to me because interpolation between two diametrically opposite points on a circle can legitimately be interpreted as either midpoint.  However, if it's really a requirement, we can combine like so:

def Edward2(dir1, dir2):
    diff = (dir2 - dir1 + 180) % 360 - 180
    return -diff if diff == -180 and dir1 < dir2 else diff

If anyone cares, here is the test code I used:

import csv

def orig(dir_1, dir_2):
    diff = dir_2 - dir_1

    if diff > 180:
        diff = 360 - diff

    elif diff < -180:
        diff = 360 + diff

    if dir_2 > dir_1 and (dir_2 - dir_1 > 180):
        diff = -diff

    return diff

def Edward1(dir1, dir2):
    diff = (dir2 - dir1) % 360
    return diff if diff <= 180 else diff - 360

def twalberg(dir1, dir2):
    return 180 - (dir1 - dir2 + 180) % 360

def AJneufeld(dir1, dir2):
    return (dir2 - dir1 + 180) % 360 - 180

def Edward2(dir1, dir2):
    diff = (dir2 - dir1 + 180) % 360 - 180
    return -diff if diff == -180 and dir1 < dir2 else diff

if __name__=="__main__":
    green = "\x1b[32m"
    red = "\x1b[91m"
    white = "\x1b[37m"
    algo = [orig, Edward1, twalberg, AJneufeld, Edward2]

    def testAll(d1, d2, desired):
        print('{:-6} {:-6} {:-6}'.format(d1, d2, desired), end='')
        for fn in algo:
            calc = fn(int(d1),int(d2))
            color = [green, red][calc != desired]
            print('{}{:>10}{}'.format(color, calc, white), end='')
        print()

    print('  dir1   dir2 desired', end='')
    for fn in algo:
        print(f'{fn.__name__:>10}', end='')
    print()
    with open('input.csv') as csvfile:
        rowreader = csv.reader(csvfile)
        for row in rowreader:
            testAll(int(row[0]), int(row[1]), int(row[2]))

Here is the test data I used, with the format dir1,dir2,desired.

input.csv

120,30,-90
340,20,40
20,340,-40
310,10,60
10,310,-60
0,300,-60
300,0,60
190,180,-10
180,190,10
200,220,20
220,200,-20
10,190,180
360,0,0
0,360,0
340,20,40
350,0,10
350,360,10
40,360,-40
180,0,-180
0,180,180
1,181,180
359,179,-180
180,0,-180
181,1,-180
179,359,180
90,270,180
270,90,-180
-90,-270,-180
-270,-90,180
0,360,0
360,0,0
180,-180,0
-180,-180,0
-180,180,0
180,180,0
900,-92,88
725,9900,175
725,9905,180
9905,725,-180

Results

With the test cases shown, only the Edward2 function gets all results correct. I'm using Python 3.7.5.


Zig-zag array - solution The problem

  For any given array, find a corresponding zig-zag array such the each
  element (a,b,c,d,e...) follows the rule:

a < b > c < d > e < f

In other words, every other element is greater than both of its neighbouring elements and the remaining elements are smaller than their neighbours.

For example, for the list:

[4, 3, 7, 8, 6, 2, 1]

the output is:

[1 8 2 7 3 6 4]

My solution

array = [4, 3, 7, 8, 6, 2, 1]
array.sort()

while len(array) > 1:
    print(array[0], array[-1], sep = " ", end= " ")
    array.pop(0)
    if array: array.pop(-1)
print(array[0])

You can just use Python's slice notation. It allows you to pass steps, which allows you to do this with ease.

I also think it's fairly easy to understand.

def zigzag(values):
    half = len(values) // 2
    new = [None] * len(values)

    new[0::2] = values[:-half]
    new[1::2] = values[-half:][::-1]
    return new

>>> zigzag([1, 2, 3, 4])
[1, 4, 2, 3]
>>> zigzag([1, 2, 3, 4, 5])
[1, 5, 2, 4, 3]


Make a version of make-rat that handles positive and negative arguments Given the following task from SICP

  Exercise 2.1

  Define a better version
  of make-rat that handles both positive
  and negative arguments. Make-rat
  should normalize the sign so that if
  the rational number is positive, both
  the numerator and denominator are
  positive, and if the rational number
  is negative, only the numerator is
  negative.

And these prerequisite functions:

(define (gcd a b) (if (= b 0) a (gcd b (remainder a b))))

(define (numer a) (car a))
(define (denom a) (cdr a))
(define (print-rat a)
  (newline)
  (display (numer a))
  (display "/")
  (display (denom a)))

(define (add-rat a b)
  (make-rat (+ (* (numer a) (denom b))
               (* (numer b) (denom a)))
            (* (denom a) (denom b))))
(define (sub-rat a b)
  (make-rat (- (* (numer a) (denom b))
               (* (numer b) (denom a)))

            (* (denom a) (denom b))))
(define (mul-rat a b)
  (make-rat (* (numer a) (numer b))
            (* (denom a) (denom b))))
(define (div-rat a b)
  (make-rat (* (numer a) (denom b))
            (* (numer b) (denom a))))

I wrote this version of (make-rat ... ).  What do you think?

(define (make-rat n d)
  (define (sign n d) (if (> (* n d) 0) + -))
  (let* ((abs-n (abs n))
         (abs-d (abs d))
         (div (gcd abs-n abs-d)))
    (cons ((sign n d) (/ abs-n div))
          (/ abs-d div))))

To normalize a fraction, one needs to do two things:

fix the signs of numerator and denominator; and
reduce them to their lowest terms.

Your implementation does these two things just fine.  However, there is an easier way.  To do (1), simply negate both numerator and denominator if the denominator is negative.  To do (2), divide by the greatest common divisor.

One could combine the two steps above.  Divide the numerator and denominator by the greatest common divisor with its sign fixed to be the same as the denominator's sign.  Implementation follows:

(define (make-rat n d)
  (let
      ((div ((if (< d 0) - +) (abs (gcd n d)))))
    (cons (/ n div) (/ d div))))

If gcd always returns a positive number (which your implementation does not), you may remove the call to abs above.


Most frequent word in an array of strings - Java Input word array is
{ "geeks", "for", "geeks", "a", "portal", "to", "learn", "can", "be", "computer","science", "zoom", "yup", "fire", "in", "be", "data", "a", "portal","geeks" };

Problem is to find most frequent word in array.
I want to find solution that can support on adding more element.

I have tried this code :-

package com.techiekunal.examples.datastructure;

import java.util.Iterator;
import java.util.PriorityQueue;

/**
 * MyWord objects will have word and its frequency
 *
 * @author Kunal.Saxena
 *
 */
class MyWord implements Comparable<MyWord>{

    // word from array
    private String word;

    // word's frequency
    private int count;

    public MyWord(int count, String word) {
        this.count = count;
        this.word = word;
    }

    public String getWord() {
        return word;
    }

    public void setWord(String word) {
        this.word = word;
    }

    public int getCount() {
        return count;
    }

    public void setCount(int count) {
        this.count = count;
    }

    // Objects are equals if words are same
    @Override
    public boolean equals(Object obj) {
        if(obj == null)
            return false;
        if(this.getClass() != obj.getClass())
            return false;

        MyWord myWord = (MyWord) obj;
        return this.word.equals(myWord.word);
    }

    @Override
    public int hashCode() {
        return super.hashCode();
    }

    // Printing word and count against it
    @Override
    public String toString() {
        return this.word + " : " + this.count;
    }

    // comparison for descending order of count
    @Override
    public int compareTo(MyWord o) {
        if(this.count > o.count) {
            return -1;
        }
        if(this.count < o.count) {
            return 1;
        }
        return 0;
    }

}

public class MostFrequentUsedWords {

    // Input array
    private static String[] arr = { "geeks", "for", "geeks", "a", "portal", "to", "learn", "can", "be", "computer",
            "science", "zoom", "yup", "fire", "in", "be", "data", "a", "portal","geeks" };

    // Queue will work as max heap to store words
    private static PriorityQueue<MyWord> queue = new PriorityQueue<>();

    // Creating PriorityQueue from given input array
    private static void createQueue() {

        for (String word : arr) {
            // check if word already exists
            if (queue.contains(new MyWord(1, word))) {

                MyWord oldWord = null;
                // iterate to find word : we need latest frequency of that word
                Iterator<MyWord> itr = queue.iterator();
                while (itr.hasNext()) {

                    MyWord next = itr.next();
                    if (next.getWord().equals(word)) {
                        oldWord = next;
                    }
                }
                // create new word by incrementing frequency, remove old word from queue, adding new word to queue
                MyWord newWord = new MyWord(oldWord.getCount() + 1, oldWord.getWord());
                queue.remove(oldWord);
                queue.add(newWord);

            } else {
                // if word is not in queue : add it with frequency 1
                MyWord newWord = new MyWord(1, word);
                queue.add(newWord);
            }
        }

    }

    public static void main(String[] args) {

        // Create priority queue
        createQueue();

        // Print Queue
        while (!queue.isEmpty()) {
            System.out.println(queue.poll());
        }
    }

}

My output is :-

geeks : 3
a : 2
portal : 2
be : 2
yup : 1
zoom : 1
science : 1
computer : 1
in : 1
can : 1
learn : 1
to : 1
data : 1
fire : 1
for : 1

I implemented this using priority queue and it is working fine. it is like creating max heap. But its complexity is n^2. Need help if this can be solve in less complex way.

As you noticed, searching for your entries is adding to your time complexity, because you have to run down your queue. Priority queues are very good at polling the top element, but not optimized for searching for specific entries.

You can optimize for searching by using a Map<String, Integer> for counting frequency, and then use a NavigableMap<Integer, Set<String>> for maintaining order (1). Adding an entry will look like this:

NavigableMap<Integer, Set<String>> ordered;
Map<String, Integer> frequency;

void add(String word) {
  int count = frequency.getOrDefault(word, 0);
  if ( count > 0 ) {
    ordered.get(count).remove(word);
  }
  ordered.computeIfAbsent(count + 1, HashSet::new).add(word);
  frequency.put(word, count + 1);
}

(1) If you're confident about your data being densely distributed and bounded, you can replace this with an array for quicker access—approaching O(1) i.o. O(log N)—but it's going to make your code messier dealing with edge cases.

I do have two recommendations for your code.

When searching for an element in the queue, you can abort once it's found. On average, this will halve your time spent searching:

Iterator<MyWord> itr = queue.iterator();
while (itr.hasNext()) {
   MyWord next = itr.next();
   if (next.getWord().equals(word)) {
       oldWord = next;
       break;  // <-- found, abort search
   }
}

And a nitpick from an 'elegance' point of view: consider adding a method to MyWord that creates a new, incremented instance. Right now, outside code is accessing the fields and creating new instances with it. This is something MyWord can handle internally.

public MyWord incremented() {
    return new MyWord(count + 1, word);
}


LeetCode 535: Encode and Decode TinyURL I'm posting a solution for LeetCode's "Encode and Decode TinyURL". If you'd like to review, please do. Thank you!
Problem
TinyURL is a URL shortening service where you enter a URL such as https://leetcode.com/problems/design-tinyurl and it returns a short URL such as http://tinyurl.com/4e9iAk.
Design the encode and decode methods for the TinyURL service. There is no restriction on how your encode/decode algorithm should work. You just need to ensure that a URL can be encoded to a tiny URL and the tiny URL can be decoded to the original URL.
Code

// The following block might slightly improve the execution time;
// Can be removed;
static const auto __optimize__ = []() {
    std::ios::sync_with_stdio(false);
    std::cin.tie(nullptr);
    std::cout.tie(nullptr);
    return 0;
}();

// Most of headers are already included;
// Can be removed;
#include <iostream>
#include <cstdint>
#include <string>
#include <unordered_map>
#include <utility>
#include <random>

static const struct Solution {
    public:
        const std::string encode(
            const std::string long_url
        ) {
            std::string tiny_encoded;

            if (!encoded_url.count(long_url)) {
                for (auto index = 0; index < kTinySize; ++index) {
                    tiny_encoded.push_back(char_pool[rand_generator() % std::size(char_pool)]);
                }

                encoded_url.insert(std::pair<std::string, std::string>(long_url, tiny_encoded));
                decoded_url.insert(std::pair<std::string, std::string>(tiny_encoded, long_url));

            } else {
                tiny_encoded = encoded_url[long_url];
            }

            return kDomain + tiny_encoded;
        }

        const std::string decode(
            const std::string short_url
        ) {

            return std::size(short_url) != kDomainTinySize ||
                   !decoded_url.count(short_url.substr(kDomainSize, kTinySize)) ? "" :
                   decoded_url[short_url.substr(kDomainSize, kTinySize)];
        }

    private:
        static constexpr char kDomain[] = "http://tinyurl.com/";
        static constexpr unsigned int kTinySize = 6;
        static constexpr unsigned int kDomainSize = std::size(kDomain) - 1;
        static constexpr auto kDomainTinySize = kDomainSize + kTinySize;
        static constexpr char char_pool[] = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789";
        std::unordered_map<std::string, std::string> encoded_url;
        std::unordered_map<std::string, std::string> decoded_url;
        std::random_device rand_generator;
};

// Your Solution object will be instantiated and called as such:
// Solution solution;
// solution.decode(solution.encode(url));

You are doing the lookup twice.
            if (!encoded_url.count(long_url)) {

                .. stuff

            } else {
                tiny_encoded = encoded_url[long_url];
            }

I know that it is O(1) for the lookup. But there is a real constant inside that. Avoid it if you can.
Use find(). Then if it is there you can simply use it.
            auto find = encoded_url.find(long_url);
            if (find == encoded_url.end()) {

                .. stuff

            } else {
                tiny_encoded = find->second;
            }

This is great if you want random URL that are hard to guess.
                for (auto index = 0; index < kTinySize; ++index) {
                    tiny_encoded.push_back(char_pool[rand_generator() % std::size(char_pool)]);
                }

But is that a requirement of the puzzle. Seems (not sure how expensive the generating the random number is) like this is very expensive way of generating a name.
There is also a chance for a clash. If you are using randomly generated values append a timestamp on the end to avoid a clash.

Personally I don't like having to specify a type. But if you are goint to do it use the type of the method rather than being this specific:
    encoded_url.insert(std::pair<std::string, std::string>(long_url, tiny_encoded));

    // Top of the class.
    using Map      = std::unordered_map<std::string, std::string>;
    using MapValue = Map::value_type;

    // In the code.
    encoded_url.insert(MapValue(long_url, tiny_encoded));

But I think I would simply have used emplace().
    encoded_url.emplace(long_url, tiny_encoded);


Macros to detect UTF-8 I'm working on a program that handles UTF-8 characters.  I've made the following macros to detect UTF-8. I've tested them with a few thousand words and they seem to work.

I'll add another one to do error-checking later, but for now I would like to know what mistakes I've made and how these macros can be improved.

//check if value is in range of leading byte
#define IS_UTF8_LEADING_BYTE(b) (((unsigned char)(b) >= 192) && (unsigned char)(b) < 248)

//check if value is in range of sequence byte
#define IS_UTF8_SEQUENCE_BYTE(b) ((unsigned char)(b) >= 128 && (unsigned char)(b) < 192)

//can be any utf8 byte, first, last...
#define IS_UTF8_BYTE(b) (IS_UTF8_LEADING_BYTE(b) || IS_UTF8_SEQUENCE_BYTE(b))

//no error checking, it must be used only on leading byte
#define HOW_MANY_UTF8_SEQUENCE_BYTES(b) ((((unsigned char)(b) & 64) == 64) + (((unsigned char)(b) & 32) == 32) + (((unsigned char)(b) & 16) == 16))

There are a lot of magic numbers.  They are numbers that would be familiar to any good programmer, but magic numbers nonetheless.  Furthermore, it's more useful to think of the problem using bitwise operations, since they are more closely related to the conceptual design behind UTF-8.

You only accept two-, three-, and four-byte sequences, i.e., only code points U+0080 to U+1FFFFF, i.e., the Basic Multilingual Plane (minus the ASCII set) plus the Supplementary Multilingual Plane.  That should be made clear in comments.

I suggest renaming IS_UTF8_BYTE() to IS_UTF8_MULTIBYTE(), since ASCII characters ≤ 127 are valid UTF-8 bytes too — just not members of a multibyte sequence.

//check if value is in range of leading byte (0b11?????? but not 0b11111???)
#define IS_UTF8_LEADING_BYTE(b) (((b) & 0xc0) && !((b) & 0xf8))

//check if value is in range of sequence byte (0b10??????)
#define IS_UTF8_SEQUENCE_BYTE(b) (((b) & 0x80) && !((b) & 0xc0))

//can be any byte within a UTF-8 multibyte sequence
#define IS_UTF8_MULTIBYTE(b) (IS_UTF8_LEADING_BYTE(b) || IS_UTF8_SEQUENCE_BYTE(b))

//no error checking; it must be used only on leading byte
#define HOW_MANY_UTF8_SEQUENCE_BYTES(b) (((b) & 0x40 == 0x40) + ((b) & 0x20 == 0x20) + ((b) & 0x10 == 0x10))

IS_UTF8_SEQUENCE_BYTE() could be more clever.

//check if value is in range of sequence byte (0b10??????)
#define IS_UTF8_SEQUENCE_BYTE(b) ((b) & 0xc0 == 0x80)


Both an Iterative and a Recursive f(x) A function f is defined
  by the rule that f(n) = n if n < 3 and
  f(n) = f(n-1) + 2f(n-2) + 3f(n-3) if
  n>=3.  Write a recursive and an
  iterative process for computing f(n).

I wrote the following:

(define (f_r n)
  ; recursive
  (cond ((< n 3) n)
        (else (+ (f_r (- n 1))
                 (* 2 (f_r (- n 2)))
                 (* 3 (f_r (- n 3)))))))

(define (f_i n)
  ;iterative
  (f_i-prime 1 0 0 n))

(define (f_i-prime n n-1 n-2 count)
  (cond ((= count 0) n)
        ((f_i-prime (+ n n-1 n-2)
                    (+ n n-1)
                    n-1
                    (- count 1)))))

What do you think?

EDIT 1: Since my first iterative solution was erroneous, here is a corrected version:

(define (f_i n)
  ;iterative
  (f_i-prime 2 1 0 n))

(define (f_i-prime f_x+2 f_x+1 f_x n-x)
  (cond ((= n-x 0) f_x)
        ((f_i-prime (+ f_x+2 (* 2 f_x+1) (* 3 f_x) )
                    f_x+2
                    f_x+1
                    (- n-x 1)))))

Your iterative definition will not produce correct results for most values of n.

When rewriting a pure recursive function as an iterative one, one needs to keep as many accumulators as there are base cases.  In the iterative step, compute the next value, store it in the first accumulator, rotate the values of all remaining accumulators and decrement the loop variable.

Here's an implementation in your style:

(define (f_i n)
  ;iterative
  ;initial values of accumulators are f(2) = 2, f(1) = 1 and f(0) = 0; loop variable is n.
  (f_i-prime 2 1 0 n))

(define (f_i-prime f-x+2 f-x+1 f-x n-x)
  (cond ((= n-x 0) f-x)   ; if n-x = 0, then n = x, so return f(x) = f(n).
        ((f_i-prime (+ f-x+2 (* 2 f-x+1) (* 3 f-x))   ; iterative step -- compute next value of f(x + 2);
                    f-x+2                             ; ... rotate all other accumulators;
                    f-x+1
                    (- n-x 1)))))                     ; ... and decrement loop variable.

One may incorporate the helper function within the main function using letrec as follows (this version differs slightly from the previous one in that the loop variable counts up from 0 to n):

(define (f-iterative n)
  (letrec
      ((f-aux
        (lambda (x f-x+2 f-x+1 f-x)
          (cond
            ((= x n) f-x)
            ((f-aux (+ x 1) (+ f-x+2 (* 2 f-x+1) (* 3 f-x)) f-x+2 f-x+1))))))
    (f-aux 0 2 1 0)))

One may also use the do expression to write the iterative version as follows:

(define (f-iterative n)
  (do
      ((x 0 (+ x 1))
       (f-x+2 2 (+ f-x+2 (* 2 f-x+1) (* 3 f-x)))
       (f-x+1 1 f-x+2)
       (f-x 0 f-x+1))
    ((= x n) f-x)))

As to your recursive definition, you may wish to memoize the results in order to decrease your algorithm's complexity, which is currently in O(3 ^ n).  Memoization will improve that to linear complexity.


Python - Do I need a class or are these functions okay? I'm fairly new to Python but had to start using it and for reasons I have to use 2.7, which is why I am using that syntax. I have this function which gets data from a file, if it exists, and then gets passed onto other functions to perform SQL queries etc. The script is only intended to work for the directory it is running in, hence the os.getcwd()
This static data is the only data that gets looked up then returned and referenced by other functions. It works as it is but would you say for professionalism and understanding python better that a class would be better here or a restructure in someway? I am of course only posting a snippet.
    currentdirectory = os.getcwd()
    versionf = "version.ini"
    verfile = os.path.join(currentdirectory, versionf)

    def main():
       get_local_details()
       push_app(application, version)
       do_something(app_name_version)

    def get_local_details():
        if os.path.isfile(verfile):
            def get_app_name():
                try:
                    global application
                    application = subprocess.check_output(['grep', 'application', verfile]).strip('\n')
                    application = application.rsplit('=')[1]
                    return application
                except:
                    print "Application missing from version.ini"
             def version_number():
                try:
                    global version
                    version = subprocess.check_output(['grep', 'version', verfile]).strip('\n')
                    version = version.rsplit('=')[1]
                    return version
                except:
                    print "version missing from version.ini"
            get_app_name()
            version_number()
            def app_name_version(application, version)
                try:
                    global applicationver
                    applicationver = application + "." + version
                    return applicationver
                except:
                    print "error here"
            app_name_version(application, version)
        else:
            print "File does not exist."

main()

contents of ini file are
application=testapp
version=1.0

do_something is a function that contains a SQL query that uses the app_name_version as the WHERE statement.
Is it even possible to use a function within a class to define the attributes?

Wow, this is kind of a mess.
I'll just review items in sequence as I read them.
My short answer, tl;dr: Yes!
Try executing python 2.7 code with a python 3 interpreter,
even if only during development on your laptop, or in unit tests.
It will give you the confidence to import six or otherwise drag
ancient code kicking and screaming into the modern era.
Small changes like print x → print(x) are easy and helpful.
The first three lines are nice.
We define currentdirectory, versionf, & verfile as top-level module constants.
PEP-8 prefers the name current_directory, but I quibble, it's fine as-is.
You conditionally def or print.
I recommend an early abort:
            if not os.path.isfile(verfile):
                raise FileNotFoundError(verfile)

You have unfortunate coupling in this code.
Python, like all O-O languages, offers you structuring tools to improve cohesion.
Please avoid the global keyword, it is a bad code smell.
And please avoid nested def, as its effect on namespaces is similar to using global variables.
For private helpers, choose _private names like def _get_app_name,
def _version_number, and def _app_name_version.
You wrote:

application = subprocess.check_output(['grep', 'application', verfile]) ...

In several places you reference verfile.
Just listen to your code.
Clearly it is telling you it wants an __init__ constructor
which assigns to self.verfile, so that it may be used here.
Also, fork'ing grep is a tool one might use,
but it's not the most appropriate one for the task at hand.
Let me introduce you to import configparser.

                global application
                ...
                return application

Ok, you used to have 3 module-level variables,
and now you've added a 4th one.
Sadly, you have scattered them all around the module,
and you will continue to define additional ones.
This is poorly structured.
There's no central place for a maintenance engineer
to discover "what are the important pieces of state I need to worry about?"
Also, not sure why, after side effecting the variable,
you bothered to return it, since caller ignores the return value.
Rather than no-arg invocations of get_app_name and version_number,
you might consider passing in the values they need as arguments.
But if you turn this into a class, then self can supply those values.

            except:
                print ...

Better for your catch to be except Exception,
lest you accidentally catch CTRL-C exceptions, as well.
Consider using a logger.
Then we continue to see a pattern of "more of the same",
I won't belabor it with repetition.
You are consistently abusing namespaces,
relying on implicit args,
and side effecting globals at module-level.
It's not quite clear from the generic code you posted,
but I'm going to go out on a limb and say that this module
seems to be about "app verification".

Please do this:
class AppVerifier:

and then define half a dozen object attributes as I suggested above.
Doing just that one thing will have a huge effect on making your
code more maintainable for the next engineer that has to interact with it.


Updating a dict with dict values This code works and does exactly what I want it to, but I feel that it is inelegant.  It seems like there should be a way to use get() and/or setdefault(), for example.  All comments and suggestions are appreciated!

Here is a function that builds a dict.  The dict's values are also dicts.  Some of the values are empty dicts {} and some are dicts that have a {key:value} pair.

def my_function(my_string, my_key):

    d = {}

    for i in range(3):
        d['test' + str(i)] = {}

    d['test3'] = {'a_key': 'a_value'}

    # d = { 'test0': {}, 'test1': {}, 'test2': {}, 'test3': {'a_key': 'a_value'} }

    if my_string != '':
        d[my_key] = {my_string: None}

    else:
        # check if d[my_key] exists
        try:
            # d[my_key] exists and it is not empty, don't overwrite
            if d[my_key]:
                pass
            # d[my_key] exists, but it is empty, write NULL
            else:
                my_string = 'NULL'
                d[my_key] = {my_string: None}

        # d[my_key] did not exist, create it and write NULL
        except KeyError:
            my_string = 'NULL'
            d[my_key] = {my_string: None}

    return d

# my_string not blank, my_key is new
f = my_function('hello', 'test4')
print(f)

# my_string not blank, my_key exists
f = my_function('hello', 'test0')
print(f)

# my_string blank, my_key exists and its value is not empty
f = my_function('', 'test3')
print(f)

# my_string blank, my_key exists and its value is empty
f = my_function('', 'test0')
print(f)

# my_string blank, my_key is new
f = my_function('', 'test4')
print(f)

Typical (and desired) output:

{'test2': {}, 'test3': {'a_key': 'a_value'}, 'test0': {}, 'test1': {}, 'test4': {'hello': None}}
{'test2': {}, 'test3': {'a_key': 'a_value'}, 'test0': {'hello': None}, 'test1': {}}
{'test2': {}, 'test3': {'a_key': 'a_value'}, 'test0': {}, 'test1': {}}
{'test2': {}, 'test3': {'a_key': 'a_value'}, 'test0': {'NULL': None}, 'test1': {}}
{'test2': {}, 'test3': {'a_key': 'a_value'}, 'test0': {}, 'test1': {}, 'test4': {'NULL': None}}

Tests

It's a good thing you've provided tests and expected output so that one can check the behavior, it is even better if one can run the tests without having to check that the output is correct. Only a tiny bit of reorganisation is required to get :

# my_string not blank, my_key is new
assert my_function('hello', 'test4') == {'test2': {}, 'test3': {'a_key': 'a_value'}, 'test0': {}, 'test1': {}, 'test4': {'hello': None}}

# my_string not blank, my_key exists
assert my_function('hello', 'test0') == {'test2': {}, 'test3': {'a_key': 'a_value'}, 'test0': {'hello': None}, 'test1': {}}

# my_string blank, my_key exists and its value is not empty
assert my_function('', 'test3') == {'test2': {}, 'test3': {'a_key': 'a_value'}, 'test0': {}, 'test1': {}}

# my_string blank, my_key exists and its value is empty
assert my_function('', 'test0') == {'test2': {}, 'test3': {'a_key': 'a_value'}, 'test0': {'NULL': None}, 'test1': {}}

# my_string blank, my_key is new
assert my_function('', 'test4') == {'test2': {}, 'test3': {'a_key': 'a_value'}, 'test0': {}, 'test1': {}, 'test4': {'NULL': None}}

# New test from Sjoerd Job Postmus's comment
assert my_function('something not empty', 'test3') == {'test1': {}, 'test0': {}, 'test3': {'something not empty': None}, 'test2': {}}

Improvements

if my_string != '': could be rewritten more concisely : if my_string:.

Also, you could use get to retrievement elements in a "safe" way.

Thus, the code becomes :

if my_string:
    d[my_key] = {my_string: None}
else:
    if not d.get(my_key):
        d[my_key] = {'NULL': None}
return d

Also, your dict initialisation could be done with a dictionnary comprehension. Then, your whole function would become:

def my_function(my_string, my_key):
    d = {'test' + str(i): {} for i in range(3)}
    d['test3'] = {'a_key': 'a_value'}

    if my_string:
        d[my_key] = {my_string: None}
    elif not d.get(my_key):
        d[my_key] = {'NULL': None}
    return d

Is this as simple as it can be ? Not quite ? The check for d.get(my_key) is actually to distinguish "test3" from the other values. It could be easier to just do the obvious thing instead of trying to retrieve stuff from the dict :

def my_function(my_string, my_key):
    d = {'test' + str(i): {} for i in range(3)}
    d['test3'] = {'a_key': 'a_value'}
    if my_string:
        d[my_key] = {my_string: None}
    elif my_key != 'test3':
        d[my_key] = {'NULL': None}
    return d


should DRY apply to unit tests as much as production code I have a unit test method that looks like this:

...
113 CPPUNIT_ASSERT( !m_pInputFilter[0]->SendPacket() );
114
115 CppUnitUtil::CUnitTestTimeline::GetInstance().Advance( 1, CppUnitUtil::Seconds );
116 CPPUNIT_ASSERT_NO_THROW( m_pTimer->Fire() );
117
118 CPPUNIT_ASSERT_EQUALS( 1, m_pOutputFilter[0]->RecievedPackets() );
119 CPPUNIT_ASSERT_EQUALS( 2, m_pOutputFilter[1]->RecievedPackets() );
120
121 CPPUNIT_ASSERT( !m_pInputFilter[1]->SendPacket() );
122
123 CppUnitUtil::CUnitTestTimeline::GetInstance().Advance( 1, CppUnitUtil::Seconds );
124 CPPUNIT_ASSERT_NO_THROW( m_pMockTimer->Fire() );
125
126 CPPUNIT_ASSERT_EQUALS( 2, m_pOutputFilter[0]->RecievedPackets() );
127 CPPUNIT_ASSERT_EQUALS( 2, m_pOutputFilter[1]->RecievedPackets() );
...

This same chunk of code (lines 113-119) is repeated several more times in this function.

If this was production code, I would've done extract method refactoring in a heartbeat on lines 113-119. But because this is part of a unit test project, I'm having second thoughts and here's why.  Supposed I create a function:

SendNextPacket( int sendFilterIndex, int expectedOutput0, int expectedOutput1 ) {
    CPPUNIT_ASSERT( !m_pInputFilter[sendFilterIndex]->SendPacket() );

    CppUnitUtil::CUnitTestTimeline::GetInstance().Advance( 1, CppUnitUtil::Seconds );
    CPPUNIT_ASSERT_NO_THROW( m_pTimer->Fire() );

    CPPUNIT_ASSERT_EQUALS( expectedOutput0, m_pOutputFilter[0]->RecievedPackets() );
    CPPUNIT_ASSERT_EQUALS( expectedOutput1, m_pOutputFilter[1]->RecievedPackets() );
}

... and my main test method becomes...

...

SendNextPacket( 0, 1, 2 );

SendNextPacket( 1, 2, 2 );

...

The problem becomes that if this unit test fails on line 119 or 127 (from original code segment), my refactored code would not actually tell me which line failed. Instead, in both cases, it will report the same line number which is now in the helper function.

So in original code, yes I have duplication which is bad, but I also have an exact point of failure, which is good.

I've caught myself several times now kicking myself because I would eliminate all duplication, but then end up having to use debugger just to see exactly which one of the calls to the refactored methods is failing.  That seems to be the opposite of what you want from a unit test.

How would you guys approach this type of scenario?

As Cygal pointed out, keeping things DRY is a very good thing and I clearly agree with that. At the same time I did not want to just simply blame the tool for its limitations or to have to use a debugger when one really shouldn't have to be used.

So here's an alternative.

We have our own CppUnit library on top of CppUnit where we defined a whole bunch of helpers not available in the framework (things like CPPUNIT_ASSERT_IN_RANGE( min, max, value ) macro).
So I added this macro to our library:

#define CPPUNIT_CALL( expression )                                  \
    do {                                                            \
        try {                                                       \
            expression;                                             \
        } catch( ::CppUnit::Exception& e ) {                        \
            _RethrowCppUnitAssert( e, CPPUNIT_SOURCELINE() );       \
        }                                                           \
    } while ( false )

and the rethrow function is defined as:

inline void _RethrowCppUnitAssert( CPPUNIT_NS::Exception&           e,
                                   const CPPUNIT_NS::SourceLine&    outerLocation ) {
    CPPUNIT_NS::Message     msg( e.message() );
    char                    location[4096];
    sprintf_s( location, "From: %s(%d)",
               outerLocation.fileName().c_str(), outerLocation.lineNumber() );
    msg.addDetail( location );
    CPPUNIT_NS::Asserter::fail( msg, e.sourceLine() );
}

So now the actual unit test code looks like this:

...

CPPUNIT_CALL( SendNextPacket( 0, 1, 2 ) );

CPPUNIT_CALL( SendNextPacket( 1, 2, 2 ) );

...

Works like a charm and everything is DRY as it should be.  If anything inside those calls throws an assertion, that assertion will be intercepted, source file/line number of the CPPUNIT_CALL will be added to message details, and the whole thing will be rethrown.

(*) - p.s. I'm not sure what's up with do {... } while( false ) loop in there. I modeled this macro after CPPUNIT_ASSERT_NO_THROW_MESSAGE and that was in there, so I figured somebody at some point must have had a good reason.


Contact search application Create a C++ program which will act like contact search app in mobile.
  For example, if the following contacts are saved in contact list:

    Massi
    Pradip
    Prasad
    Praveed
    Raju

  And if user type "a" then following names should be displayed:

    Massi
    Pradip
    Prasad
    Praveed
    Raju

  if user type "Pr" then following names should be displayed:

    Pradip
    Prasad
    Praveed

  Basically it should display all the names which contain the search
  string anywhere in name.

#include <iostream>
#include <stdio.h>
#include <conio.h>
#include <string>
#include <map>
#include<iomanip>

using namespace std;

int main() {
    map<string,int> contacts;
    contacts["Pradip"]=999092456;
    contacts["Raju"]=999092457;
    contacts["Praveed"]=999092445;
    contacts["Prasad"]=999092415;
    contacts["Asim"]=999092785;
    contacts["Massi"]=999092215;
    int key_code;
    string key;
    while (true)
    {

            if ( kbhit() ){

                key_code = getch();
                system("CLS");

                if (key_code == 8 ){
                    key.size() ? key.pop_back(): 0;
                }
                else{
                    key+=key_code;
                }
                cout<<"Contacts with : "<<key<<endl;

                for (map<string,int>::iterator &tter = contacts.begin();tter != contacts.end();tter++)
                {
                    if (string::npos == tter->first.find(key) && !key.empty()){
                        continue;
                    }
                    std::cout<<setw(4)<<"Name : "<<setw(10)<< std::left <<tter->first.c_str()<<setw(14)<<", Number : "<<setw(10)<<tter->second<<std::endl;
                }

            }
            else
                continue;
    }
    return 0;
}

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

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.

Don't use system("cls")

There are two reasons not to use system("cls") or system("pause").  The first is that it is not portable to other operating systems which you may or may not care about now.  The second is that it's a security hole, which you absolutely must care about.  Specifically, if some program is defined and named cls or pause, your program will execute that program instead of what you intend, and that other program could be anything.  First, isolate these into a separate functions cls() and pause() and then modify your code to call those functions instead of system.  Then rewrite the contents of those functions to do what you want using C++.  For example, if your terminal supports ANSI Escape sequences, you could use this:

void cls()
{
    std::cout << "\x1b[2J";
}

Isolate platform-specific code

In this code, there are several things that are DOS/Windows only including #include <conio.h> and the getch() and kbhit() functions within that, and also system("cls"); that I've already mentioned.  Your code runs successfully on Linux if I supply those missing functions, but it would be nice if there were an #ifdef WINDOWS already in the code so that one could recompile without having to alter the source code.

Eliminate "magic numbers"

There are a few numbers in the code, such as 8 and 14 that have a specific meaning in their particular context.  By using named constants such as BACKSPACE or COLUMN_WIDTH, the program becomes easier to read and maintain.

Don't abuse the ternary operator

The code currently includes this inscrutable bit of code:

key.size() ? key.pop_back(): 0;

This is not valid C++ even if your compiler accepts it.  The problem is that the two options must both evaluate to the same type.  std::string.pop_back() returns void but 0 is an int.  The fix is to simply use an if instead which makes the code both valid and readable C++:

if (key.size()) {
    key.pop_back();
}

Be careful with references

The for loop starts with this:

map<string,int>::iterator &tter = contacts.begin();

That's not right because tter should be an iterator, not a reference to an iterator.  It should instead be written as:

map<string,int>::iterator tter = contacts.begin();

Or even better, you could use the next suggestion if your compiler is compliant.

Use "range-for" to simplify your code

Instead of using an explicit iterator, your for loop could use "range-for":

for (const auto &person : contacts)

Simplify I/O by defining a function

This code outputs each contact when needed but it does so using a relatively long formatting string.  I'd suggest creating a function do that so that that formatting code is isolated to one place.  In particular, I'd create an operator<< function like so:

std::ostream& operator<<(std::ostream &out, const std::pair<const std::string, int> &p) {
    return out
        << std::setw(4) << "Name : " << std::setw(10) << std::left << p.first
        << std::setw(14) << ", Number : " << std::setw(10) << p.second;
}

Note, too, that I've used simply p.first rather than p.first.c_str().

Use an initializer list to initialize static data

Use of an initializer list simplifies construction of data structures:

std::map<std::string,int> contacts{
    {"Pradip",999092456},
    {"Raju",999092457},
    {"Praveed",999092445},
    {"Prasad",999092415},
    {"Asim",999092785},
    {"Massi",999092215}
};

Avoid continue and break

Looping constructs in code are generally considerably easier to read and understand if they do not use continue or break.  In this case, it's very easy to avoid them:

for (const auto &person : contacts) {
    if (string::npos != person.first.find(key)) {
        std::cout << person << std::endl;
    }
}

Think about the user

There is no obvious way to gracefully exit the program.  I'd suggest using a switch to control when the program is done.  Something like this:

bool done=false;
while (!done)
{
    if ( kbhit() ){

        key_code = getch();
        cls();
        switch (key_code) {
            case BACKSPACE:
                if (key.size()) {
                    key.pop_back();
                }
                break;
            case CTRL_C:
                done = true;
                break;
            default:
                key+=key_code;
        }
        std::cout << "Contacts with : " << key << std::endl;
        // etc.

Omit return 0

When a C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no reason to put return 0; explicitly at the end of main.


Springer book downloader in Kotlin I'm starting with Kotlin recently and am hoping to improve.

I wrote a small app to parse the list of free Springer books and download the books into your chosen local folder.

Comments around obvious mistakes, unidiomatic Kotlin, any other points of improvements will be greatly appreciated. Thank you.

Gradle dependencies:

    implementation("org.apache.poi:poi:4.1.2")
    implementation("org.apache.poi:poi-ooxml:4.1.2")
    implementation("org.jsoup:jsoup:1.13.1")

Kotlin code:

package dev.rayfdj.kotlinutils.springer

import org.apache.poi.ss.usermodel.WorkbookFactory
import org.jsoup.Jsoup
import java.io.File
import java.io.FileOutputStream
import java.net.URL
import java.nio.channels.Channels
import java.nio.file.Files
import java.nio.file.Path
import java.nio.file.Paths

data class Book(val title: String, val author: String, val edition: String, val year: String,
                val category: String, val url: String) {
    fun suggestedFileName(): String { return "$title, $edition - $author.pdf" }
}

fun extractBooksFromExcelFile(xlsxFile: File): List<Book> {
    // drop(1): skip the first row because it contains the headers
    return WorkbookFactory.create(xlsxFile).getSheetAt(0).drop(1).map {
        Book(it.getCell(0).stringCellValue, it.getCell(1).stringCellValue,
                it.getCell(2).stringCellValue, it.getCell(4).numericCellValue.toString(),
                it.getCell(11).stringCellValue, it.getCell(18).stringCellValue)
    }
}

fun deriveFullLocalPathForBook(downloadFolder: String, book: Book): Path {
    val fullLocalFileName = arrayOf(
            downloadFolder,
            book.category,
            book.suggestedFileName()).joinToString(separator = File.separator)
    return Paths.get(fullLocalFileName)
}

fun createDirectoriesAndFile(fullLocalFilePath: Path) {
    Files.createDirectories(fullLocalFilePath.parent)
    if(!Files.exists(fullLocalFilePath)) { Files.createFile(fullLocalFilePath) }
}

fun getBookDownloadURL(book: Book): URL {
    val bookPage = Jsoup.connect(book.url).get()
    val bookCanonicalURL = bookPage.select("link[rel=canonical]").attr("href")

    val bookCanonicalPage = Jsoup.connect(bookCanonicalURL).get()
    val bookPDFRelativeURL = bookCanonicalPage.select("a[href^=\"/content/pdf/\"]").attr("href")

    return URL("https://link.springer.com${bookPDFRelativeURL}")
}

fun downloadAndSaveBook(bookDownloadURL: URL, fullLocalFilePath: Path) {
    Channels.newChannel(bookDownloadURL.openStream()).use { inChannel ->
        FileOutputStream(fullLocalFilePath.toFile()).channel.use { outChannel ->
            print("Saving $bookDownloadURL to $fullLocalFilePath... ")
            outChannel.transferFrom(inChannel, 0, Long.MAX_VALUE)
            println("DONE.")
        }
    }
}

fun main(args: Array<String>) {
    if(args.size != 2) {
        println("Please pass <full_path_to_springer_excel_file> and <full_path_to_download_folder")
        kotlin.system.exitProcess(-1)
    }
    val (excelFile, downloadFolder) = args

    val books = extractBooksFromExcelFile(File(excelFile))

    books.forEach { book ->
        val fullLocalFilePath = deriveFullLocalPathForBook(downloadFolder, book)
        createDirectoriesAndFile(fullLocalFilePath)
        val bookDownloadURL = getBookDownloadURL(book)
        downloadAndSaveBook(bookDownloadURL, fullLocalFilePath)
    }
}

It's not exact translation of your code, but still :) I've just tried!

Unfortunately, it's not a github project, so I don't have an access to the excel sheet, so I can't really run and test it. But, anyways, ideas are:

Utilization of toString() java method
Using of lazy properties allows to 'cache' the URL easily. It's also possible because all of the Books properties are immutable.
Extension method download() doesn't really belong to a book, but, instead,
can easily download any URL
I used require(), though, it's not really correct, since it throws an exception, instead of gracefully exiting the app. I used it, just to demonstrate it, nothing more.

And, probably, something else. Please, tell me what you're thinking!

data class Book(
    val title: String,
    val author: String,
    val edition: String,
    val year: String,
    val category: String,
    val url: String
) {
    constructor(row: Row) : this(
        row.getCell(0).stringCellValue, row.getCell(1).stringCellValue,
        row.getCell(2).stringCellValue, row.getCell(4).numericCellValue.toString(),
        row.getCell(11).stringCellValue, row.getCell(18).stringCellValue
    )

    override fun toString() = "$title, $edition - $author"

    val downloadURL by lazy {
        val canonicalURL = Jsoup.connect(url).get().select("link[rel=canonical]").attr("href")
        val pdfRelativeURL =
            Jsoup.connect(canonicalURL).get().select("""a[href^="/content/pdf/"]""").attr("href")
        URL("https://link.springer.com${pdfRelativeURL}")
    }
}

fun URL.download(to: Path): Path {
    Files.createDirectories(to.parent)

    Channels.newChannel(openStream()).use { inChannel ->
        FileOutputStream(to.toFile()).channel.use { outChannel ->
            print("Saving $this to $to... ")
            outChannel.transferFrom(inChannel, 0, Long.MAX_VALUE)
            println("DONE.")
        }
    }

    return to
}

fun main(args: Array<String>) {
    require(args.size == 2) { "Please pass <full_path_to_springer_excel_file> and <full_path_to_download_folder" }

    val (excelPath, downloadFolder) = args
    val excelFile = WorkbookFactory.create(File(excelPath))

    // drop(1): skip the first row because it contains the headers
    val books = excelFile.getSheetAt(0).drop(1).map { Book(it) }

    books.map { book ->
        val path = Paths.get(downloadFolder, book.category, "$book.pdf")
        book.downloadURL.download(to = path)
    }
}
```


Extracting citations of court documents in multiple languages I've got 20'000+ court documents I want to pull specific data points out of: date, document number, verdict. I am using Python and Regex to perform this.

The verdicts are in three languages (German, French and Italian) and some of them have slightly different formatting. I am trying to develop functions for the various data points that take this and the different languages into regards.

I'm finding my functions very clumsy. Has anybody got a more pythonic way to develop these functions?

def gericht(doc):
    Gericht = re.findall(
       r"Beschwerde gegen [a-z]+ [A-Z][a-züöä]+ ([^\n\n]*)", doc)
    Gericht1 = re.findall(
       r"Beschwerde nach [A-Za-z]. [0-9]+ [a-z]+. [A-Z]+ [a-z]+ [a-z]+[A-Za-z]+ [a-z]+ [0-9]+. [A-Za-z]+ [0-9]+ ([^\n\n]*)", doc)
    Gericht2 = re.findall(
       r"Revisionsgesuch gegen das Urteil ([^\n\n]*)", doc)
    Gericht3 = re.findall(
       r"Urteil des ([^\n\n]*)", doc)
    Gericht_it = re.findall(
       r"ricorso contro la sentenza emanata il [0-9]+ [a-z]+ [0-9]+ [a-z]+ ([^\n\n]*)", doc)
    Gericht_fr = re.findall(
       r"recours contre l'arrêt ([^\n\n]*)", doc)
    Gericht_fr_1 = re.findall(
       r"recours contre le jugement ([^\n\n]*)", doc)
    Gericht_fr_2 = re.findall(
       r"demande de révision de l'arrêt ([^\n\n]*)", doc)

    try:
       if Gericht != None:
          return Gericht[0]
    except:
         None

    try:
       if Gericht1 != None:
          return Gericht1[0]
    except:
        None

    try:
       if Gericht2 != None:
          return Gericht2[0]
    except:
       None

    try:
       if Gericht3 != None:
          return Gericht3[0]
    except:
       None

    try:
      if Gericht_it != None:
          return Gericht_it[0]
    except:
      None

    try:
     if Gericht_fr != None:
       Gericht_fr = Gericht_fr[0].replace('de la ', '').replace('du ', '')
         return Gericht_fr
    except:
      None

   try:
       if Gericht_fr_1 != None:
           Gericht_fr_1 = Gericht_fr_1[0].replace('de la ', '').replace('du ', '')
           return Gericht_fr_1
   except:
       None

   try:
      if Gericht_fr_2 != None:
         Gericht_fr_2 = Gericht_fr_2[0].replace('de la ', '').replace('du ', '')
         return Gericht_fr_2
   except:
       None

Always use 4 spaces for your indentation, you use; 3, 4, and 5. If you're one space out, then it can break your code, so it really does matter.

You're doing roughly the same for all your different regexes, and so you should loop through the regexes, with the common code in the loop.
However I'd change your common code to not replace de la or du, and to not need the try-except as you can check if gericht is truthy,
so it isn't indexed if it's None or an empty array.
Take:

REGEXES = [
    r"Beschwerde gegen [a-z]+ [A-Z][a-züöä]+ ([^\n\n]*)",
    r"Beschwerde nach [A-Za-z]. [0-9]+ [a-z]+. [A-Z]+ [a-z]+ [a-z]+[A-Za-z]+ [a-z]+ [0-9]+. [A-Za-z]+ [0-9]+ ([^\n\n]*)",
    r"Revisionsgesuch gegen das Urteil ([^\n\n]*)",
    r"Urteil des ([^\n\n]*)",
    r"ricorso contro la sentenza emanata il [0-9]+ [a-z]+ [0-9]+ [a-z]+ ([^\n\n]*)",
    r"recours contre l'arrêt ([^\n\n]*)",
    r"recours contre le jugement ([^\n\n]*)",
    r"demande de révision de l'arrêt ([^\n\n]*)",
]

def gericht(doc):
    for regex in REGEXES:
        gericht = re.findall(regex, doc)
        if gericht:
            return gericht[0]

However this is needlessly finding all items for the regex in the document, which is not what you want. You just want the first.
To do this without editing the regexes you could use itertools.chain and use re.finditer.
This can get you:

from itertools import chain

def gericht(doc):
    gericht = next(chain.from_iterable(re.finditer(r, doc) for r in REGEXES), None)
    if gericht is not None:
        return gericht.group(0)

You may be able to change this to merge all the regexes together with |, but then you won't prioritize the first regex over the others. Which may work with:

def gericht(doc):
    gericht = next(re.finditer('|'.join(REGEXES), doc), None)
    if gericht is not None:
        return gericht.group(0)

But I'm not too good with regexes.


Deserialize, Copy and Merge Orders from XML to C# Here's the flow,

Load XML file
Deserialize to list in c#
Map xml object to c# list
Merge orders but increment order's product count

public Order[] Load(string path)
{
    var element = XElement.Load(path);
    var orders = new List<XElement>();
    orders = element.Elements();

    var _orders = new List<Order>();
    var _alreadyAddedOrders = new List<string>();
    foreach (var order in orders)
    {
        var orderTemp = new Order();
        orderTemp.OrderDate = Convert.ToInt32(order.Attribute("date").ToString());
        orderTemp.Id = Convert.ToInt32(order.Attribute("id").ToString());
        orderTemp.CategoryId = Convert.ToInt32(order.Attribute("categoryId").ToString());
        orderTemp.ProductCount = Convert.ToInt32(order.Attribute("productCount").ToString());

        foreach (var secondOrder in _orders)
        {
            var orderMatches = (orderTemp.OrderDate == secondOrder.OrderDate &&
                                orderTemp.Id == secondOrder.Id &&
                                orderTemp.CategoryId == secondOrder.CategoryId);
            if (orderMatches)
            {
                orderTemp.ProductCount += secondOrder.ProductCount;
                _alreadyAddedOrders.Add(secondOrder.OrderDate.ToString() + secondOrder.Id.ToString() + secondOrder.CategoryId.ToString());
            }
        }

        if(!_alreadyAddedOrders.Contains(orderTemp.OrderDate.ToString() + orderTemp.Id.ToString() + orderTemp.CategoryId.ToString()))
            _orders.Add(orderTemp);
    }

    return _orders.ToArray();
}

public class Order
{
    public int Id;
    public int CategoryId;
    public int OrderDate;
    public int ProductCount;
}

and XML is like this, more then 20,000 rows,

<orders>
    <order date='2000-04-20' Id='1' categoryId='100' productCount='3' />
    <order date='2000-06-21' Id='2' categoryId='101' productCount='14' />
</orders>

Note : Order ID is not unique, date + id + category, is unique altogether

Naming
var orders = new List<XElement>();
var _orders = new List<Order>();

First, according to the C# naming convention, you should never have underscores in your variable names declared inside methods. Second, this is pretty confusing.
How about :
var xmlOrders = new List<XElement>();
var convertedOrders = new List<Order>();

It's a weird (and probably controversial) thing to say, but naming has to be one of the hardest thing to do when it comes to programming. You want your name to be so clear that anyone reading your code would be able to understand it quickly. So names need to be clear and exact.
For example : secondOrder isn't a second order, so it's confusing.
What is it, how should it be named? Well it seems like this order is an alreadyAddedOrder. You might want to find another name, but it should give the same "vibe".
C# things

You can use the object initializer :
var orderTemp = new Order();
orderTemp.OrderDate = Convert.ToInt32(order.Attribute("date").ToString());
orderTemp.Id = Convert.ToInt32(order.Attribute("id").ToString());
orderTemp.CategoryId = Convert.ToInt32(order.Attribute("categoryId").ToString());
orderTemp.ProductCount = Convert.ToInt32(order.Attribute("productCount").ToString());

becomes :
var orderTemp = new Order
{
    OrderDate = Convert.ToInt32(order.Attribute("date").ToString()),
    Convert.ToInt32(order.Attribute("id").ToString()),
    //etc..
};

It's simply shorter and clearer, but it is compiled the same way as your previous code.

Note : Order ID is not unique, date + id + category, is unique altogether

We've got a great case of override Equals.
As it is right now, the responsibility of deciding if two orders are equal is stored (as code) in your Load method. Is this fine? I don't think so!
An Order should be able to tell if it's equal to another Order`.
public class Order
{
    public int Id;
    public int CategoryId;
    public int OrderDate;
    public int ProductCount;

    public override Equals(object obj)
    {
       //etc.
    }
}

I recommend you read a little bit on how to correctly implement the Equals override as you'll also need to override the GetHashCode method.

Don't use Convert.ToInt32 when you have a string that should be an int. Use int.Parse. reference

Also, override ToString to replace orderTemp.OrderDate.ToString() + orderTemp.Id.ToString() + orderTemp.CategoryId.ToString().

Encapsulation
public class Order
{
    public int Id;
    public int CategoryId;
    public int OrderDate;
    public int ProductCount;
}

Now, you might not feel it's necessary to implement this as it's a pretty "defensive programming" thing, but I feel it's important to talk about it. If I look at this class from a business point of view, it means that if I pass an order, I should be able to modify the said order afterwards. But when an order is passed, should I be able to modify, say, the OrderDate? Not really. There's also an important point that once you override the GetHashCode method, you should consider making your object "read only", so that it doesn't mess with the hashes.
Order becomes :
public class Order
{
    public Order(int id, int categoryId, int orderDate, int productCount)
    {
        //etc.
    }

    public int Id { get; }
    public int CategoryId { get; }
    public int OrderDate { get; }
    public int ProductCount { get; }

    //Equals and GetHashCode override.
}

General points

Don't use _alreadyAddedOrders to keep string. You're looking for orders, this list should contain orders.

Why is OrderDate an int? If it's stored as milliseconds, it should be a long, otherwise I don't see how you can parse an int to a meaningful date.


Morse code translator in C# I'm writing a Morse code translator for homework in C#. It takes an input from the user and returns the Morse code version of their input. I understand that this code may look horrible, so how could I improve it in terms of efficiency and readability?

Program.cs

using System;
using System.Collections.Generic;

namespace MorseCodeTranslator
{
    class Program
    {
        static Dictionary<char, string> translator;

        static void Main(string[] args)
        {
            InitialiseDictionary();
            getUserInput();
        }

        private static void InitialiseDictionary()
        {
            char dot = '.';
            char dash = '−';

            translator = new Dictionary<char, string>()
            {
                {'a', string.Concat(dot, dash)},
                {'b', string.Concat(dash, dot, dot, dot)},
                {'c', string.Concat(dash, dot, dash, dot)},
                {'d', string.Concat(dash, dot, dot)},
                {'e', dot.ToString()},
                {'f', string.Concat(dot, dot, dash, dot)},
                {'g', string.Concat(dash, dash, dot)},
                {'h', string.Concat(dot, dot, dot, dot)},
                {'i', string.Concat(dot, dot)},
                {'j', string.Concat(dot, dash, dash, dash)},
                {'k', string.Concat(dash, dot, dash)},
                {'l', string.Concat(dot, dash, dot, dot)},
                {'m', string.Concat(dash, dash)},
                {'n', string.Concat(dash, dot)},
                {'o', string.Concat(dash, dash, dash)},
                {'p', string.Concat(dot, dash, dash, dot)},
                {'q', string.Concat(dash, dash, dot, dash)},
                {'r', string.Concat(dot, dash, dot)},
                {'s', string.Concat(dot, dot, dot)},
                {'t', string.Concat(dash)},
                {'u', string.Concat(dot, dot, dash)},
                {'v', string.Concat(dot, dot, dot, dash)},
                {'w', string.Concat(dot, dash, dash)},
                {'x', string.Concat(dash, dot, dot, dash)},
                {'y', string.Concat(dash, dot, dash, dash)},
                {'z', string.Concat(dash, dash, dot, dot)},
                {'0', string.Concat(dash, dash, dash, dash, dash)},
                {'1', string.Concat(dot, dash, dash, dash, dash)},
                {'2', string.Concat(dot, dot, dash, dash, dash)},
                {'3', string.Concat(dot, dot, dot, dash, dash)},
                {'4', string.Concat(dot, dot, dot, dot, dash)},
                {'5', string.Concat(dot, dot, dot, dot, dot)},
                {'6', string.Concat(dash, dot, dot, dot, dot)},
                {'7', string.Concat(dash, dash, dot, dot, dot)},
                {'8', string.Concat(dash, dash, dash, dot, dot)},
                {'9', string.Concat(dash, dash, dash, dash, dot)}
            };
        }

        public static void getUserInput()
        {
            string input;
            Console.WriteLine("What did you want to say?");
            input = Console.ReadLine();
            input = input.ToLower();
            Console.WriteLine("Your output is: " + translate(input));
            Console.WriteLine("Press enter to end.");
            Console.ReadLine();
        }

        private static string translate(string input)
        {
            System.Text.StringBuilder sb = new System.Text.StringBuilder();
            foreach(char character in input)
            {
                if(translator.ContainsKey(character))
                {
                    sb.Append(translator[character] + " ");
                } else if (character == ' ')
                {
                    sb.Append("/ ");
                } else
                {
                    sb.Append(character + " ");
                }
            }
            return sb.ToString();
        }
    }
}

Is translator really a translator? IMO a better name could be dictionary.

I also don't understand an idea to create variables like dot and dash.
{'a', ".-"} would be much clearer.

Don't use misleading function names. The function getUserInput does much more than getting something from user! It also translates the text and prints it. So, in Main, I'd write:

input = getUserInput();
morse = translate(input);
PrintOnScreen(morse);

Be consistent in naming: now some functions have names starting with capital letter, some not.


Python: Open Multiple Files I am trying to open two CSV files, one for reading and one for writing. The script should continue only if both files were successfully opened. My code seems to accomplish that but it seems like it could be improved further.

My questions are:

Should I consider a different approach?
My code feels a bit bloated inside the try statement (imagine 100 additional lines of code). Is there a "cleaner" approach?
If the files were opened successfully, but a different error occurs later on that is not an IOError, this code will not catch it, correct?

Code:

input_file = 'in_file.csv'
output_file = 'out_file.csv'

def main():

    try:
        with open(input_file, 'rb') as in_csv, open(output_file , 'wb') as out_csv:
            writer = csv.writer(out_csv, delimiter='\t')
            reader = csv.reader(in_csv, delimiter='\t')
            for row in reader:
                # many lines of code...
    except IOError as e:
        print "Error: cannot open file"
        if e.errno == errno.EACCES:
            print "\tPermission denied."
            print "\tError message: {0}".format(e)
            sys.exit()
        # Not a permission error.
        print "\tDoes file exist?"
        print "\tError message: {0}".format(e)
        sys.exit()

if __name__ == '__main__':
    main()

To answer your questions…

Your approach is fine.
Move your hundreds of lines into a function, then break down that function into reasonable-sized chunks.  If it's still huge and ugly, then pose that as another code review question.
If an error other an IOError occurred, then the error would not get caught.  However, when the with block is exited for any reason, input_file and output_file will get properly closed.

In addition to what @JimDennis said, I would like to point out that it it customary to print error messages to sys.stderr, and exit with a non-zero status when an error occurs.

def process(csv_reader, csv_writer):
    for row in csv_reader:
        # many lines of code...

def main(input_filename, output_filename):
    try:
        with open(input_filename, 'rb') as in_csv, open(output_filename , 'wb') as out_csv:
            writer = csv.writer(out_csv, delimiter='\t')
            reader = csv.reader(in_csv, delimiter='\t')
            process(reader, writer)
    except IOError as e:
        print >> sys.stderr, "Error: cannot open file"
        if e.errno == errno.EACCES:
            print >> sys.stderr, "\tPermission denied."
            print >> sys.stderr, "\tError message: {0}".format(e)
            sys.exit(1)
        # Not a permission error.
        print >> sys.stderr, "\tError message: {0}".format(e)
        sys.exit(1)
    except Exception as other_exception:
        print >> sys.stderr, "Error: " + str(other_exception)
        sys.exit(2)

if __name__ == '__main__':
    main('in_file.csv', 'out_file.csv')


"command preprocessing" with bash - string parsing and handling pipes The following is a bash script named wrapper.sh:

#!/bin/bash

data="ZAA:huhu;ZBB:blub;___uname" # actually coming from a Python script

i=1 # counter
while true ; do # loop infinitely
    segment=$(echo $data | cut -d ';' -f$i)
    if [ -z "$segment" ] ; then # is segment empty?
        break
    fi
    if [[ $segment == ___* ]] ; then # is this a command?
        cmd="${segment:3}" # strip command prefix
        $cmd "${@:2}" # run *interactive* command
        # cat | $cmd "${@:2}" # "OPTIONAL": pass data from stdin ONLY IF THERE IS A PIPE (?)
    else
        varname=$(echo $segment | cut -d ':' -f1)
        vardata=$(echo $segment | cut -d ':' -f2)
        export $varname=$vardata # export environment variable
    fi
    i=$((i + 1)) # increment counter
done

# Just a test
echo $ZAA
echo $ZBB

The above script has to accomplish the following goals:

Pass its own parameters to a Python script, which processes them and returns a data string (intentionally left out of the above example)
Set (export) an arbitrary number of environment variables. Their names and contents are provided in data.
Run a command also provided in data and pass on all but the first argument given to the shell script.

Goal the code does not achieve yet: If a pipe is passed to the script (as in some_program | ./wrapper.sh), the pipe is supposed to passed on to the command called by wrapper.sh. The statement cat | $cmd "${@:2}" basically does this, but it hangs if no pipe is passed to wrapper.sh.

Why? Well actually, this thing started as a pure Python script, which was calling the command through subprocess.Popen. But it turned out insanely hard and error-prone to run an interactive command like this where an actual human user is supposed to interact with the (running) command. This is something bash is infinitely better at.

Example:

(env) user@box:~> ./wrapper.sh
Linux
huhu
blub
(env) user@box:~> ./wrapper.sh omitted
Linux
huhu
blub
(env) user@box:~> ./wrapper.sh omitted -a
Linux X.X 4.X.XXX #1 SMP XXX 2018 (XXX) x86_64 x86_64 x86_64 GNU/Linux
huhu
blub

I am not exactly a frequent bash coder, so any ideas on how to improve the above are highly welcome. I might also have overlooked "edge cases" where my script does something strange - I'd love to know.

The infinite loop with break, counter, and multiple invocations of cut looks clumsy at best. Consider instead

IFS=";"
for segment in $data; do
    # your logic here
done

Similarly, cutting the segment is unnecessary:

    IFS=":"
    set $segment
    export $1=$2
    IFS=";"

does the same job.
I don't understand the goal not achieved yet. The script has no knowledge where its input coming from. Whoever calls your script is in the position to redirect it at will. Perhaps, close stdin before invoking it?


jQuery - Repeating same actions on different DOM elements I feel like I’m violating a lot of DRY rules here but can’t think of a better way to do this. I have two event methods (there are actually four total just showing two) that are doing a lot of similar actions but on different DOM elements. Is there a better way to re-factor this so I’m not repeating myself so much.

    $('.user .expandDetails a').on('tap', function () {
        var expandWrapper = $('.userFooter');
        if ($('.userInfo .details').is(':hidden')) {
            $('.userInfo .details').addClass('active').slideDown();
            $('.userInfo .expandUserDetails').removeClass('down').addClass('up').addClass('active');
        } else {
            $('.userInfo .expandUserDetails').removeClass('up').addClass('down').removeClass('active');
            $('.userInfo .details').removeClass('active').slideUp();
        }
    });

    $('.user .paymentDetails a').on('tap', function () {
        var expandWrapper = $('.paymentFooter');
        if ($('.paymentInfo .details').is(':hidden')) {
            $('.paymentInfo .details').addClass('active').slideDown();
            $('.paymentInfo .expandPaymentDetails').removeClass('down').addClass('up').addClass('active');
        } else {
            $('.paymentInfo .expandPaymentDetails').removeClass('up').addClass('down').removeClass('active');
            $('.paymentInfo .details').removeClass('active').slideUp();
        }
    });

Please include code samples where applicable somewhat new to jquery.

You're right, you are violating some DRY rules, so lets clean it up. First, I would cache some of the selectors you're calling.

var $info = $('.userInfo');
var $details = $info.find('.details');
var $detailsExpand = $info.find('.expandUserDetails');

Then I would take a look at some of the toggle options that jQuery offers, mainly:

toggleClass
slideToggle

This will save you the trouble of having to use .addClass, .removeClass, slideUp, and slideDown.

For example, you have:

if ($('.paymentInfo .details').is(':hidden')) {
    $('.paymentInfo .details').addClass('active').slideDown();
    $('.paymentInfo .expandPaymentDetails').removeClass('down').addClass('up').addClass('active');
} else {
    $('.paymentInfo .expandPaymentDetails').removeClass('up').addClass('down').removeClass('active');
    $('.paymentInfo .details').removeClass('active').slideUp();
}

We can remove that else statement, and just let our new toggle methods handle the adding/removing.

if ($details.is(':hidden')) {
    $details
        .toggleClass('active', true) // true means the class will be added
        .slideToggle();

    $detailsExpand
        .toggleClass('down', false)  // false means the class will be removed
        .toggleClass('up', true)
        .toggleClass('active', true);
}

Lastly, and this might be overkill, you could try creating a jQuery plugin:

$.fn.toggleDetails = function (options) {
    var settings = $.extend({
        infoClass: null,
        expandClass: null
    }, options);

    var $info = $(settings.infoClass);

    var $details = $info.find('.details');
    var $detailsExpand = $info.find(settings.expandClass);

    if ($details.is(':hidden')) {
        $details
            .toggleClass('active', true)
            .slideToggle();

        $detailsExpand
            .toggleClass('down', false)
            .toggleClass('up', true)
            .toggleClass('active', true);
    }
};

And then applying your new plugin would be as easy as calling the .toggleDetails method on your selected elements, and providing both an infoClass and expandClass.

$('.user .expandDetails a').toggleDetails({
    infoClass: '.userInfo',
    expandClass: '.expandUserDetails'
});

$('.user .paymentDetails a').toggleDetails({
    infoClass: '.paymentInfo',
    expandClass: '.expandPaymentDetails'
});

Caveats:

This code hasn't been tested.
I'm not sure how to assign a down or up to slideToggle. I would have expected a similar true/false parameter similar to toggleClass.