Strategy pattern - design hash table My programming skill has gotten rusty, so I would like to elicit constructive criticism that can make me write code better. I know the standard design is to use templates, and even obtain the hash function, allocator, etc through it, but I wanted to experiment with patterns, so I'm pretty sure my design here blows. but I'm trying to use the strategy pattern to make a customizable hash table interface, just to reacquaint myself with C++.

How do I design this better (without using all the template parameters, for specifying hash function, etc)? If not the strategy pattern, what pattern would fit better?

//The generic map interface
template <typename K, typename V>
class Map
{
private:

    //Make a reference class so we can enable [] operation
    class reference
    {
        public:
            operator V const () { return map_.Find(key_); }

            reference& operator=(const V& val) { map_.Add(key_, val); return *this; }
            reference& operator=(const reference& ref) { *this = (V&) ref; return *this; }

        private:
            reference (Map& map, const K key) : map_(map), key_(key) {}
            Map& map_;
            K key_;
            friend class Map;
    };

public:
    Map() : hash_(NULL), ds_(NULL), size_(0) { }

    void SetHashStrategy(int hash_type = HashType::SIMPLE_HASH);
    void SetCollisionStrategy(int collision_method = CollisionType::CHAINING);
    void SetDataStructureStrategy(int data_structure_types = DSType::DYNAMIC_MAP);
    virtual ~Map()
    {
        Empty();
        delete hash_;
        delete ds_;
    }

    //Interface functions
    void   Add(K key,V value) { ++size_; ds_->Add(key, value); }
    void   Remove(K key)      { --size_; ds_->Remove(key); }
    V&     Find(K key)        { return ds_->Find(key); }
    size_t Size()             { return size_; }
    void   Empty()            { size_ = 0; ds_->Empty(); }

    //Square bracket access
    reference operator[] (const K& key) { return (reference(*this, key)); }

    //printing
    friend std::ostream& operator<< (std::ostream& o, const V& v) { return o << v; }

private:
    HashType* hash_; //is an ADT
    DataStructureType<K,V>* ds_; //this is also an ADT
    size_t size_;
};

And then, choose the hash object like this:

template <class K, class V>
void Map<K,V>::SetHashStrategy(int hash_type)
{
  if(hash_ != NULL)
  {
    delete hash_;
  }

  if(hash_type == HashType::SIMPLE_HASH)
  {
    hash_ = new SimpleHash;
  }
  else if(hash_type == HashType::MD5_HASH)
  {
    hash_ = new MD5Hash;
  }
}

And choose the data structure like this:

template <class K, class V>
void Map<K,V>::SetDataStructureStrategy(int ds_type)
{
  if(ds_ != NULL)
  {
    delete ds_;
  }

  if(ds_type == DSType::DYNAMIC_MAP)
  {
    ds_ = new DynamicMap<K,V>;
  }
  else if(ds_type == DSType::ARRAY_MAP)
  {
    ds_ = new ArrayMap<K,V>;
  }
}

You're misusing the term strategy pattern.
Instead of doing this kind of thing:

template <class K, class V>
void Map<K,V>::SetDataStructureStrategy(int ds_type)
{
  if(ds_ != NULL)
  {
    delete ds_;
  }

  if(ds_type == DSType::DYNAMIC_MAP)
  {
    ds_ = new DynamicMap<K,V>;
  }
  else if(ds_type == DSType::ARRAY_MAP)
  {
    ds_ = new ArrayMap<K,V>;
  }
}

The strategy pattern would look something more like this:

template <class K, class V>
void Map<K,V>::SetDataStructureStrategy(DataStructureStrategy * dsStrategy)
{
  this->dsStrategy = dsStrategy;
}

That is, you would have a DataStructureStrategy abstract parent class,
with concrete implementations in subclasses,
for example DynamicMapStrategy and ArrayMapStrategy.

And then, the code to initialize the data structure would look like this:

ds_ = dsStrategy->newDataStructure();

where newDataStructure would be implemented in the specialized subclasses like this:

// implementation of DynamicMapStrategy::newDataStructure
return new DynamicMap<K,V>;

// implementation of ArrayMapStrategy::newDataStructure
return new ArrayMap<K,V>;

In other words:

Your hashmap class shouldn't have to know how a strategy is implemented. At no point should the code have conditional statements on DSType. In fact there's no need for DSType.
The same goes for all the other kind of strategies that you implied: hash strategy, collision strategy. Define a common interface for each strategy that the hashmap wants to work with. Move the implementations of strategies outside of the hashmap, into subclasses of the common interfaces.
These strategies should not change during the lifetime of a hashmap. It wouldn't be efficient to switch the strategy of an existing hashmap. So it would be better to make these strategies required at construction time, and remove the setters.


Convert int64 to custom base64 number string I want to convert int64 to custom base 64 number string, with this specification:

0..9   -> 0..9
10..35 -> a..z
36..61 -> A..Z
62     -> _
63     -> -

Is this implementation correct/efficient?

func EncodeLogKey(id int64) string {
  str := make([]byte,0,12)
  if id == 0 { return "0" }
  for id>0 {
    var ch rune
    mod := rune(id % 64)
    if mod <= 10 {
      ch = '0' + mod
    } else if mod <= 10+26 {
      ch = 'a' + (mod - 10)
    } else if mod <= 10+26+26 {
      ch = 'A' + (mod - 10 - 26)
    } else if mod % 2 == 0 {
      ch = '_'
    } else {
      ch = '-'
    }
    str = append(str,byte(ch))
    id /= 64
  }
  return string(str)
}

func DecodeLogKey(logKey string) (int64, error) {
  res := int64(0)
  for ch := range logKey {
    res *= 64
    mod := int64(ch)
    if ch >= '0' && ch <= '9' {
      res += (mod - '0')
    } else if ch >= 'a' && ch <= 'z' {
      res += 10 + (mod - 'a')
    } else if ch >= 'A' && ch <= 'Z' {
      res += 10 + 26 + (mod - 'A')
    } else if ch == '_' {
      res += 10 + 26 + 26
    } else if ch == '-' {
      res += 10 + 26 + 26 + 1
    } else {
      return res, fmt.Errorf("Invalid logKey character: '%c'", res)
    }
  }
  return res, nil
}

Your solution as given has a few problems.  Firstly, DecodeLogKey doesn't work, as you're ranging over the index of logKey rather than the value of logKey.  Also, the range will start from the first character in the string, and for what you're doing, you need it to start from the last character in the string.

I'd suggest a solution like this:

var codes = "0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ_-"

func EncodeLogKey(id int64) string {
    str := make([]byte, 0, 12)
    if id == 0 {
        return "0"
    }
    for id > 0 {
        ch := codes[id%64]
        str = append(str, byte(ch))
        id /= 64
    }
    return string(str)
}

func DecodeLogKey(logKey string) (int64, error) {
    res := int64(0)

    for i := len(logKey); i > 0; i-- {
        ch := logKey[i-1]
        res *= 64
        mod := strings.IndexRune(codes, rune(ch))
        if mod == -1 {
            return -1, fmt.Errorf("Invalid logKey character: '%c'", ch)
        }
        res += int64(mod)
    }
    return res, nil
}

That way you can easily change the character set you're working with, and the code becomes a lot more readable.  It could be made more flexible by using len(codes) as the base rather than assuming it's 64.

You still need to include checks for non-Latin1 UTF-8 characters such as 好 if you want to be really thorough.

In terms of testing for correctness, you should write unit tests (preferably table driven) to be sure that your code is working correctly. Dave Cheney has a good post on Writing table driven tests in Go that you should read.

Play for my solution


Coin Change Algorithm - coding challenge Recently I came across a coding problem where I have to find the least positive value which is not possible to make using the coins present in the coin denomination array(array values can be used only once).
suppose that

if the input is 3 5 7, the answer is 1

if the input is 1 2 4, the answer is 8

if the input is 1 3 4, the answer is 2

I have written a solution using recursion but it has a time complexity of \$O(n!)\$.
It is given that the size of the array cannot be greater than 25 and no individual value being greater than 100,000.

    public class Test {
        public static void main(String[] args) {
            Scanner scan = new Scanner(System.in);
            int coins = scan.nextByte(); // max 25
            int maxPossible = 0;
            Integer[] coinsDenominationarray = new Integer[coins];
            for (int i = 0; i < coins; i++) {
                coinsDenominationarray[i] = scan.nextInt(); // max 1,00,000
                maxPossible += coinsDenominationarray[i];
            }
            Arrays.sort(coinsDenominationarray);
        /*
         * +1 because the coins will not be able to make 1 more than sum of all the coins.
         * Using this value for the upper limit for the for-loop.
         * In case the recursion is not able to find any value not possible to make
           using any combination of coins. This will be the final answer.
        */
            System.out.println(solve(maxPossible+1, coinsDenominationarray));
        }

        public static int solve(Integer maxNotPossible, Integer[] arr) {
            for (int num = 1; num < maxNotPossible; num++) {
                if (isPresent(num, arr)) {
                    continue;
                }
                if (!isPossible(num, 0, possibleDenominations(num, arr))) {
                    return num;
                }
            }
            return maxNotPossible;
        }

        public static boolean isPossible(int toMake, int sum, Integer[] arr) {
            if (toMake < sum) {
                return false;
            }
            for (int num : arr) {
                int currentSum = sum + num;
                if (currentSum == toMake) {
                    return true;
                } else if (currentSum > toMake) {
                    return false;
                } else if (isPossible(toMake, currentSum, removeAndReturn(num, arr))) {
                    return true;
                }
            }
            return false;
        }

        public static Integer[] removeAndReturn(Integer num, Integer[] arr) {
            List<Integer> list = new LinkedList<Integer>(Arrays.asList(arr));
            list.remove(num);
            return list.toArray(new Integer[list.size()]);
        }

        public static Integer[] possibleDenominations(Integer maxNum, Integer[] arr) {
            List<Integer> list = new ArrayList<>();
            for (int num : arr) {
                if (num >= maxNum) {
                    break;
                }
                list.add(num);
            }
            return list.toArray(new Integer[list.size()]);
        }

        public static boolean isPresent(int num, Integer[] arr) {
            List<Integer> list = Arrays.asList(arr);
            return list.contains(num);
        }

    }

link to the original challenge:
https://www.techgig.com/challenge/IG_Graduate_Programme_2018

Unfortunately, this is not an open challenge.

I'll start with a review of your existing code, and then make a suggestion
for a better algorithm.

The first thing to notice is that there are many conversions from arrays
to lists and back to arrays:

In the isPresent() function, the array is converted to a list in order
to check if it contains a given number. But the array is already sorted,
so one can call Arrays.binarySearch instead, which is more also efficient.
The possibleDenominations function “truncates” the given array by conversion
to a list and back to an array.
You could compute the last index to be included instead and then return
a subarray with Arrays.copyOfRange. But I am not sure if this truncating
is needed at all, since your solution algorithm already early returns if
the values become too large.
In the removeAndReturn function, the array is converted to a list
and back to an array in order to get a new array with the first element
removed.

All these conversions can be avoided completely: Either

keep the original array and pass indices (of the first and last
entry to be considered) around, or
store the values in an (Array)List right at the beginning, and use
list.subList() to pass views of a portion of the list around, without
actually copying the elements.

The method

public static boolean isPossible(int toMake, int sum, Integer[] arr) {

can be simplified by taking a single parameter targetSum instead of
toMake and sum:

public static boolean isPossible(int targetSum, Integer[] arr) {
    if (targetSum == 0) {
        return true;
    } else if (targetSum < 0) {
        return false;
    }
    for (int num : arr) {
        if (isPossible(targetSum - num, removeAndReturn(num, arr))) {
            return true;
        }
    }
    return false;
}

Further remarks:

“Test” is a non-descriptive name for a program. Why not name it after the
challenge, for example public class CoinChange?
Why do you call nextByte() if the result is assigned to an int?
The variable name coins does not clearly indicate that this is the
number of coins.
coinsDenominationarray is an array, but there is no need to make the type
part of the variable name, I would call it for example coinDenominations or
coinValues.
Most of the code is done in a separate function, which is good. I would put
the calculation of maxPossible and the sorting into that function as well,
so that I/O and the actual computation are separated completely.
The parameter name arr in the solve function  is non-descriptive.
Why not use the same name as in the main program?
All methods (with the only exception of main) can be private.

Putting it all together, the program could look like this:

import java.util.Scanner;
import java.util.ArrayList;
import java.util.List;
import java.util.Collections;

public class CoinChange {
    public static void main(String[] args) {
        Scanner scan = new Scanner(System.in);
        int numCoins = scan.nextInt();

        ArrayList<Integer> coinValues = new ArrayList<Integer>();
        for (int i = 0; i < numCoins; i++) {
            coinValues.add(scan.nextInt());
        }
        int firstNotPossible = findFirstNotPossible(coinValues);
        System.out.println(firstNotPossible);
    }

    private static int findFirstNotPossible(List<Integer> coinValues) {
        Collections.sort(coinValues);

        // Compute sum of values plus one:
        int maxNotPossible = 1;
        for (int value : coinValues) {
            maxNotPossible += value;
        }

        for (int num = 1; num < maxNotPossible; num++) {
            if (!coinValues.contains(num) && !isSummable(num, coinValues)) {
                return num;
            }
        }
        return maxNotPossible;
    }

    // Check if `targetSum` is representable as a sum of numbers in `values`
    // (which is assumed to be a sorted list of positive numbers).
    private static boolean isSummable(int targetSum, List<Integer> values) {
        if (targetSum == 0) {
            return true;
        } else if (targetSum < 0) {
            return false;
        }
        for (int i = 0; i < values.size(); i += 1) {
            if (isSummable(targetSum - values.get(i), values.subList(i + 1, values.size()))) {
                return true;
            }
        }
        return false;
    }
}

A better algorithm

Your isPossible() function solves the "Subset sum problem"
with “brute force,” i.e. by (recursively) trying all possible combinations.

This problem can be efficiently solved with "dynamic programming", as described in
the Wikipedia article.

But this problem can be solved even simpler:

A number \$ N \$  stores the lowest value which is not
representable as sum of coin values considered so far, initially \$ N = 1 \$.
For all values \$ v \$ in the (sorted) array of coin values:

If \$ v > N \$ then we are done, \$ N \$ is not representable.
Otherwise \$ N \$ is incremented by \$ v \$.

This requires only a single traversal of the array, and no additional storage:

import java.util.Scanner;
import java.util.Arrays;

public class CoinChange {
    public static void main(String[] args) {
        Scanner scan = new Scanner(System.in);
        int numCoins = scan.nextInt();

        int[] coinValues = new int[numCoins];
        for (int i = 0; i < numCoins; i++) {
            coinValues[i] = scan.nextInt();
        }
        int firstNotPossible = findFirstNotPossible(coinValues);
        System.out.println(firstNotPossible);
    }

    private static int findFirstNotPossible(int[] coinValues) {
        Arrays.sort(coinValues);

        int firstNotPossible = 1;
        for (Integer value : coinValues) {
            if (value > firstNotPossible) {
                break;
            }
            firstNotPossible += value;
        }
        return firstNotPossible;
    }
}


Prefixing prices of certain product categories with text in Woocommerce I had the need to prefix the price for certain product categories on only the shop page (because the price can be higher depending on extras that the customer might want).

These were my requirements for this code:

Add the prefix only on the shop page, not on the product page;
Only add the prefix to certain (hardcoded) product categories.

I'd like some advice on possible improvements and certainly about possible security risks, if there are any.

Along with that, I've heard that globals are considered bad practise, but I see them a lot in Wordpress. What are the alternatives, if there's any.

This is the code:

add_filter('woocommerce_get_price_html', 'change_product_price_html');
function change_product_price_html($price){

  // Check if we're on the shop page
  // Don't want the prefix on the product pages.
  if( is_shop() ) {
    global $product;

    // ID of categories to add the prefix to
    $categories = ["17"];

    // Loop through all the category ID's if they equal one of the values
    // in the array, add the prefix.
    foreach( $product->get_category_ids() as $id ) {
      if( in_array($id, $categories) ) {

        // Add prefix
        $prefixedPrice   = "<span class='woocommerce-Price-amount amount'>Starting at </span>";
        $prefixedPrice  .= $price;

        return $prefixedPrice;
      }
    }
    // Nothing matched in the loop,
    // return the price without the prefix.
    return $price;
  }
  // We're not on the shop page so
  // return the price without the prefix.
  return $price;
}

You could reduce your script to this: (untested)
add_filter('woocommerce_get_price_html', 'change_product_price_html');
function change_product_price_html($product, $price) {
    $categories = ["17"];
    if (!is_shop() || !array_intersect_key(array_flip($product->get_category_ids()), array_flip($categories))) {
        return $price;
    }
    return "<span class=\"woocommerce-Price-amount amount\">Starting at </span>{$price}";
}

pass the $product into the custom function's scope as an argument to avoid the global declaration.

merge the un-prefixed return conditions into a single expression.

while only a benchmark executed on your system will tell the real truth, I am employing a key-based comparison between two arrays.  The downside is that it may do more work than it needs to (by finding multiple matches). The good news is, it is not going to perform multiple full array scans like a looped in_array() technique might.  I'm not going to be too pushy about how to best compare the two category arrays -- I just wanted to show an alternative.

Alternatively, this will be an easier array-to-array comparison to read:
if (!is_shop() || !array_intersect($product->get_category_ids(), $categories)) {
    return $price;
}

I assume you were calling your posted function like change_product_price_html($price);.
To add the $product variable as the first expected argument, call my custom function like this:
change_product_price_html($product, $price);


Specifying the Kind on DateTime I've got a system and every object in my system has a base class with property: DateTime DateCreated

In the constructor for the base class I set the DateCreated as below:

public EntityBase()
{
    Active = true;
    DateCreated = DateTime.UtcNow;
}

This has been fine but now I'm exposing my data as json via a web service call. As the date are UTC I want the format to have the Z suffix as below:

2016-08-01T10:39:18Z

The DateCreated in each object has Kind set to Utc because I'm setting it using DateTime.UtcNow. However, as I load the data from the database the Kind remains as Unspecified (I assumed that the UTC part would be saved to the database but it appears not). When I return the dates via the web service function I get it without the Z suffix so the calling application doesn't know it is UTC:

2016-08-01T10:39:18

It seems weird to me that the UTC Kind of the DateTime object isn't saved to the database - I thought it would be. The datatype is datetime2(7).

To get the date returned as required I've changed the property in the base class to this:

private DateTime _DateCreated;
[DisplayName("Date Created")]
public DateTime DateCreated
{
    get { return _DateCreated; }
    set
    {
        _DateCreated = value;
        _DateCreated = DateTime.SpecifyKind(_DateCreated, DateTimeKind.Utc);//ensure this is set
    }
}

This does exactly what I need but seems kind of hacky - Is this really the best way?

After Googling someone suggests that using DateTimeOffset would be more appropriate. I'm not convinced? This would have me questioning the use of DateTime in any scenario - and always using DateTimeOffset!?!

_DateCreated = value;
        _DateCreated = DateTime.SpecifyKind(_DateCreated, DateTimeKind.Utc);//ensure this is set

This creates a race condition: another thread can access _DateCreated in between the two assignments and get a non-UTC value. If you're going to do it at this level it should be

        _DateCreated = DateTime.SpecifyKind(value, DateTimeKind.Utc);

That leaves the question of whether it should be done at this level. You haven't said how you're getting the date from the database. Can that be configured?

  This would have me questioning the use of DateTime in any scenario - and always using DateTimeOffset!?!

Yes. DateTime is one of those mistakes that everyone recognises is a mistake, but is so deeply embedded in so many libraries and tools that it's a pain in the neck to eradicate.


Dynamic variables in PHP from enum Related to the question Verb conjugator for French, I was asked an question on whether one could summarize all the $exceptionIs<NAME_OF_EXCEPTION> = $exceptionmodel-> getValue() === ExceptionModel::NAME_OF_EXCEPTION lines. In other words, can one make dynamic variables out of the comparison of a value combined with all values of an enum?

As I'm a bit rusty in php, I made this version, and ask you whether this is a good solution, or if it needs major refactoring. To avoid posting other peoples code, I've mocked the ExceptionModel class, and replace the output of ExceptionModel::getConstants() with a predefined array. This to give you working code to review.  The original code is located on github, as the classes ExceptionModel and Enum.

<?php
// A mockup of the original ExceptionModel inheriting from Enum
class ExceptionModel {
    const NO_EXCEPTIONS = 'no_exception';
    const ALLER = 'aller';
    const AVOIR_IRR = 'avoir_irr';
    const ETRE_IRR = 'etre_irr';

    // ... many more lines ...

    function getConstants() {
        // ... returns array of constants ...
    }
}

function myFunction($exception) {
    // In final version, it should use the following line
    // $exceptionModels = ExceptionModel::getConstants();

    // ... but for now, use this array
    $exceptionModels = array (
        "NO_EXCEPTIONS" => 'no_exception',
        "ALLER" => 'aller',
        "AVOIR_IRR" => 'avoir_irr',
        "ETRE_IRR" => 'etre_irr'
    );

    // Generate dynamic variables testing for equality of
    // of $exception and an Enum value from ExceptionModel
    foreach ($exceptionModels as $constName => $constValue) {
        ${'exceptionIs' . $constName} = $exception === $constValue;
    }

    if ($exceptionIsALLER) {
        echo "ExceptionModel is aller. ";
    }

    else if ($exceptionIsAVOIR_IRR) {
        echo "ExceptionModel is avoir_irr";
    } else {
        echo "ExceptionModel was neither, it is: " . $exception;
    }

    echo "\n";
}

echo "<pre>";
myFunction("none");
myFunction(ExceptionModel::ALLER);
myFunction(ExceptionModel::AVOIR_IRR);
echo "</pre>";
?>

This correctly produces the output:

ExceptionModel was neither, it is: none
ExceptionModel is aller.
ExceptionModel is avoir_irr

PHP allows computed variable names, but it's nearly always a bad idea.  Unless you are careful about variable naming conventions, you could easily trash a variable accidentally, possibly introducing a security vulnerability.

You would be better off using an associative array.

// Generate dynamic variables testing for equality of
// of $exception and an Enum value from ExceptionModel
foreach ($exceptionModels as $constName => $constValue) {
    $detected[$constName] = $exception === $constValue;
}

if ($detected['ALLER']) {
    echo "ExceptionModel is aller.";
} else if ($detected['AVOIR_IRR']) {
    echo "ExceptionModel is avoir_irr.";
} else {
    echo "ExceptionModel was neither, it is: " . $exception;
}


Comparing XML string with different formats I need to compare 2 xml strings where the formatting may be different. The approach is to use the XmlWriterSettings to format both strings and compare the formatted result:

public static class XmlHelper
{
    public static string FormatXml(string xml)
    {
        try
        {
            var stringBuilder = new StringBuilder();

            var element = XDocument.Parse(xml);

            var settings = new XmlWriterSettings();
            settings.OmitXmlDeclaration = true;
            settings.Indent = true;
            settings.IndentChars = "  ";
            settings.NewLineChars = Environment.NewLine;
            settings.NewLineOnAttributes = false;
            settings.NewLineHandling = NewLineHandling.Replace;

            using (var xmlWriter = XmlWriter.Create(stringBuilder, settings))
                element.Save(xmlWriter);

            return stringBuilder.ToString();
        }
        catch (Exception ex)
        {
            Logger.Error("Unable to format XML: '" + xml + "'", ex);
            return xml;
        }
    }

    public static bool CompareXml(string xmlA, string xmlB)
    {
        if (xmlA == null && xmlB == null)
            return true;
        if (xmlA == null || xmlB == null)
            return false;

        var xmlFormattedA = FormatXml(xmlA);
        var xmlFormattedB = FormatXml(xmlB);

        return xmlFormattedA.Equals(xmlFormattedB, StringComparison.InvariantCultureIgnoreCase);
    }
}

My questions are:

Do you see any problems with that approach?
Is there a simpler way to accomplish that kind of comparison?

Googling for xml compare c# produces countless results so I just review this code without suggesting anything alternative.

settings.IndentChars = "  ";

I find new string(' ', 3) is easier to understand then three (?) spaces.

    if (xmlA == null && xmlB == null)
        return true;

This is questionable. I'd say it should be false.

var settings = new XmlWriterSettings();
settings.OmitXmlDeclaration = true;

Don't you like object initializers?

var settings = new XmlWriterSettings
{
    OmitXmlDeclaration = true
};


Folding over a Rose Tree : Tree a -> [a] I posted this question on StackOverflow.

Given this algebraic data type:

data Tree a = Node {
    rootLabel :: a,
    subForest :: [Tree a]
}  deriving (Show)

and function:

treeFold :: (a -> [b] -> b) -> Tree a -> b
treeFold f (Node x ts) = f x (map (treeFold' f) ts)

Write a function (using treeFold) with the signature: Tree a -> [a].

Given this example:

ghci> stringTree
Node {rootLabel = "foo",
      subForest = [Node {rootLabel = "bar",   subForest = []},
                   Node {rootLabel = "bippy", subForest = []},
                   Node {rootLabel = "baz", subForest = []} ]}

I came up with:

ghci> treeFold' (\x y -> x : (join y) ) stringTree
["foo","bar","bippy","baz"]

Please review this implementation.

I'm not sure if introducing join here (from Control.Monad) is overkill. Note that I don't have a good sense as to whether it's OK to casually use Monads - mainly using the right abstraction - not going overboard.

I don't think that using monads here itself is overkill,
but I would suggest using concat instead of join.

There are no different Monads that could be an argument
to join here. Using concat is more apropriate (imo).


which for loop has better performance in javascript? I can't find out which loop is better performance than another, if you know help me.

I try about loops in js and performance in this site
and this is code snippet, you can run and check.

const array1 = [];

for (let i = 0; i <= 50000; i++) {
  array1.push(i);
}

//for...
var forT0 = performance.now()

for (let element = 0; element < array1.length; element++) {
  console.log(element);
}

var forT1 = performance.now()
console.log("Call to doSomething took " + (forT1 - forT0) + " milliseconds.")

//for...in
var forInT0 = performance.now()

for (const element in array1) {
  console.log(element);
}

var forInT1 = performance.now()
console.log("Call to doSomething took " + (forInT1 - forInT0) + " milliseconds.")

//for...of
var forOfT0 = performance.now()

for (const element of array1) {
  console.log(element);
}

var forOfT1 = performance.now()
console.log("Call to doSomething took " + (forOfT1 - forOfT0) + " milliseconds.")

//forEach
var forEachT0 = performance.now()

array1.forEach(element => console.log(element));

var forEachT1 = performance.now()
console.log("Call to doSomething took " + (forEachT1 - forEachT0) + " milliseconds.")

Not sure if this is a right fit for this site, as this is clearly not 'real' code, but let me give you an answer anyway.

First, in your test code, the "for" case is clearly set up incorrectly. It should look more like this:

for (let i = 0; i < array1.length; i++) {
  console.log(array1[i]);
}

Second, I don't think you are asking the right question. This is clearly a case of premature optimisation. There will be differences obviously, but in real life situations the difference will probably be marginal. It is much wiser to optimise for readability and developer comfort when writing code. Use the loop that makes most sense for you in your current situation. If you run into performance issues you can start looking at optimising then, but not before.

There are too many variables to draw conclusions from your test case. The size of the array you loop over for instance. On large arrays for will be probably be fastest (probably even a bit slower than a while loop). On small arrays a forEach is possibly faster, unless you do complex operations inside the loop. And all that is dependant on the javascript engine implementation and the hardware it runs on.

So as I said, don't worry about this stuff before you run into performance issues. And when you do, test your performance with real code and real data, not oversimplified testcode.


{Sum+=i++} to Reach N I am developing code for a problem posted on StackOverflow.
I have written my solution in Java but it is taking too much time to execute for numbers such as 21432154197846387216432.  How could I improve the performance of the following code?

import java.util.Scanner;

public class Main {

    // private static final double N = Double.parseDouble("916548345678631") ;
    // private static final long loopUntil = (int)(Math.sqrt(2 * N));
    // private static double M = 1.0 ;

    public static void main(String[] args) {

        Scanner scanner = new Scanner(System.in).useDelimiter("\n");
        while (scanner.hasNext()) {

            double N = Double.parseDouble(scanner.next());
            long loopUntil = (long) (Math.sqrt(2 * N));
            double M = 1.0;

            int doProcess = doProcess(N, loopUntil, M);
            System.out.println(doProcess);
        }
        scanner.close();
    }

    public static int doProcess(double N, long loopUntil, double M) {
        int result = 0;
        double constantDevisor = 1.0 / 2.0 ;

        // System.out.println("Loop Unttil " + loopUntil);
        while (M <= loopUntil) {
            double val = 0;
            // System.out.println((N / M) +" " + ( M / 2 ) + " " + (1.0/2.0));
            val = (N / M) + (M / 2) + constantDevisor;
            // System.out.println(val);
            if (Math.floor(val) == val) {
                System.out.println("With : " + M);
                result++;
            }
            M++;
        }

        return result;
    }

}

You have one serious problem - that supergigantic number you want to use is larger than the precision of both doubles AND longs. In fact, because both doubles and longs use 64 bits in Java, you would be better off using longs because with doubles, only 53 of those 64 bits are available for what you want to do.

What this means is that, in order to get a meaningful calculation AT ALL for 21432154197846387216432 that you would have to implement it with something else because the algorithm, as you have implemented it, will give you answers that are completely meaningless because your initial value is a victim of roundoff error.

Now, for numbers up to the limit of long (9223372036854775807), I have created a different implementation that uses only long operations. It's organized differently because we have to force the algorithm to work correctly without any fractional arithmetic.

public class MainTwo {

    private static final long N = 916548345678631l;

    public static void main(String[] args) {

        long start = System.currentTimeMillis();
        long loopUntil = (long) (Math.sqrt(2 * N));
        long M = 1l;

        int doProcess = doProcess(N, loopUntil, M);
        System.out.println(doProcess);

        long end = System.currentTimeMillis();
        System.out.println("[Main.main] elapsed time: " + (end - start));
    }

    public static int doProcess(long N, long loopUntil, long M) {
        int result = 0;

        while (M <= loopUntil) {

            boolean yesProcess = false;
            long val = 0;
            boolean mEven = (M % 2) == 0;
            if (mEven) {
                yesProcess = (N % M == M/2);
                if (yesProcess) {
                    val = (N + 1) / M + (M / 2);
                }
            }
            else {
                yesProcess = N % M == 0;
                if (yesProcess) {
                    val = (N / M) + (M + 1) / 2;
                }
            }

            if (yesProcess) {
                System.out.println("With : " + M);
                result++;
            }
            M++;
        }

        return result;
    }

}

And just for the sake of completeness, here is an implementation using BigInteger that will be able to work correctly against your giant number. Be warned though - using BigInteger makes it more than 20 times slower than the long implementation. I used this BigSquareRoot class.

public class MainThree {
//    private static final BigInteger N = new BigInteger("916548345678631");
    private static final BigInteger N = new BigInteger("21432154197846387216432");

    private static final BigSquareRoot bsr = new BigSquareRoot();
    private static final BigInteger zero = new BigInteger("0");
    private static final BigInteger one = new BigInteger("1");
    private static final BigInteger two = new BigInteger("2");

    public static void main(String[] args) {

        long start = System.currentTimeMillis();
        BigInteger loopUntil = bsr.get(N.multiply(two)).toBigInteger();
        BigInteger M = one;

        int doProcess = doProcess(N, loopUntil, M);
        System.out.println(doProcess);

        long end = System.currentTimeMillis();
        System.out.println("[Main.main] elapsed time: " + (end - start));
    }

    public static int doProcess(BigInteger N, BigInteger loopUntil, BigInteger M) {
        int result = 0;

        while (M.compareTo(loopUntil) < 0) {

            boolean yesProcess = false;
            BigInteger val = zero;
            boolean mEven = M.mod(two).equals(zero);
            if (mEven) {
                yesProcess = N.mod(M).equals(M.divide(two));
                if (yesProcess) {
                    val = (N.add(one).divide(M)).add(M.divide(two));
                }
            }
            else {
                yesProcess = N.mod(M).equals(zero);
                if (yesProcess) {
                    val = N.divide(M).add(M.add(one).divide(two));
                }
            }

            if (yesProcess) {
                System.out.println("With : " + M);
                result++;
            }
            M = M.add(one);
        }

        return result;
    }
}


Replacing elements from a list and its sublists Write a procedure substitute that takes three arguments: a list, an old word, and a new word. It should return a copy of the list, but with every occurrence of the old word replaced by the new word, even in sublists. For example:
> (substitute ’((lead guitar) (bass guitar) (rhythm guitar) drums) ’guitar ’axe)
((lead axe) (bass axe) (rhythm axe) drums)

source
The argument "l" is the list that would be evaluated, I just don't know what name is appropriate because the word "list" is already a procedure. Subl will evaluate the items that are lists.
Please review my code.
(define (substitute l old new)
    (if (null? l) '()
        (let ((head (car l))
            (tail (cdr l))
            (subl (lambda (x) (substitute x old new))))
            (cond ((list? head) (cons (subl head) (subl tail)))
            ((equal? head old) (cons new (subl tail)))
            (else (cons head (subl tail)))))))

How is my use of let and lambda? Did I manage data abstraction correctly? How can I make this code better and faster?

Separate your concerns

Think of it this way - as we walk through the input, we do one of two things. If the next thing is a list, we recurse. If it's not a list, we do the equality checking. You're doing everything in one check, which makes it a little hard to follow.

Let's just define a maybe-swap:

(define (maybe-swap val old new)
    (if (equal? val old) new val))

And use that directly:

(define (substitute lst old new)
    (if (list? lst)
        (map (lambda (elem) (substitute elem old new)) lst)
        (maybe-swap lst old new)))

This does all the same thing that you're doing, but once you split it up, there's less logic to have to reason about. You can even move maybe-swap into substitute directly:

(define (substitute lst old new)
    (define (maybe-swap elem) (if (equal? elem old) new elem))
    (if (list? lst)
        (map (lambda (elem) (substitute elem old new)) lst)
        (maybe-swap lst)))

This also reduces the code duplication. You're currently recursing via (subl X) in four different places. I'm recursing in just one spot - effectively for lists I'm calling (map subl lst).

map

I think in a previous review you didn't want to use map, which we can easily implement like:

(define (map f lst)
    (if (null? lst) lst
        (cons (f (car lst)) (map f (cdr lst)))))


Plugin for finding placed blocks and if they are in a certain sequence This is for a plugin I am making for the game called Minecraft. The code is responsible for finding out the blocks placed by the player and if they are in the following sequence as shown in the picture then create an Antenna. block in block.getType() returns the block placed by the player. Also, it doesn't matter in what order the player places the blocks.

// if block placed was a jukebox and block directly above is a diamond block
// and the block above the diamond block is an iron fence
if(block.getType() == Material.JUKEBOX
        && block.getRelative(BlockFace.UP, 1).getType() == Material.DIAMOND_BLOCK
        && block.getRelative(BlockFace.UP, 2).getType() == Material.IRON_FENCE){
    System.out.println("Antenna Created");
    player.sendMessage("Antenna Created");
}

// if block placed was a diamond block and block directly above is a iron fence
// and the block below is a jukebox
if(block.getType() == Material.DIAMOND_BLOCK
        && block.getRelative(BlockFace.UP, 1).getType() == Material.IRON_FENCE
        && block.getRelative(BlockFace.DOWN, 1).getType() == Material.JUKEBOX){
    System.out.println("Antenna Created");
    player.sendMessage("Antenna Created");
}

// if block placed was an iron fence and the block directly below is a diamond block
// and the block below that is a jukebox
if(block.getType() == Material.IRON_FENCE
        && block.getRelative(BlockFace.DOWN, 1).getType() == Material.DIAMOND_BLOCK
        && block.getRelative(BlockFace.DOWN, 2).getType() == Material.JUKEBOX){
    System.out.println("Antenna Created");
    player.sendMessage("Antenna Created");
}

I am not happy with the following code and I get the feeling there is a better way to do this? I'm also wondering how would I go about designing these if Statements to allow for future extendibility?

As it is mentioned in the comments, It seems your code is exactly same as the below. So that is the first suggestion.

main = block.getType();
up1 = block.getRelative(BlockFace.UP, 1).getType();
up2 = block.getRelative(BlockFace.UP, 2).getType();
down1 = block.getRelative(BlockFace.DOWN, 1).getType();
down2 = block.getRelative(BlockFace.DOWN, 2).getType();

if ((main == Material.JUKEBOX && up1 == Material.DIAMOND_BLOCK && up2 == Material.IRON_FENCE)||
   (main == Material.DIAMOND_BLOCK && up1 == Material.IRON_FENCE && down1 == Material.JUKEBOX) ||
   (main == Material.IRON_FENCE && down1 == Material.DIAMOND_BLOCK && down2 == Material.JUKEBOX)) {
    System.out.println("Antenna Created");
    player.sendMessage("Antenna Created");
}

Now, as far as extendibility goes, You need to add newer conditions, it is not a good idea. What you have coded here is just an FSM (A state machine).

The easiest method to capture a state machine is to turn that into a regular expression. In this case, Say, the ordering is

up1 up2 main down1 down2

The conditions are

DIJ.. | I.DJ. | ..IDJ

So this captures your state machine fully. Adding future conditions is just as simple as adding to the above regular expression.

The code would look like

first(var) {
  return first_char(var)
}
string = { first(up1.toS()), first(up2.toS()),
           first(main.toS()),
           first(down1.toS()), first(down2.toS())};
re = new Regexp('DIJ..|I.DJ.|..IDJ')
if (re.match(string)) {
  player.send('Antena Created');
}


Trying to write an equals method for a class containing array of objects Im trying to write a College class equals method that compare the content of the arrays of the two objects of college class. So The method will compare the Student[] array in college object to other Student[] array in another college object and same for the teacher[] array. I have written an equals method for this, but it looks too long and not pretty. Is there a better way to write it?

College Class

public class College
{
  private Student[] student;
  private Teacher[] teacher;
  public College()
  {
    student = new Student[9];
    teacher = new Teacher[9];
  }

  public Student[] getStudent()
  {
    return student;
  }

  public void setStudent(Student[] student)
  {
    this.student = student;
  }

  public Teacher[] getTeacher()
  {
    return teacher;
  }

  public void setTeacher(Teacher[] teacher)
  {
    this.teacher = teacher;
  }

  public boolean equals(Object inObj)
  {// this equals method

    boolean isEqual = false;
    College inCollege = (College)inObj;

    if(student.length == inCollege.getStudent().length)
    {
      for(int i = 0; i < student.length; i++)
      {
        if(student[i].equals(inCollege.student[i]))
        {
            isEqual = true;
        }
      }
    }
    if((teacher.length == inCollege.getTeacher().length) && isEqual == true)
    {
      isEqual = false;
      for(int i = 0; i < inCollege.getTeacher().length; i++ )
      {
        if(teacher[i].equals(inCollege.teacher[i]))
        { System.out.println("im in");
          isEqual = true;
        }
      }
    }
    return isEqual;
  }
}

Student Class

public class Student
{
  private String name;
  private int age;

  public Student()
  {
    name = "Samrah";
    age = 19;
  }

  public Student(String name, int age)
  {
      this.name = name;
      this.age = age;
  }
  public String getName()
  {
    return name;
  }

  public void setName(String name)
  {
    this.name = name;
  }

  public int getAge()
  {
    return age;
  }

  public void setAge(int age)
  {
    this.age = age;
  }

  public boolean equals(Object inObj)
  {
    boolean isEqual = false;
    if(inObj instanceof Student)
    {
        Student inStudent = (Student)inObj;
        if(this.name.equals(inStudent.getName()))
          if(this.age == inStudent.getAge())
              isEqual = true;

    }
    return isEqual;
  }

  public String toString()
  {
    String str;

    return str = "name: "+name+" age: "+age;
   }

}

Teacher Class

public class Teacher
{
  private String name;
  private int age;

  public Teacher()
  {
    name = "Sanjay";
    age = 45;
  }

  public Teacher(String name, int age)
  {
    this.name = name;
    this.age = age;
  }

   public void setName(String name)
  {
    this.name = name;
  }

  public String getName()
  {
    return name;
  }

  public int getAge()
  {
    return age;
  }

  public void setAge(int age)
  {
    this.age = age;
  }

  public boolean equals(Object inObj)
  {
    boolean isEqual = false;
    if(inObj instanceof Teacher)
    {
       Teacher inTeacher = (Teacher)inObj;
        if(this.name.equals(inTeacher.getName()))
          if(this.age == inTeacher.getAge())
              isEqual = true;

    }
    return isEqual;
  }

  public String toString()
  {
    String str;

    return str = "name: "+name+" age: "+age;
   }
}

Trying to do something like this:

    boolean isEqual = false;

    if(inObject instanceof EngineClass)
    {
        EngineClass inEngine = (EngineClass)inObject;
        if(cylinders == inEngine.getCylinders())
        if(fuel.equals(inEngine.getFuel()))
            isEqual = true;
    }
    return isEqual;

but with since its an array I have to loop through the arrays so I'm really confused.

It depends on what is exactly possible in your class - can you override class, can some fields be null at any point? For example retrofit creates classes using reflection and some classes can be very unexpectedly null unless set in constructor without parameters.
Many IDEs have generators, they do this really well.
If you override equals, you really should override hashCode too:
https://stackoverflow.com/questions/2265503/why-do-i-need-to-override-the-equals-and-hashcode-methods-in-java

I included equals generated by Android Studio - 2 possibilities, depending on if you are okay with subclasses too (hashcodes both same).

    @Override
    public boolean equalsV1(Object o) {
        if (this == o) return true;
        if (o == null || getClass() != o.getClass()) return false;
        College college = (College) o;
        return Arrays.equals(student, college.student) &&
                Arrays.equals(teacher, college.teacher);
    }

    @Override
    public boolean equalsV2(Object o) {
        if (this == o) return true;
        if (!(o instanceof College)) return false;
        College college = (College) o;
        return Arrays.equals(student, college.student) &&
                Arrays.equals(teacher, college.teacher);
    }

    @Override
    public int hashCode() {
        int result = Arrays.hashCode(student);
        result = 31 * result + Arrays.hashCode(teacher);
        return result;
    }

You can notice, that there are static methods for arrays in Array class.
Code is trying to exit as soon as it is sure, that equality isn't there rather than your approach, where code runs till the end and maybe it will set equals to true along the way, which has to run longer.
You don't need to use getters, you can use fields directly since it's accessing fields of different instance, but same class.
In College class it's fine, but in Student your class would crash if name is null. It shouldn't happen as even if your empty constructor you set it to something. But sadly in java I can still create Student class and pass null myself (would be good to do null check there). So just to be safe, equals should be able to handle that. There's method for that to make it easier - Objects.equals(name, other.name) rather than name.equals(other.name). First one will work fine and return false when name is null. Second one will raise NullPointerException.
equals shouldn't ever throw Exception. If you aren't sure to guarantee that, it might be better to just surround code in try-catch and return false on exception.
Unless it's exercise, try to rely on these generators. They are pretty good and worked well for me.
Now I switched to kotlin, that has data classes, which automatically have equals and hashCode based on all class properties already saves work and lines of code!


Reordering a list from the ends towards the middle let  shuffley (numbers:int list) =
    let rec loop numbers acc =
        match numbers with
        | head::tail -> loop (List.rev(tail)) (head::acc)
        | [] -> List.rev(acc)
    loop numbers []
shuffley [1;2;3;4;5;6;7;8]

I am trying to practice some F# and I was wondering if could be a good example of tail recursion or this is just some nonsense.

As this algorithm is tail recursive lets tackle another problem: having recursion instead of loops often makes it easier to reason about a single step but more difficult to see whether one step is (partially) undoing effects of the previous one.

Here, the remaining numbers are reversed in every step. This is an O(n) operation (n = length of the list), doing this in every step means the overall complexity is O(n2).

It seems the result could also be computed by

splitting the list into a, b
interleaving a and the reversed b

For splitting in linear time, there is a 'trick': moving two 'pointers' while keeping track of the visited elements (acc). The second pointer (ys) always skips two elements while the first one (xs) skips just one. The function needs to be called with (input, input), i.e. both pointers at the first element:

let rec split acc =
    function
    | x :: xs, _ :: _ :: ys -> split (x :: acc) (xs, ys)
    | x :: xs, [_] -> split (x :: acc) (xs, [])
    | xs, _ -> List.rev acc, xs

Interleaving is straightforward:

let rec interleave acc =
    function
    | h :: t, o -> interleave (h :: acc) (o, t)
    | [], h :: t -> interleave (h :: acc) (t, [])
    | [], [] -> List.rev acc

both functions are of linear complexity, combining them sequentially is O(n) still:

let shuffley' list =
    let rec interleave acc =
        function
        | h :: t, o -> interleave (h :: acc) (o, t)
        | [], h :: t -> interleave (h :: acc) (t, [])
        | [], [] -> List.rev acc

    let rec split acc =
        function
        | x :: xs, _ :: _ :: ys -> split (x :: acc) (xs, ys)
        | x :: xs, [_] -> split (x :: acc) (xs, [])
        | xs, _ -> List.rev acc, xs

    let a, b = split [] (list, list)
    interleave [] (a, List.rev b)

On my machine, shuffley [0..10000] (10k) takes about the same time as shuffley' [0..1000000] (1M) (less than a second), whereas shuffley [0..100000] (100k) doesn't even terminate within a minute.

Note that we can now also shuffley' any list, not just ints (due to the removed type annotations). If you want to pass an empty list in F# interactive, just use shuffley' ([] : int list).

I reversed the accumulator and 'real' parameters to make use of the concise functionality.


Reading files with code value pairs I have a text file with about a hundred code value pairs [$code, Value].

For example:

Sample file

$codeA
valueOfA
$codeB
valueofB
.
.
.

These values need to be initiated into textFields, radioButtons, checkBoxes, etc over "several panels" contained within a main Controller panel. I switch between panels through a simple card layout.

I read through the lines and use if/else statements to get values for the fields, which isn't efficient. I was wondering if there is a better approach, a more elegant approach maybe.

    /*
 * read File
 */
private void readFile() {
    BufferedReader br = null;
    try {
        br = new BufferedReader(new FileReader(fileName));
        String line = null;
        while ((line = br.readLine()) != null) {
            if (line.equals("$codeA")) {
                panel1.txtFieldA.setText(br.readLine()));
            } else if (line.equals("$codeB")) {
                panel2.txtFieldB.setText(br.readLine()));
            }
            //.....
            //.....
        }
    } catch (IOException e) {
        e.printStackTrace();
    } finally {
        if (br != null) {
            try {
                br.close();
            } catch (IOException e) {
                e.printStackTrace();
            }
        }
    }
}

Make a Map like this

Map<String, JTextField> map = new HashMap<>();
map.put("$codeA", panel1.txtFieldA);
map.put("$codeB", panel2.txtFieldB);

and change your loop like this

String code = null;
while ((code = br.readLine()) != null) {
    JTextField jtf = map.get(code);
    if (jtf != null)
        jtf.setText(br.readLine());
    else
        br.readLine();
}


Renaming PDF files based on given rules in a text file I'm learing Python by writing a program to solve some housekeeping I do weekly, namely renaming PDF files according to rules given in a text file.

My end goal is to have idiomatic Python, with full test coverage. I have done some refactoring, and written a couple of tests. Ideally I would have done TDD from the beginning, but I find it challenging to write tests since I don't know Python that well yet.

The PDF documents can be named with two patterns:

xx.pdf
xx-y.pdf

They are stored in the folder ./test relative to the script root.

The text file Instruments.txt contains the following pattern:

01 SopranoCn
02 SoloCn
03 RepianoCn
...
18 Percussion
20 AllParts
...

The whole purpose is to rename files like this:

xx.pdf   (01.pdf)   --> xx_{instrument_name}.pdf (01_SopranoCn.pdf)
xx-y.pdf (02-1.pdf) --> xx_{instrument_name}{part_number}.pdf (02_SoloCn1.pdf)

The y-argument is optional, and can be added to any instrument.

Here's the whole source:

"""
Invoke with "python notesort.py <path to folder>".
A list of instrument numbers with corresponding instrument names
must be available in the root folder of the script.
"""

# Standard library imports
import os

FOLDER = './test'

def create_instrument_map():
    """Create a dictionary mapping each prefix
    to the corresponding instrument name."""
    with open('Instruments.txt') as instrument_file:
        return {
            line.split()[0]: line.split()[1] for line in instrument_file
        }

def rename_files_with_instrument_name(instrument_map):
    """Rename PDF files in folder based on the root of
    the original file name."""
    for filename in os.listdir(FOLDER):
        root, extension = os.path.splitext(filename)
        if not extension.endswith('.pdf'):
            continue
        try:
            if root[2]:
                if root[:2] in instrument_map:
                    new_file_name = '{prefix}_'\
                                    '{instrument_name}'\
                                    '{part_number}'\
                                    '{extension}'.format(
                                        prefix=root[:2],
                                        instrument_name=instrument_map[root[:2]],
                                        part_number=root[3],
                                        extension=extension)
        except:
            if root in instrument_map:
                new_file_name = '{prefix}_{instrument_name}{extension}'.format(
                                    prefix=root,
                                    instrument_name=instrument_map[root],
                                    extension=extension)
        old_path = os.path.join(FOLDER, filename)
        new_path = os.path.join(FOLDER, new_file_name)
        os.rename(old_path, new_path)
        print 'Renamed', filename, '-->', new_file_name

if __name__ == '__main__':
    instrument_map = create_instrument_map()
    rename_files_with_instrument_name(instrument_map)

I have started to write some tests for it, but I find it difficult to test rename_files_with_instrument_name. I do not have any experience writing tests, but I see the benefit and want to practise writing them.

These are the tests I have so far:

import unittest
from notesort import create_instrument_map

class TestNoteSort(unittest.TestCase):

    def test_create_instrument_map_creates_soprano_cornet(self):
        instrument_map = create_instrument_map()
        self.assertEqual(instrument_map['01'], 'SopranoCn')

    def test_create_instrument_map_soprano_not_solo(self):
        instrument_map = create_instrument_map()
        self.assertNotEqual(instrument_map['01'], 'SoloCn')

if __name__ == '__main__':
    unittest.main()

Please feel free to make comments on any part of my implementation. No need to be easy on me, even if I'm a beginner.

I am especially interested in how I can get further with testing, and also how to refactor rename_files_with_instrument_name.

For now I'll just throw up a few random thoughts.

The first thing I would do with rename_files_with_instrument_name is get rid of its reliance on the global FOLDER variable. Take a directory path as an argument, or shove it in a class and make the folder a parameter to the class's __init__ method which gets stored in an instance variable. That should immediately make it easier to write tests for rename_files_with_instrument_name; just pass in a folder and check that you get the results you expect.

The second thing I would do with rename_files_with_instrument_name is try and shorten the name a little. It's very long. Something like add_instrument can be good enough in context, but even rename_with_instrument is an improvement.

Going inside rename_files_with_instrument_name, using bald except is usually a bad practice. I gave an example case where bald except is bad in my answer to another question. If you think an exception might be thrown, it's better to name the specific exception. Also, make sure the situation where the exception gets raised is actually exceptional (an error or some situation which some code outside the function should handle). It looks a little like you're handling a normal, expected case in your except block—the case where the root filename is shorter than three characters. It's better to handle those with if. Not only is it easier to understand, it's also actually faster; catching exceptions is fairly time-consuming in most languages.

However, you don't actually need a conditional here; if you try to take a slice of a string which is longer than the entire string, the entire string just gets returned. That is:

>>> s = "ab"
>>> s[:30]
"ab"

So you can just write, without the try block, if root[:2] in instrument_map.

It's also usually good to avoid continue if you can. Sometimes you can't, and in those cases, don't contort yourself. But I think you can here. It looks like you could just write:

if extension.endswith(".pdf"):
    # All the code to construct a new filename


Method that filters a list parameters based on different fields I have this code which is a callback for Filter in WPF ICollectionView. When I run code analysis, the cyclometic complexity is reported to be 11. I consider this line by line comparison gives a better readability. How can I improve the cyclomatic complexity or improve this code in general?

 private bool FilterLicenseList(object item)
 {
     var license = item as LicenseEntry;
     if (!string.IsNullOrEmpty(Filter.Firm) && !license.Firm.Contains(Filter.Firm)) return false;
     if (!string.IsNullOrEmpty(Filter.RegistrationCode) && !license.RegistrationCode.Contains(Filter.RegistrationCode)) return false;
     if (!string.IsNullOrEmpty(Filter.LicenseCode) && !license.LicenseCode.Contains(Filter.LicenseCode)) return false;
     if (!string.IsNullOrEmpty(Filter.Branch) && !license.Branch.Contains(Filter.Branch)) return false;
     if (Filter.Active != null && license.Active != Filter.Active) return false;
     return true;
  }

Your code has lots of duplication. You can make it more readable:

private bool FilterLicenseList(object item)
{
    var license = item as LicenseEntry;
    return FilterString(Filter.Firm, license.Firm)
        && FilterString(Filter.RegistrationCode, license.RegistrationCode)
        && FilterString(Filter.LicenseCode, license.LicenseCode)
        && FilterString(Filter.Branch, license.Branch)
        && FilterBoolean(Filter.Active, license.Active);
}

All you need to do now is to implement the two helper methods FilterString and FilterBoolean.

I know that even the rewritten code is visually redundant since the field name appears twice in each line of code, but removing this would be more difficult. Compared to the original code I find the rewritten code much more readable, and there is no chance that in one of the lines an exclamation mark is missing, which would be pretty easy to overlook.


Using Elixir pipeline to transform a string into a map Consider the list of string values,

lines = ["foo: 1", "bar: 2", "baz: 3"]

That one might transform into the Elixir map,

%{ "foo": "1", "bar": "2", "baz": "3"}

using a simple Enum.reduce,

Enum.reduce(lines, %{}, fn(line, result) ->
  [key, value] = String.split(line, ": ")
  Map.put(result, key, value)
end)

How might one refactor this into something more succinct, specifically with the third argument passed to reduce? For example, my instinct wants to do something like,

Enum.reduce(lines, %{}, fn(line, result) ->
  String.split(line, ": ") |> Map.put(result)
end)

but the Elixir pipe operator doesn't allow piping multiple values nor piping them to anything other than the first argument.

You can do this with an anonymous function

Enum.reduce(lines, %{}, fn(line, result) ->
  String.split(line, ": ")
  |> (fn(x) -> Map.put(result, hd(x), List.last(x)) end).()
end)

You can also do it like this

Enum.reduce(lines, [], fn(line, result) -> result ++ String.split(line, ": ") end)
  |> Enum.chunk(2)
  |> Map.new(fn [k,  v] -> {k, v} end)

which I less efficient, because it does 2 iterations, but I think it looks cleaner.


How to refactor the following sql statement? Declare @IsExistRoleAR As bit
Declare @IsExistRoleEN As bit
Declare @IsExist AS bit
set @IsExistRoleAR=(SELECT CASE WHEN COUNT(RoleID) > 0 THEN 1 ELSE 0 END AS isExists
                    FROM Roles
                    WHERE RoleDescAR='Arabic Name')

set @IsExistRoleEN=(SELECT CASE WHEN COUNT(RoleID) > 0 THEN 1 ELSE 0 END AS isExists
FROM Roles
WHERE RoleDescEN='English Name')

set @IsExist=(@IsExistRoleAR | @IsExistRoleEN)
select @IsExist

You could just use OR:

  SELECT CASE WHEN COUNT(RoleID) > 0 THEN 1 ELSE 0 END AS isExists
  FROM Roles
  WHERE RoleDescAR='Arabic Name' OR RoleDescEN='English Name'


Constructing a JavaScript slide I am a newbie to native JavaScript and am pretty much coming to grips with the operators, etc. At the moment I am currently constructing a slide.

"use strict";

 var intSeconds = 3;

 function loadSlider(){

flickrImgsShown = 0;
sliderImages = new Array(slides + 10);
sliderImages[0] = document.querySelector(".img");

    for (var i = 1; 1 < sliderImg; i++ ) {
        sliderImages[i] = document.querySelector(".img" + (i + 1));
        document.querySelector(".identifer" + i);
    }

    function next() {

        flickrImgsShown++;
        if(flickrImgsShown == slides)
            flickrImgsShown = 0;
        transition();

        clearInterval(slideTimer);
        slideTimer = setInterval(timer, interSeconds * 1000);
    }

    function prev () {

        flickrImgsShown--;
        if(flickrImgsShown == -1)
            flickrImgsShown = slides -1;
        transition();

        clearInterval(slideTimer);
        slideTimer = setInterval(timer, interSeconds * 1000);
    }

    slideTimer = setInterval(timer, intSeconds * 1000);

    function timer() {

        flickrImgsShown--;
        if(flickrImgsShown == -1)
            flickrImgsShown = slides -1;
        transition();
    }

}

I am not a fan of the getelementbyidand style.visibility in my code. Is there a better method I can apply to call out an ID or change a style in the DOM?

Your questions seem to have been answered (use jQuery), so here are a couple of small points regarding readability:

be consistent with your variable names (eg sliderImg vs flickrImgs: one is plural, one singular, and in the rest of the code, you mainly use flickrX, except for the sliderImg one; I would prefer sliderX everywhere, because it doesn't seem to matter where the images come from).
some variable names could be a lot better. Eg when reading flickrImgs/sliderImg I'm thinking it's an array of images (or image paths/names). But they are not, they are integers, so I would prefer imageCount/imageSize, and currentSliderPosition. flickrDisplay on the other hand seems to actually be a collection of images, so sliderImages would probably be a good name for it.
your indentation is off, which decreases the readability of your code.
your comments aren't all that helpful. NOW AND NEXT FUNCTION/SLIDE TRANSITION aren't really necessary, and For each loop incrementing each slide by one doesn't really help either (because that's exactly what the code already told me; If you do want to add a comment, it would be more helpful to explain what it means when a slide is increased.
use curly brackets even for one line statements to increase readability and to possibly avoid bugs.
visiable isn't a valid value for visibility, you probably meant visible.


Hybrid Merge/Insertion sort algorithm Explanation: Although merge sort runs in Ω(nlgn) and insertion sort runs in Ω(n^2), the constant factors in insertion sort can make it faster in implementation for small problem sizes.  This sorting implementation should still be stable.
Recursive Merge sort subroutine method:
private static void recursiveMergeSort(double[] arr, int lowerBound, int upperBound) {
    if (lowerBound < upperBound) {
        // Split the sub array into halves
        int mid = lowerBound + (upperBound - lowerBound) / 2;
        recursiveMergeSort(arr, lowerBound, mid);
        recursiveMergeSort(arr, mid + 1, upperBound);
        merge(arr, lowerBound, mid, upperBound);
    }
}

Merge method: *note-
I would like to replace the while loop with for and if-else statements.
private static void merge(double[] arr, int left, int mid, int right) {
    int i = 0, j = 0, k = left;
    //System.out.println("used merge");

    // Sizes of the temporary arrays to be copied
    int n1 = (mid - left) + 1;
    int n2 = (right - mid);

    // Create temporary arrays and copy data
    double[] leftTemp = Arrays.copyOfRange(arr, left, mid + 1);
    double[] rightTemp = Arrays.copyOfRange(arr, mid + 1, right + 1);

    // Merge the temp arrays back into arr[left...right]
    while (i < n1 && j < n2) {
        if (leftTemp[i] <= rightTemp[j]) {
            arr[k++] = leftTemp[i++];
        } else {
            arr[k++] = rightTemp[j++];
        }
    }

    // Copy remaining elements, if any
    while (i < n1) {
        arr[k++] = leftTemp[i++];
    }
    while (j < n2) {
        arr[k++] = rightTemp[j++];
    }
}

Insertion sort subroutine method:
private static void insertionSort(double[] arr, int left, int right){
    for (int i = left + 1; i <= right; i++) {
        double key = arr[i];
        int j = i - 1;

        while (j >= left && arr[j] > key) {
            arr[j + 1] = arr[j];
            j--;
        }
        arr[j + 1] = key;
    }
}

Hybrid merge/insertion sorting method:
Optimized is a value that is best set between [25,100]
private static void insertionRecursiveMergeSort(double[] arr, int left, int right) {
    // If <= OPTIMIZED use insertion sort subroutine
    if (right <= left + OPTIMIZED - 1) {
        insertionSort(arr, left, right);
    } else {
        int mid = left + (right - left) / 2;
        insertionRecursiveMergeSort(arr, left, mid);
        insertionRecursiveMergeSort(arr, mid + 1, right);
        merge(arr, left, mid, right);
    }
}

For test runs, I used array sizes 1M, 2M, 3M, and 5M with optimized set to 25, 50, 100, and 125.

Optimization notes.

Allocate the temporary array once, and pass it down to the recursive invocations.

 Do not fall back to the insertion sort immediately. Postpone it until all the recursions complete, and insertion sort the entire array once.
It feels scary to insertion sort the entire array. Wouldn't it degrade performance to \$O(n^2)\$? The answer is no. The array is already "almost sorted": every element is at most OPTIMIZED away from its final destination. Therefore, an inner loop does at most OPTIMIZED iterations per element, and the overall complexity is \$O(n * \texttt{OPTIMIZED})\$. This observation also hints that OPTIMIZED should be in a ballpark of \$\log n\$.
Edit: I don't know what I was thinking. The above is totally wrong (it is valid for quicksort, but not here), except the observation that OPTIMIZED should be in a ballpark of \$\log n\$.

A little-known trick allows to cut the number of comparisons in the insertion sort in half. Instead of
  while (j >= left && arr[j] > key)

consider first comparing key to arr[left]:
  if (key < arr[left]) {
      // The key shall land at the left. No need to compare values anymore
      for (j = i; j > 0; j--) {
          arr[j] = arr[j-1];
      }
  } else {
      // arr[left] is a natural sentinel. No need to compare indices anymore
      for (j = i; arr[j] > key; j--) {
          arr[j] = arr[j-1];
      }
  }

Now if you opt for a postponed insertion sort, it could be optimized even more: find the min among first OPTIMIZED elements and swap it with arr[0]. Now there is no need to test for key < arr[left] - it is guaranteed to fail. Go straight into the unguarded loop.


Idiomatic way to write proper F# (x)unit + fsunit tests? I am writing some unit tests using XUnit in F# and I am wondering what is the most idiomatic way in the F# sense to write them.

Let's start a simple case:

BicValidationTests.fs:

module Rm.Bai.Domain.BicValidationTests
open Rm.Bai.Domain.BicValidation
open FsUnit
open Xunit

let shortValidBic    = "MYMB gb 2L"
let expectedShortBic = "MYMBGB2L"
let ok               = Result<string, ValidationError>.Ok
let error            = Result<string, ValidationError>.Error

[<Fact>]
let ``Short Bic should be valid when all rules are respected`` () =
    shortValidBic
    |> validateAndFormat
    |> should equal (ok expectedShortBic)

One thing I am not too sure is whether it is better to include directly the values knowing that there is nothing shared among different test functions right into the test functions, such as:

[<Fact>]
let ``Short Bic should be valid when all rules are respected`` () =
    "MYMB gb 2L"
    |> validateAndFormat
    |> should equal (ok "MYMBGB2L")

This seems to be even more so the case since the answer I got here
which seems to indicate that if you have several test files, you need to rely on declaring types / classes to contain let variables for things other than primitive types like number or strings.

For example:

let sepaCompliantBics = [
    "DSXLAD46";
    "KAOLADZOAQC";
    "GFDIATGN";
    "XHOCATENR1X";
    "IMITBEW1";
    "JYPPBEPW807";
    "AHODBGG2";
    "LTQJBGMRKKA";
    "LAHYHRDK";
    ...
]

[<Fact>]
let ``Bics should be valid with SEPA-compliant countries`` () =
    sepaCompliantBics
    |> List.map(fun bic -> (validateAndFormat bic, bic))
    |> List.iter(fun (validation, bic) -> validation |> should equal (ok bic))

sepaCompliantBics in the example above will be set to null if it happens to not be the first file in the test project.

The solutions are then:

type Tests() =
    let ok               = Result<string, ValidationError>.Ok
    let error            = Result<string, ValidationError>.Error

    let sepaCompliantBics = [
        "DSXLAD46";
        "KAOLADZOAQC";
        "GFDIATGN";
        "XHOCATENR1X";
        "IMITBEW1";
        "JYPPBEPW807";
        "AHODBGG2";
        "LTQJBGMRKKA";
        "LAHYHRDK";
        ...
    ]

    [<Fact>]
    let ``Bics should be valid with SEPA-compliant countries`` () =
        sepaCompliantBics
        |> List.map(fun bic -> (validateAndFormat bic, bic))
        |> List.iter(fun (validation, bic) -> validation |> should equal (ok bic))

or:

let getSepaCompliantBics() = [
        "DSXLAD46";
        "KAOLADZOAQC";
        "GFDIATGN";
        "XHOCATENR1X";
        "IMITBEW1";
        "JYPPBEPW807";
        "AHODBGG2";
        "LTQJBGMRKKA";
        "LAHYHRDK";
        ...
    ]

[<Fact>]
let ``Bics should be valid with SEPA-compliant countries`` () =
    getSepaCompliantBics()
    |> List.map(fun bic -> (validateAndFormat bic, bic))
    |> List.iter(fun (validation, bic) -> validation |> should equal (ok bic))

or again:

[<Fact>]
let ``Bics should be valid with SEPA-compliant countries`` () =
    [
        "DSXLAD46";
        "KAOLADZOAQC";
        "GFDIATGN";
        "XHOCATENR1X";
        "IMITBEW1";
        "JYPPBEPW807";
        "AHODBGG2";
        "LTQJBGMRKKA";
        "LAHYHRDK";
        ...
        // this can be a pretty long list btw
    ]
    |> List.map(fun bic -> (validateAndFormat bic, bic))
    |> List.iter(fun (validation, bic) -> validation |> should equal (ok bic))

So I am wondering if having a long list like the example above is problem if it is defined in the test function itself rather than said outside? Is it still an idiomatic way to write unit tests with F#?

I think that regardless of a language you're using the most idiomatic way would be to take advantage of InlineData attribute

It would look roughly like this

[<Theory>]
[<InlineData("DSXLAD46", "expectedbic1")>]
[<InlineData("KAOLADZOAQC", "expectedbic2")>]
let ``Bics should be valid with SEPA-compliant countries`` input output =
    let result = validateAndFormat input
     match result with
        | Ok s ->
            Assert.Equal(s, output)
        | Error _ -> Assert.True(false) //looks like there is no Assert.Fail() in xunit


Python + spark to parse and save logs I need to parse logs and have got following code. I can see two problems: map().filter() may induce some performance penalties and copy-paste block

parser.py:

class Info(object):
    a = ""
    j = ""
    z = ""
    infoline = ""

    def __init__(self, a, j, z, infoline):
        self.a = a
        self.j = j
        self.z = z
        self.infoline = infoline

# function check if the line parameter is produced by provider 'prov'
# yes - returns certain substring of line
# no - returns None
def get_infoline(line, prov):
   ...

def process(line, prov):
    retA = None
    retJ = None
    retZ = None

    infoline = get_infoline(line, prov)line param from
    if infoline:
        # filling some of retA, retJ, retZ
        ...
        return Info(retA, retJ, retZ, infoline)

job.py:

from pyspark import SparkContext
import parser

...

prov = ...
log = sc.textFile(pathTofile)
parsed = log.map(lambda ln: parser.proccess(ln, prov)).filter(lambda i: i)

summaryA = parsed.map(lambda info: (info.a, 1)).reduceByKey(add) \
    .map(lambda (a,b): (b,a)).sortByKey(False) \
    .map(lambda (count, name): ("%s\t%i" % (name, count))) \
    .saveAsTextFile('/output/path/a.tsv')

summaryJ = parsed.map(lambda info: (info.j, 1)).reduceByKey(add) \
    .map(lambda (a,b): (b,a)).sortByKey(False) \
    .map(lambda (count, name): ("%s\t%i" % (name, count))) \
    .saveAsTextFile('/output/path/j.tsv')

summaryZ = parsed.map(lambda info: (info.z, 1)).reduceByKey(add) \
    .map(lambda (a,b): (b,a)).sortByKey(False) \
    .map(lambda (count, name): ("%s\t%i" % (name, count))) \
    .saveAsTextFile('/output/path/z.tsv')

With your first file, parser.py:

Because the Info class' __init__ method requires the three arguments: a, j, and z, which it then attributes to itself, you can remove the class attributes of the same name, as they become superfluous.
Within the process method, you set several variables before doing anything; retA, retJ, and retZ, which you set to None. If you aren't modifying these objects in place, you can omit these completely and add an else statement.

if infoline:
    # filling some of retA, retJ, retZ
    return Info(retA, retJ, retZ)
else:
    return Info(None, None, None)
In the aforementioned if statement, you can simply use

if get_infoline(line, prov):

Rather than setting the infoline variable. Then, if you need the result, you can define it within the statement.


Square the sorted array using haskell I want inputs on how I can square the sorted Array more efficiently in haskell. I wrote a very naive algorithm to solve the problem. What I am doing is comparing the head and the last element of list on absolute value and whichever is max I square the element and add it to accumulator.

But I think it is very slow. Is there any data structure that I can use to make it more efficient.

import qualified Data.List as List

sqrarr:: [Int] -> [Int]
sqrarr xss = helper  xss []

helper::[Int] -> [Int] -> [Int]
helper  [] acc = acc
helper [x] acc = [(x^ 2)] ++ acc
helper (x:xss) acc = case abs x > abs (List.last xss) of
  False -> helper (x: (List.init xss)) (((List.last xss)^2): acc)
  True -> helper xss ((x ^ 2):acc)

The above code works and gives the desired input

sqrarr [-7, -4, 2,5,8]
[4,16,25,49,64]

I have found another solution

import Data.Array
sqrarr2::[Int] -> [Int]
sqrarr2 xss = helper2 0 (length xss -1)
                (listArray (0,(length xss -1)) xss)
                []

helper2::Int -> Int -> Array Int Int -> [Int] -> [Int]
helper2 l r arr acc = case l > r of
  True -> acc
  False -> case abs (arr!l) > abs (arr!r) of
    False -> helper2 l (r - 1) arr ((arr!r ^ 2):acc)
    True -> helper2 (l + 1) r arr ((arr!l ^ 2):acc)

This looks needlessly convoluted: there is a lot happening, all at once, on top of each other.

The way to think about this is: first square each number, then sort the resulting list. The way to transform each element of a list is map. The way to sort is, well, sort. The way to compose two functions (i.e. pass the result of one as argument to the other) is . (dot)

So:

sqrarr = sort . map sq
  where
    sq x = x * x


Optimizing/Cleaning up Python Script for Zipf's Law I have attached two scripts which calculate the most popular songs according to zipf's law from the following standard input:

6 3
100 one
50 two
10 three
30 four
5 five
10 six

The first line details the number of songs to be input followed by the number of songs to return. Each line that follows holds the play counts and respective song titles. In the case of a tie, the algorithm must give precedence to the song that appeared earlier on the album.

Both of my attempts to solve are currently not efficient enough. Any help in optimizing one or both of these would be greatly appreciated.

Attempt 1:

import sys
from operator import itemgetter

def main():

    line1 = sys.stdin.readline().split()
    x = int(line1[0])-1
    z = int(line1[1])

    data = ""
    for line in sys.stdin:
        data+=line
    data = data.split()

    while(x >= 0):
        data[2*x]= float(data[2*x]) * (x+1)
        x-=1

    answers = [(data[a+1], data[a]) for a in range(0,len(data),2)]
    answers.sort(key=itemgetter(1), reverse=True)
    p=0
    while(p<z):
        print answers[p][0]
        p+=1

main()

Attempt 2:

import sys
from operator import itemgetter

def main():

    line1 = sys.stdin.readline().split()
    x = int(line1[0])-1
    p = x
    z = int(line1[1])

    data = ""
    for line in sys.stdin:
        data+=line
    data = data.split()

    while(x >= 0):
        data[2*x]= float(data[2*x]) * (x+1)
        x-=1

    y=0
    list_of_lists = []
    while(y<=p):
        list_of_lists.append([data[2*y+1], data[2*y]])
        y+=1
    list_of_lists = sorted(list_of_lists, key=itemgetter(1), reverse=True)

    n=0
    while(n<z):
        print list_of_lists[n][0]
        n+=1

main()

Currently Cprofile reports are indicating that the first script is faster. Thanks!

I must confess that I do not understand what you're trying to achieve and how Zipf's law come into play !
But I've some suggestions about the code itself :

Use meaningful variable names.
Concatenating strings only to split the result afterward is terribly costly, for no advantage.
Instead, use meaningful data structures. You deal with a sequence of (count, title) pairs. Flattening that and playing with index parity is confusing.

So, here is my take on it :

import sys
from operator import itemgetter

def main(source = sys.stdin):

   nb_in, nb_out = (int(x) for x in source.readline().split())

   #(lazely) collect Count and Title.
   data = ( source.readline().split() for i in range(nb_in) )

   #multiply each count by rank.
   answers = [ (title, int(count)*(rank+1)) for (rank, (count, title)) in enumerate(data) ]
   answers.sort(key = itemgetter(1), reverse = True)

   for title, _ in answers[:nb_out]:
      print title

main()


Which is correct : Open one Connection for inserting List of objects or open connection for every single insertion? I have written the following method twice but I don't know which is better from performance perspective, code design and best practice.

  First:

public int Insert()
        {
            int affectedRow = -1;
            using (IfxConnection con = new IfxConnection(ConfigurationManager.ConnectionStrings["sh"].ToString()))
            {

                StringBuilder cmdTxt = new StringBuilder();
                cmdTxt.Append(" INSERT INTO shedule(day,short,name,depcode,studycode,batchnum) VALUES (?,?,?,?,?,?) ");
                using (var myIfxCmd = new IfxCommand(cmdTxt.ToString(), con))
                {
                    myIfxCmd.CommandType = CommandType.Text;
                    myIfxCmd.Parameters.Add("day", IfxType.Char);
                    myIfxCmd.Parameters.Add("short", IfxType.NVarChar);
                    myIfxCmd.Parameters.Add("name", IfxType.NVarChar);
                    myIfxCmd.Parameters.Add("depcode", IfxType.Integer);
                    myIfxCmd.Parameters.Add("studycode", IfxType.Integer);
                    myIfxCmd.Parameters.Add("batchnum", IfxType.Integer);

                    if (con.State == ConnectionState.Closed)
                    {
                        con.Open();
                    }

                    myIfxCmd.Parameters[0].Value = ((object)this.DayId) ?? DBNull.Value;
                    myIfxCmd.Parameters[1].Value = ((object)this.ShortName) ?? DBNull.Value;
                    myIfxCmd.Parameters[2].Value = ((object)this.Name) ?? DBNull.Value;
                    myIfxCmd.Parameters[3].Value = this.DepCode;
                    myIfxCmd.Parameters[4].Value = this.StudyCode;
                    myIfxCmd.Parameters[5].Value = this.BatchNum;

                    affectedRow = myIfxCmd.ExecuteNonQuery();
                }
            }
            return affectedRow;
        }

THEN :

   foreach (Day a in days)
    {
        affectedRow = a.Insert();
    }

  Second:

public int Insert(List<Day> days)
        {
            int affectedRow = -1;
            using (IfxConnection con = new IfxConnection(ConfigurationManager.ConnectionStrings["sh"].ToString()))
            {

                StringBuilder cmdTxt = new StringBuilder();
                cmdTxt.Append(" INSERT INTO shedule(day,short,name,depcode,studycode,batchnum) VALUES (?,?,?,?,?,?) ");
                using (var myIfxCmd = new IfxCommand(cmdTxt.ToString(), con))
                {
                    myIfxCmd.CommandType = CommandType.Text;
                    myIfxCmd.Parameters.Add("day", IfxType.Char);
                    myIfxCmd.Parameters.Add("short", IfxType.NVarChar);
                    myIfxCmd.Parameters.Add("name", IfxType.NVarChar);
                    myIfxCmd.Parameters.Add("depcode", IfxType.Integer);
                    myIfxCmd.Parameters.Add("studycode", IfxType.Integer);
                    myIfxCmd.Parameters.Add("batchnum", IfxType.Integer);

                    if (con.State == ConnectionState.Closed)
                    {
                        con.Open();
                    }
                    foreach (Day a in days)
                    {
                        myIfxCmd.Parameters[0].Value = ((object)a.DayId) ?? DBNull.Value;
                        myIfxCmd.Parameters[1].Value = ((object)a.ShortName) ?? DBNull.Value;
                        myIfxCmd.Parameters[2].Value = ((object)a.Name) ?? DBNull.Value;
                        myIfxCmd.Parameters[3].Value = a.DepCode;
                        myIfxCmd.Parameters[4].Value = a.StudyCode;
                        myIfxCmd.Parameters[5].Value = a.BatchNum;

                        affectedRow = myIfxCmd.ExecuteNonQuery();
                    }
                }

            }

            return affectedRow;
        }

In general, the best is to do all your inserts in one connection.

Open/Close a connection implies a penalty. Although it is mitigated by some connection pooling mechanism on the database side (btw what is it ?), it is additional work that is not useful nor required.

The reason is that there is the possibility to use a Transaction to ensure that "all or nothing" is inserted, regarding whether things goes good or bad in your insertion loop, and in most cases a transaction is tied to a connection.

Here is something that includes a transaction. That'the way I am doing it in SQLServer. I assumed that the Informix transaction are correctly implemented using the ADO.NET scheme, and I do not know is this implementation is working.

public int Insert(List<Day> days)
{
    int affectedRow = -1;
    using (IfxConnection con = new IfxConnection(ConfigurationManager.ConnectionStrings["sh"].ToString()))
    {
        con.Open();
        using(var transaction = con.BeginTransaction())
        {
            var cmdTxt = " INSERT INTO shedule(day,short,name,depcode,studycode,batchnum) VALUES (?,?,?,?,?,?) ";
            using (var myIfxCmd = new IfxCommand(cmdTxt, con,transaction ))
            {
                myIfxCmd.CommandType = CommandType.Text;
                myIfxCmd.Parameters.Add("day", IfxType.Char);
                myIfxCmd.Parameters.Add("short", IfxType.NVarChar);
                myIfxCmd.Parameters.Add("name", IfxType.NVarChar);
                myIfxCmd.Parameters.Add("depcode", IfxType.Integer);
                myIfxCmd.Parameters.Add("studycode", IfxType.Integer);
                myIfxCmd.Parameters.Add("batchnum", IfxType.Integer);

                foreach (Day a in days)
                {
                    myIfxCmd.Parameters[0].Value = ((object)a.DayId) ?? DBNull.Value;
                    myIfxCmd.Parameters[1].Value = ((object)a.ShortName) ?? DBNull.Value;
                    myIfxCmd.Parameters[2].Value = ((object)a.Name) ?? DBNull.Value;
                    myIfxCmd.Parameters[3].Value = a.DepCode;
                    myIfxCmd.Parameters[4].Value = a.StudyCode;
                    myIfxCmd.Parameters[5].Value = a.BatchNum;

                    affectedRow = myIfxCmd.ExecuteNonQuery();
                }
            }

            // commit the changes in the database.
            // if an exception occurs somewhere, then the using blocks
            // makes that the transaction is disposed and rolled back,
            // and nothing will be updated in the database
            transaction.Commit();
        }
    }

    return affectedRow;
}


Bouncing ball simulation I was fooling around with some maths and realized that I could use a parabola to represent a bouncing ball. So, naturally, I decided to make it in code.

var Ball = function(r, v, a, y, x) {
    this.r = r;
    this.v = v;
    this.a = a;
    this.y = y;
    this.x = x;
};

Ball.prototype.draw = function() {
    this.y = this.v*this.r*this.r + this.a*this.r;
    var NewBallY = 380 - this.y;
    ellipse(this.x, NewBallY, 20, 20);
    if(this.y < 0) {
        this.r = 0;}
    this.r += 2;
};

var Ball1 = new Ball(0, random(-0.05, -0.02), random(2.0, 6.0), 40, random(20, 380));
var Ball2 = new Ball(0, random(-0.05, -0.02), random(2.0, 6.0), 40, random(20, 380));
var Ball3 = new Ball(0, random(-0.05, -0.02), random(2.0, 6.0), 40, random(20, 380));

var draw = function() {
    background(255, 255, 255);
    Ball1.draw();
    Ball2.draw();
    Ball3.draw();
};

The equation I used to calculate the parabola representing a ball's \$y\$ position is here, where \$y\$ is the y position of the ball, \$V\$ is the velocity, \$A\$ is the acceleration, and \$r\$ is the amount of time passed since the start of the bounce:

$$y=Vr^{2}+Ar$$

Using this.r to represent elapsed time is highly unconventional.  I would have guessed that r stood for "radius".  Why not use this.t like a sane programmer?

The draw() function ends with this.r += 2;, which is odd.  Why should the drawing routine manage the passage of time?  I would expect the time to be managed either in reference to the wall clock or to be passed in as a parameter by whatever code calls draw().

These don't look like parabolas to me.  At best, they would be degenerate ones, since this.x never changes.

The placement of the closing brace here:

if(this.y < 0) {
    this.r = 0;}

is weird.  Conventional formatting would be:

if (this.y < 0) {
    this.r = 0;
}


An easier way to test a valid username I have the following requirement for valid usernames:

Only alphanumeric characters and -
Must not start with a -

I wrote this regex (on Rubular):

/^[^a-zA-Z0-9]|[^a-zA-Z0-9-]+/

Is there a better to test this requirement other than to iterate through all the possible scenarios? I'm doing something like this.

  # There much more, but for brevity's sake I'm only including a few
  illegal_chars = [ "~", "!", "@", "#", "$"]

  illegal_chars.each do |char|
    test "username does not accept #{char}" do
      user = users(:homer)
      user.username = "name#{char}"
      refute user.valid?, "Username should not be valid"
    end
  end

I'll repeat for cases where #{char} is at the beginning and end of the word.

Am I testing too much? Is there a better way? Running 70 or 80 tests for something like this seems excessive.
As an aside, can my regex be expressed better?

Your regex looks wrong, see my link below.

If your functional requirement is that usernames match a certain regex, then your tests should reflect that. It is wrong to test a bunch of edge cases that a generic regex catches.

See this. All you need in your test is to do:

username_regex = /^[a-z0-9][a-z0-9-]*$/i
passing_usernames = ['passing','pAsSing-PASS-'.....]
failing_usernames = ['-failing','failing#'....]
passing_usernames.each { |username| assert_match username_regex, username}
failing_usernames.each { |username| assert_no_match username_regex, username }

If you're getting your username from somewhere else then just plug that in the same way.


5 input based search form for Android I've been trying to look for a better way in which a user has up to 5 inputs in which they can search for results in a database.

I have 2 EditTexts in which the user can have alphanumeric values and 3 Spinners. As you can see in the code, it's badly designed in the way it consists of if or else if statements which is just horrible, but it works.

What would be the best way to avoid long if or else if statements and be able to pick and choose what you would like to search for in the database? I know there is the switch but does that not just take a value and return the result of something on the one case?

if(tag != "" && name != "" && breed != "Breed" && sex != "Sex"){
    cursor = cupboard().withDatabase(sqLiteDatabase).query(Cattle.class)
            .withSelection("tag LIKE ? AND name LIKE ? AND breed LIKE ? AND sex LIKE ?", tag, name, breed, sex)
            .getCursor();

    if(cursor.getCount() < 1){
        Toast.makeText(getContext(), "Sorry, No Cattle with those details!", Toast.LENGTH_LONG).show();
    }

} else if (tag == "" && name == "" && breed == "Breed"){
    cursor = cupboard().withDatabase(sqLiteDatabase).query(Cattle.class)
            .withSelection("sex LIKE ?", sex)
            .getCursor();
    if(cursor.getCount() < 1){
        Toast.makeText(getContext(), "Sorry, No Cattle with those details!", Toast.LENGTH_LONG).show();
    }

} else if (breed != "Breed"){
    cursor = cupboard().withDatabase(sqLiteDatabase).query(Cattle.class)
            .withSelection("breed LIKE ?", breed)
            .getCursor();
    if(cursor.getCount() < 1){
        Toast.makeText(getContext(), "Sorry, No Cattle with those details!", Toast.LENGTH_LONG).show();
    }

} else if (sex != "Sex"){
    cursor = cupboard().withDatabase(sqLiteDatabase).query(Cattle.class)
            .withSelection("sex LIKE ?", sex)
            .getCursor();
    if(cursor.getCount() < 1){
        Toast.makeText(getContext(), "Sorry, No Cattle with those details!", Toast.LENGTH_LONG).show();
    }

} else if (tag == "" && name == "" && sex == "Sex"){
    cursor = cupboard().withDatabase(sqLiteDatabase).query(Cattle.class)
            .withSelection("breed LIKE ?", breed)
            .getCursor();
    if(cursor.getCount() < 1){
        Toast.makeText(getContext(), "Sorry, No Cattle with those details!", Toast.LENGTH_LONG).show();
    }

}

Disclaimer: I'm a C# developer and I haven't written Java in ages.

First of all I'd move this to a separate method, with (String selection, String[] selectionArgs) as parameters:

cursor = cupboard().withDatabase(sqLiteDatabase).query(Cattle.class)
        .withSelection(selection, selectionArgs)
        .getCursor();

if(cursor.getCount() < 1){
    Toast.makeText(getContext(), "Sorry, No Cattle with those details!", Toast.LENGTH_LONG).show();

You can now call that method in each if instead of copy-pasting the same code over and over again and only changing a small part.

But I'm mostly baffled by the convoluted logic WRT those ifs and the resulting selection and selectionArgs. Are you sure you didn't mean to construct something like this (pseudo-code):

var selectionOptions = new Dictionary<String, String>();

if(breed != "" && breed != "Breed")
{
   selectionOptions.Add("breed LIKE ?", breed);
}

if(sex != "" && sex != "Sex")
{
   selectionOptions.Add("sex LIKE ?", sex);
}

// same for tag and name

// now construct the selection
var selection = string.Join(" AND ", selectionOptions.Keys);

// pass the selection and its arguments to the method I mentioned above
ThatMethodMentionedAbove(selection, selectionOptions.Values);

The logic being:

Look at each of the four possible parameters
Only add a parameter to the query if it has a value and its value is not its own name

That would make sense to me, whereas this kind of logic:

if (tag == "" && name == "" && breed == "Breed"){
    // snip
    .withSelection("sex LIKE ?", sex)

... just looks... odd. And indeed, very un-maintainable.


Verify application configuration from JSON file with matching type using TypeScript in an Angular project To bootstrap our application we have a configuration stored in a JSON file. This config holds different settings, like definition of endpoints (e. g. an API or SMS gateway etc.) or some default values for login.
endpoint.ts
export const isEndpoint = (candidate: any): candidate is Endpoint => {
  return (
    candidate && typeof candidate === 'object' &&
    candidate.hasOwnProperty('protocol') && (candidate.protocol === null || ['http', 'https'].includes(candidate.protocol)) &&
    candidate.hasOwnProperty('address') && (candidate.address === null || typeof candidate.address === 'string') &&
    candidate.hasOwnProperty('port') && (candidate.port === null || typeof candidate.port === 'number') &&
    candidate.hasOwnProperty('path') && (candidate.path === null || typeof candidate.path === 'string')
  );
}

export interface Endpoint {
  protocol: 'http'|'https'|null,
  address: string|null,
  port: number|null,
  path: string|null,
}

configutation.ts
import { Endpoint, isEndpoint } from './endpoint';

export const isConfiguration = (candidate: any): candidate is Configuration => {
  return (
    candidate && typeof candidate === 'object' &&
    candidate.hasOwnProperty('endpoints') &&
      candidate.endpoints.hasOwnProperty('api') && isEndpoint(candidate.endpoints.api) &&
      candidate.endpoints.hasOwnProperty('smsGateway') && isEndpoint(candidate.endpoints.smsGateway) &&
    candidate.hasOwnProperty('auth') &&
      candidate.auth.hasOwnProperty('company') && (candidate.auth.company === null || typeof candidate.auth.company === 'string')
  );
}

export interface Configuration {
  endpoints: {
    api: Endpoint,
    smsGateway: Endpoint
  },

  auth: {
    company: string|null
  }
}

In the end the code is used in an Angular service like this:
get configuration$(): Observable<Configuration> {
  if (this.configutation$) {
    return this.configutation$;
  }

  const path = environment.configuration;

  this.configutation$ = this.http.get<any>(path).pipe(
    map(configutation => {
      if (isConfigutation(configutation)) {
        return configutation;
      }

      throw new Error('Configutation is broken.');
    }),
    shareReplay(1),
    catchError(error => {
      console.log(error);
      return of(null);
    })
  );

  return this.configutation$;
}

Now the isEndpoint and isConfiguration methos seems very cumbersome, especially when the configuration grows. I was thinking of using JSON schema and validate the input against the schema file. But if somebody replaces that file for example during build, then it still could go wrong.
Can this be improved?
Is this the TypeScrpt way or how could this be improved?

If you're worried that someone may replace your schema during build, you should also be worried that someone might replace any other file as well. That's pure paranoia...
I would say the typescript way is to write the config in typescript as well. If that's not an option, then sure go for schema validation.
Anyway for your current implementation, there is a quite a lot that can be simplified in those conditions.
This:
candidate && typeof candidate === 'object'

can be replaced with just
typeof candidate === 'object'

As there is no falsy value of type object.
Further those hasOwnProperty checks are not necesary. Deserialized json will have all those properties belong to their own. And if somebody passes an object that was not created by parsing json, but let's say instantiating a class, do you really care if those properties belong to the instance, the class or its parent?
export const isConfiguration = (candidate: any): candidate is Configuration => {
  return (
    typeof candidate === 'object' &&
    typeof candidate.endpoints === 'object' &&
    isEndpoint(candidate.endpoints.api) &&
    isEndpoint(candidate.endpoints.smsGateway) &&
    typeof candidate.auth === 'object' &&
    (candidate.auth.company === null || typeof candidate.auth.company === 'string')
  )
}