TwoFish implementation with Bouncy Castle After much research, I was able to implement the twofish algorithm using Bouncy Castle Portable for .NET. The code has been tested and is working perfectly, to the best of my knowledge.  But I do not know if it was well implemented and I would like someone's opinion. It would be nice to know if it is done correctly because it would be for other developers to use. Is there any improvement in security and performance that could be made? Any idea?

public class BCEngine
{
    private readonly Encoding _encoding;
    private readonly IBlockCipher _blockCipher;
    private PaddedBufferedBlockCipher _cipher;
    private IBlockCipherPadding _padding;

    public BCEngine()
    {

    }

    BCEngine(IBlockCipher blockCipher, Encoding encoding)
    {
        _blockCipher = blockCipher;
        _encoding = encoding;
    }

    #region Public Methods

    public static string TwoFishEncryption(string TextPlain, string Password, byte[] Salt)
    {
        Sha3Digest Sha3Digest = new Sha3Digest();
        Pkcs5S2ParametersGenerator gen = new Pkcs5S2ParametersGenerator(Sha3Digest);
        gen.Init(Encoding.UTF8.GetBytes(Password), Salt, 1000);
        KeyParameter param = (KeyParameter)gen.GenerateDerivedParameters(new TwofishEngine().AlgorithmName, 256);

        BCEngine bcEngine = new BCEngine(new TwofishEngine(), Encoding.UTF8);
        bcEngine.SetPadding(new Pkcs7Padding());
        return bcEngine.Encrypt(TextPlain, param);
    }

    public static string TwoFishDecryption(string TextEncripted, string Password, byte[] Salt)
    {
        Sha3Digest Sha3Digest = new Sha3Digest();
        Pkcs5S2ParametersGenerator gen = new Pkcs5S2ParametersGenerator(Sha3Digest);
        gen.Init(Encoding.UTF8.GetBytes(Password), Salt, 1000);
        KeyParameter param = (KeyParameter)gen.GenerateDerivedParameters(new TwofishEngine().AlgorithmName, 256);

        BCEngine bcEngine = new BCEngine(new TwofishEngine(), Encoding.UTF8);
        bcEngine.SetPadding(new Pkcs7Padding());
        return bcEngine.Decrypt(TextEncripted, param);
    }

    #endregion

    #region Private Methods

    void SetPadding(IBlockCipherPadding padding)
    {
        if (padding != null)
            _padding = padding;
    }

    string Encrypt(string plain, ICipherParameters SetKeyParameter)
    {
        byte[] result = BouncyCastleCrypto(true, _encoding.GetBytes(plain), SetKeyParameter);
        return Convert.ToBase64String(result);
    }

    string Decrypt(string cipher, ICipherParameters SetKeyParameter)
    {
        byte[] result = BouncyCastleCrypto(false, Convert.FromBase64String(cipher), SetKeyParameter);
        return _encoding.GetString(result, 0, result.Length);
    }

    byte[] BouncyCastleCrypto(bool forEncrypt, byte[] input, ICipherParameters SetKeyParameter)
    {
        try
        {
            _cipher = _padding == null ?
            new PaddedBufferedBlockCipher(_blockCipher) : new PaddedBufferedBlockCipher(_blockCipher, _padding);

            _cipher.Init(forEncrypt, SetKeyParameter);

            byte[] ret = _cipher.DoFinal(input);
            return ret;

        }
        catch (CryptoException ex)
        {
            // throw new CryptoException(ex);
        }
        return null;
    }

    #endregion

}

static vs instance

public BCEngine()
{

}

BCEngine(IBlockCipher blockCipher, Encoding encoding)
{
    _blockCipher = blockCipher;
    _encoding = encoding;
}

Default constructors should provide reasonable default values for the two fields initialized by the other constructor but wait... both constructors are useless because all public APIs are static. The only instance members are private methods. I can create an instance of BCEngine but I cannot do anything with it.

BCEngine bcEngine = new BCEngine(new TwofishEngine(), Encoding.UTF8);
bcEngine.SetPadding(new Pkcs7Padding());
return bcEngine.Encrypt(TextPlain, param);

But wait again.. you instantiate it to call the private instance methods. This is very inconsistent and chaotic.
You should either make everything static or everything instance.
It's ok if an instance uses private static methods as helpers but the other way around is kind of weird. At least the default constructor should be private too.
However your implementation uses private fields where two of them are not readonly so you use them to store some state but fortunately they are not static. You should remove these fields and pass the required arguments from method to method. Then you won't need any instances and the methods would be pure <-- LINK.

using and disposables
You should check your code for disposable types. I'm pretty sure there are plenty of them. Encryption usually requires to dispose something.

#region
Regions are rarely necessary and for member grouping virtually never. It's easier to read the code without them.


Refactor C# unsafe method I have the following two unsafe methods that I would like to refactor:

public static unsafe void UnsafeMethod(byte[] array, int pos, short value)
{
    fixed (byte* numRef = &(array[pos]))
    {
        *((short*)numRef) = value;
    }
}

public static unsafe void UnsafeMethod(byte[] array, int pos, int value)
{
    fixed (byte* numRef = &(array[pos]))
    {
        *((int*)numRef) = value;
    }
}

If I want to make the method managed will the following conversion be right ?

public static void UnsafeMethod(byte[] array, int pos, short value)
{
    array[pos] = value;
}

public static unsafe void UnsafeMethod(byte[] array, int pos, short value)
{
    fixed (byte* numRef = &(array[pos]))
    {
        *((short*)numRef) = value;
    }
}

is not

public static void UnsafeMethod(byte[] array, int pos, short value)
{
    array[pos] = value;
}

Explanation:
&(array[pos]) gets the address of the memory of the "pos"th element of the array and byte* numRef = &(array[pos]) makes that the pointer numRef points to these address.
if you do *numRef = value you write value into the address numRef is pointing -- which is the "pos"th element of the array.
But you lose precision, because value is short (2 Byte) but you write into a byte (1 Byte) address.

if you cast numRef -- which is a pointer to a byte (= ONE byte) -- into a short (= TWO byte) then the pointer points to the same address BUT the pointer is 2 Byte "wide".
What i mean with "wide" is that if numRef points to 0x0 and you do numRef++ then numRef will point to 0x2 because it is short and short is 2 Byte big.

We have this short* numRef and you want to do *numRef = value then you will write in the memory of the "pos"th element of the array AND in the "pos + 1"th element of the array. so this method is not:

array[pos] = value

it is something like (i can't say that exactly because i am not the compiler):

array[pos] = upperByte(value)
array[pos + 1] = lowerByte(value)

Grafical Explanation:

May this be your array:

Address in RAM| 0x02 | 0x03 | 0x04 | 0x05 | 0x06 | 0x07 | 0x08 | 0x09 |
Array Indexer | [0]  | [1]  | [2]  | [3]  | [4]  | [5]  | [6]  | [7]  |
Content       | 0x00 | 0x00 | 0x00 | 0x00 | 0x00 | 0x00 | 0x00 | 0x00 |

and pos is 2.
and value is this: value = 0x1234

so the pointer numRef points to 0x04:

print(numRef); // Ouptup will be 0x04

and looks like this:

Pointer byte* numRef      -->      <--
Address in RAM| 0x02 | 0x03 | 0x04 | 0x05 | 0x06 | 0x07 | 0x08 | 0x09 |
Array Indexer | [0]  | [1]  | [2]  | [3]  | [4]  | [5]  | [6]  | [7]  |
Content       | 0x00 | 0x00 | 0x00 | 0x00 | 0x00 | 0x00 | 0x00 | 0x00 |

and after *numRef = value the array looks like this:

Pointer byte* numRef      -->      <--
Address in RAM| 0x02 | 0x03 | 0x04 | 0x05 | 0x06 | 0x07 | 0x08 | 0x09 |
Array Indexer | [0]  | [1]  | [2]  | [3]  | [4]  | [5]  | [6]  | [7]  |
Content       | 0x00 | 0x00 | 0x34 | 0x00 | 0x00 | 0x00 | 0x00 | 0x00 |

if you cast it to short then you have this:

Pointer short* numRef     -->             <--
Address in RAM| 0x02 | 0x03 | 0x04 | 0x05 | 0x06 | 0x07 | 0x08 | 0x09 |
Array Indexer | [0]  | [1]  | [2]  | [3]  | [4]  | [5]  | [6]  | [7]  |
Content       | 0x00 | 0x00 | 0x00 | 0x00 | 0x00 | 0x00 | 0x00 | 0x00 |

and after *numRef = value the array looks like this:

Pointer short* numRef     -->             <--
Address in RAM| 0x02 | 0x03 | 0x04 | 0x05 | 0x06 | 0x07 | 0x08 | 0x09 |
Array Indexer | [0]  | [1]  | [2]  | [3]  | [4]  | [5]  | [6]  | [7]  |
Content       | 0x00 | 0x00 | 0x12 | 0x34 | 0x00 | 0x00 | 0x00 | 0x00 |

How it could be implemented (not recommended):

  if you use this methods PLEASE test it. with different system (32 / 64
  bit).

for the public static unsafe void UnsafeMethod(byte[] array, int pos, short value):

public static void SafeMethod(byte[] array, int pos, short value)
{
    array[pos] = value >> 8;
    array[pos + 1] = value & 0xFF;
}

and for the same method with int (and here is the problem - is this int 64 or 32 bit?!) it could look like this:

// For 64 bit integer
public static void SafeMethod(byte[] array, int pos, int value)
{
    array[pos    ] = (value >> 24) & 0xFF;
    array[pos + 1] = (value >> 16) & 0xFF;
    array[pos + 2] = (value >> 8) & 0xFF;
    array[pos + 3] = value & 0xFF;
}

Legend:

value >> x <=> shift the bits of value x times right
value << x <=> shift the bits of value x times left
value & 0xFF <=> value & 0b11111111 <=> logical bitwise and


Zeckendorf Representation of positive integer Zeckendorf's theorem states that all positive integers crepresented uniquely as the sum of one or more distinct Fibonacci numbers in such a way that the sum does not include any two consecutive Fibonacci numbers.

So like given the input 10, the outcome would be [8,2].

This is my working code that was for Reddit's dailyprogrammer challenge:

def fib_list(max_num):
    x=3     #starting at 3 because we have [1,1] in list already.
    list_of_fib = [1,1]
    while x < max_num:
        x = list_of_fib[-1] + list_of_fib[-2]
        list_of_fib.append(x)
    return list_of_fib

def find_num_smaller(num, my_list):
    for value in reversed(my_list):
        if value <= num:
            return value

def solve(num):
    combo_list = []
    list_to_pick_from = fib_list(num)
    while num > 0:
        temp_value = find_num_smaller(num,list_to_pick_from)
        num -= temp_value
        combo_list.append(temp_value)
    print(combo_list)

solve(10)

Running it through timeit.timeit I get about 0.06917929102201015 seconds with the input of 1234. Medium size input of 123456789 yields about 0.22879931901115924 seconds and a large size input of 123456789123456789123456789 yields 1.3149479770218022 seconds. Is there anyway to speed it up? Or make it more acceptable at a "higher standard"  (whatever this means, I guess some would call it 'Pythonic' but I'm not sure the correct term)? Maybe is there some better practice I can employ here.

I have thought of turning Solve() into a generator and just get all the combo values out, but I'm not sure if that's faster.

I know that if I were to run it multiple times, I would check if the input is within the already generated fib_list, and if it isn't I would generate a new one. But that's another question, I just want to see one input.

First some general review:

You should use a if __name__ == "__main__": guard around the test run to allow importing of your code from another module.
You should add docstrings to your functions to explain what they do. Docstrings are of the format:

def f(x, y):
    """Returns the sum of `x` and `y`"""
    return x + y

As you already mentioned, here is a way to use generators. This has two draw-backs compared to your code:

In find_num_smaller we have to iterate forwards (you cannot iterate backwards through an iterator, at least not without converting it to a list first).
We need to re-generate the generator every loop, because it will be partially exhausted

def fib(max_num):
    """
    Yields the fibonacci sequence until the last element smaller than `max_num`
    """
    n_1, n = 0, 1
    while n < max_num:
        yield n
        n_1, n = n, n + n_1

def find_num_smaller(num, my_list):
    """
    Iterates through an iterator until the value in the iterator is larger than `num`.
    Returns the value before that.
    """
    last_value = next(my_list)
    for value in my_list:
        if value > num:
            return last_value
        last_value = value
    return last_value

def solve(num):
    """
    Description here
    """
    x = num
    while x > 0:
        temp_value = find_num_smaller(x, fib(num))
        x -= temp_value
        yield temp_value

if __name__ == "__main__":
    print list(solve(10))

Note that your fund_num_smaller will return None if all values in my_list are smaller than num. Here I added returning the last element in that case.

However, this actually performs worse than your code (mostly because iterating reversed saves a lot of time and re-doing the fibonacci sequence wastes a lot of cycles).

As an alternative consider this code:

def find_num_smaller(num, my_list):
    for value in reversed(my_list):
        if value <= num:
            return value

def solve(num):
    l = list(fib(num))
    while num > 0:
        temp_value = find_num_smaller(num, l)
        num -= temp_value
        yield temp_value

Where I used your find_num_smaller and put the fibonacci into a list.

Here is acomparison between the three approaches for the input 123456789123456789123456789123456789123456789123456789 on my machine:

no generator     406 function calls in 0.000 seconds
only generators  10111 function calls in 0.003 seconds
mixed            407 function calls in 0.000 seconds

So using generators here will not give you a speed improvement for most inputs. For the fibonacci numbers it will, however, give you a memory improvement (which will take quite some time to matter).

For really large inputs (like 123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789) there is no improvement visible:

no generator  2947 function calls in 0.017 seconds
mixed         2948 function calls in 0.017 seconds

And for the input 123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789123456789:

no generator  5886 function calls in 0.063 seconds
mixed         5887 function calls in 0.064 seconds

So both approaches seem to scale with the number of digits of the input.


Permutation code in C I am pretty new to competitive programming. It took me almost 3 hours to make this, initially I started with 4 for loops and then to make it scalable for sizes bigger than 4, I decided to turn this into a recursion which is when it took most of my time, even though I am proud of it, I am still worried that it took me an hour to actually come up with the solution and I have a feeling that it is neither efficient nor good, (you could say I am basically doing combination rather than permutation),
I would like a code review please
and how would you make this more efficient?

#include <stdio.h>
#include <stdlib.h>
#include <stdbool.h>
#include <string.h>

int counter = 0;

int factorial(int n){
    if(n==1) return 1;
    else return n*factorial(n-1);
}

int index_present_check(int* n_arr, int n, int depth){
    for(int i = 0 ; i < depth ; i++) if(n_arr[i]==n) return 1;
    return 0;
}

int present_in_array_check(char** store_arr,char* arr_to_print,int counter){
    for(int i = 0; i < counter; i++) if(strcmp(store_arr[i],arr_to_print)==0) return 1;
    return 0;
}

void permutation(int depth,int length,char* src_arr, char* arr_to_print,int*     n_arr,char** store_arr){
    for(int i = 0 ; i < length ; i ++){
        if(depth ==1){
            arr_to_print[0] = src_arr[i];
            n_arr[0] = i;
            permutation(depth+1,length,src_arr,arr_to_print,n_arr,store_arr);
        }
        else{
            if(!index_present_check(n_arr,i,depth)){
                arr_to_print[depth-1] = src_arr[i];
                n_arr[depth-1] = i;
                permutation(depth+1,length,src_arr,arr_to_print,n_arr,store_arr);
                n_arr[depth-1] = -1;
            }
        }
    }
    if(depth==length){
        if(!present_in_array_check(store_arr,arr_to_print,counter)){
            printf("%s ",arr_to_print);
            strcpy(store_arr[counter],arr_to_print);
            counter++;
            if(counter%factorial(length-1)==0){
                printf("\n");
            }
        }
    }
}

int main()
{
    char* src_arr = "ABCDE";
    int length = 4;
    int depth = 1;

    int iteration_length = factorial(length);

    char* arr_to_print = malloc(length*sizeof(char));
    int* n_arr = malloc(length*sizeof(int));
    char** store_arr = (char**)malloc(sizeof(char*)*iteration_length);
    for(int i = 0 ; i<iteration_length;i++) store_arr[i] = malloc(sizeof(char)*length);

    permutation(depth,length,src_arr,arr_to_print,n_arr,store_arr);

    return 0;
}

I posted this in reddit too, but it was a bit off timing and didn't get much code review, however it was helpful nonetheless.

There are only two versions of the main function:

/* 1 */
int main(int argc, char *argv[]) { ... }

/* 2 */
int main(void) { ... }

Note the void keyword in the second version. In C, if the function has no parameters it should be declared as

 <type> f(void)

Please, take a look at this SO answer for details.
Do not write one-line code:

for(int i = 0; i < counter; i++) if(strcmp(store_arr[i],arr_to_print)==0) return 1;

I'd rewrite it this way:

for (int i = 0; i < count; i++)
    if (strcmp(store_arr[i], arr_to_print) == 0)
        return 1;

Do not use int as type for array indexing. It is not guaranteed  that int can hold any possible index value of an array object. The only type that can hold any index of an array object is size_t. See this SO answer for details.
I'd replace

printf("\n");

with

putchar('\n');

Do not cast the result of malloc call. Really, you have to do it in C++, but not in C. See this SO question for details.
Do not repeat yourself. Consider this line:

int* n_arr = malloc(length * sizeof(int));

You declare n_arr as a pointer to int. It would be better to rewrite this line in this way:

int *n_arr = malloc(length * sizeof *n_arr);

In this case you don't have to change the expression of the sizeof operator if you change n_arr type. It is very common C idiom.
Do not computer factorial using recursion. It is really slow. Usually, you can just create the factorial table and hardcode it:

static const unsigned long long factorial_table[] = {
    1,
    2,
    6,
    24,
    120,
    720,
    ...
};

In this line:

char* src_arr = "ABCDE";

you create a pointer to a string literal. It would be better to create just a static array:

static src_arr[] = "ABCDE";

And you can make it const since it never changes:

static const src_arr[] = "ABCDE";


Function with underscore overuse The code below is converting: data from one format to another

# input sample
{
  productText: [
    {
      "language": "en",
      "version": "1",
      "sequence": 1,
      "text": "blah",
      "textType": "ROMANCE"
    }
  ]
}

# output sample
{
  productText: [
    {
      sequence: 1,
      textType: "ROMANCE"
      text: [
        {
          text: "blah",
          language: "en",
          version: "1",
        }
      ]
    }
  ]
}

The source:

deserialize: (profileJSON)->
  return profileJSON unless profileJSON.productText?
  profileJSON.productText = _(profileJSON.productText).reduce(
    (acc, el)->
      current  = _(acc).findWhere(
        _(el).pick('sequence', 'textType')
      )
      unless current?
        current = _(el).pick('sequence', 'textType')
        current.text = []
        acc.push(current)
      current.text.push(
        _(el).pick('version', 'text', 'language')
      )
      return acc
    []
  )
  return profileJSON

My colleague said that I overuse underscore. Is he right? Is it a good piece of code?

Interesting question,

I am not sure there is such a thing as over-use of underscore, and what the drawback would be. I think that code looks okay from that perspective.

However, from a once over:

deserialize as a function name is unfortunate, especially since it does not deserialize JSON
profileJSON is equally unfortunate, it does not contain JSON
This seems out of order, and a roundabout way of doing things:

current.text = []
acc.push(current)
current.text.push(
  _(el).pick('version', 'text', 'language')
)

could be more like

current.text = [ _(el).pick('version', 'text', 'language') ]
acc.push(current)

You are wrapping el 3 times, wrap it once and assign it to a variable, perhaps that is what your colleague meant?
You call _(el).pick('sequence', 'textType') twice, again you should have cached this, see above
acc and el are meaningless variables and don't give me any insight as to what is inside productText


(Brilliant.org) Arrays Intermediate 3: Contiguous Knapsack Problem

Given an array and a number \$t\$, write a function that determines if there exists a contiguous sub-array whose sum is \$t\$.

Source (Note that the URL may not link to the exact problem as it's from a quiz site, and questions seem to be randomly generated).
I was able to come up with a \$O(n^2)\$ solution, and further thinking hasn't improved upon it:
def substring_sum(lst, target):
    sums = []
    for val in lst:
        if val == target:
            return True
        for idx, _ in enumerate(sums):
            sums[idx] += val
            if sums[idx] == target:
                return True
        sums.append(val)
    return False

You are currently calculating the sum of all possible subarrays. A different approach is to imagine a sliding window on the array. If the sum of the elements in this window is smaller than the target sum, you extend the window by one element, if the sum is larger, you start the window one element later. Obviously, if the sum of the elements within the window is the target, we are done.

This algorithm only works if the array contains only non-negative numbers (as seems to be the case here when looking at the possible answers).

Here is an example implementation:

def contiguous_sum(a, target):
    start, end = 0, 1
    sum_ = sum(a[start:end])
    # print(start, end, sum_)
    while sum_ != target and start < end < len(a):
        if sum_ < t:
            end += 1
        else:
            start += 1
            if start == end:
                end += 1
        sum_ = sum(a[start:end])
        # print(start, end, sum_)
    return sum_ == target

This algorithm can be further improved by only keeping a running total, from which you add or subtract:

def contiguous_sum2(a, t):
    start, end = 0, 1
    sum_ = a[0]
    #print(start, end, sum_)
    while sum_ != t and start < end < len(a):
        if sum_ < t:
            sum_ += a[end]
            end += 1
        else:
            sum_ -= a[start]
            start += 1
            if start == end:
                sum_ += a[end]
                end += 1
        #print(start, end, sum_)
    return sum_ == t

The implementation can be streamlined further by using a for loop, since we actually only loop once over the input array, as recommended in the comments by @Peilonrayz:

def contiguous_sum_for(numbers, target):
    end = 0
    total = 0
    for value in numbers:
        total += value
        if total == target:
            return True
        while total > target:
            total -= numbers[end]
            end += 1
    return False

All three functions are faster than your algorithm for random arrays of all lengths (containing values from 0 to 1000 and the target always being 100):


Core-data object with relationships conforming protocol in swift I have a situation where I have to use protocol to be conformed by NSManagedObject which has relationships with other entities.

My protocol is like:

protocol Account {
  var accountId: String { get }
  var isValidAccount: Bool { get }
}

Here is that entity:

class AccountMO: NSManagedObject {
  @NSManaged var uid: String!
  @NSManaged var anotherEntity: AnotherEntity?

  private (set) var storedMOC: NSManagedObjectContext!
  override var managedObjectContext: NSManagedObjectContext? {
    return storedMOC
  }

  override init(entity: NSEntityDescription, insertInto context: NSManagedObjectContext?) {
    super.init(entity: entity, insertInto: context)
    storedMOC = context!
  }
  init(moc:NSManagedObjectContext) {
    //I'm using this method to init AccountMO
    let mEntity = NSEntityDescription.entity(forEntityName: "Account", in: moc)
    super.init(entity: mEntity!, insertInto: moc)
  }
  func addAnotherEntity() {
    //this method creates AnotherEntity
    //and this method will be called after particular event has happened
  }
}

Extension which is conforming protocol:

extension AccountMO: Account {

  var accountId: String {
    return uid!
  }
  var isValidAccount: Bool {
    return anotherEntity != nil
  }
}

Whenever I want Account properties, I uses DB interface to fetch and return AccountMO as:

class DBInterface {
  //initialization methods
  var mainThreadMOC: NSManagedObjectContext?

  func createNewAccount(uid: String) {
    //create and save account to database
  }
  func getAccount() -> Account {
    //this method abstracts database implementations from outside classes
   //in future if I wanted to replace core-data by another database, I've to change this method only
    let privMOC = privateMOC()//this is child of mainThreadMOC
    let accountMO = fetchAccountEntity(moc: privMOC)
    return accountMO as Account
  }
}

This works fine!

But what I felt is storedMOC in AccountMO is something odd man out!
If I don't store MOC in AccountMO object, then it crashes on

return anotherEntity != nil

this line.
Apparently this is caused because AccountMO looses it's managedObjectContext once returned from getAccount() method.

So, to prevent AccountMO from losing it's managedObjectContext I've added that extra property to it.

Please feel free to tear this implementation down if needed!

NSManagedObjects already have a property for their current context. It is weak however, which is usually ok for most cases because you would retain the context in the class which is using the objects or in the fetch controller that fetched them. If you are using a child context then you need to be sure to retain that otherwise the object fetched from it will lose it.

More info here.


Computation for finding the largest prime factor of 600851475143 is slow I implemented the following for Project 3 in Project Euler:

--Problem 3. The prime factors of 13195 are 5, 7, 13 and 29. What is the
--largest prime factor of the number 600851475143?

prime :: Integer -> Bool
prime x = prime' x (x `div` 2) where
          prime' _ 1 = True
          prime' x y = if x `mod` y == 0 then False
                       else prime' x (y - 1)

largestPrimeFactor :: Integer => Maybe Integer
largestPrimeFactor x = largestPrimeFactor' x (x `div` 2)
             where largestPrimeFactor' _ 1 = Nothing
                   largestPrimeFactor' x y = if x `mod` y == 0 && prime y then Just y
                                             else largestPrimeFactor' x (y - 1)

I was able to get Just 29 per the introduction to this problem from Project Euler:

*Main> largestPrimeFactor 13195
Just 29
*Main> largestPrimeFactor 600851475143 -- running for (as of now) 15 hours!

My implementation succeeded for getting 29 from 13195, but the computation for 600851475143 has been running for 15 hours so far.

Please review my implementation for correctness and idiomatic Haskell.

Your algorithm doesn't scale.  In this answer, I've outlined the three common strategies for finding the largest prime factor of a number, only one of which is reasonably efficient.  You've chosen Option 2 (testing largest candidates first, then check for primality).  However, you start at \$\lfloor\frac{n}{2}\rfloor\$ rather than \$\lceil\sqrt{n}\rceil\$, which is even less efficient, and furthermore it incorrectly produces Nothing whenever n is already prime.

Here's a Haskell implementation of Option 3 (testing smallest candidates first):

largestPrimeFactor :: Integer -> Maybe Integer
largestPrimeFactor n
  | n <= 1    = Nothing
  | otherwise = Just $ largestPrimeFactor' n (2 : [3, 5..])
  where
    largestPrimeFactor' n pseudoprimeCandidates@(c:cs)
      | c * c >= n = n
      | m == 0     = largestPrimeFactor' d pseudoprimeCandidates
      | otherwise  = largestPrimeFactor' n cs
      where
        (d, m) = divMod n c

Personally, I'd avoid contaminating the output with Maybe, since Nothing will only result from obviously illegal input anyway.

largestPrimeFactor :: Integer -> Integer
largestPrimeFactor n
  | n <= 1    = error "largestPrimeFactor n where n <= 1"
  | otherwise = largestPrimeFactor' n (2 : [3, 5..])
  where
    largestPrimeFactor' n pseudoprimeCandidates@(c:cs)
      | c * c >= n = n
      | m == 0     = largestPrimeFactor' d pseudoprimeCandidates
      | otherwise  = largestPrimeFactor' n cs
      where
        (d, m) = divMod n c


Making a ul list element animation on hover I have an experimental script that makes an animation of a ul list element background-position, when the list element is hovered. Is there another way to manage this task or just optimize this code?

I want to make a menu that has elements with an animated background and have an idea about rotoscope style design or something in that direction. I'm thinking in cross-browser compatibility way too. In this stage this code runs fine in Opera, Internet Explorer 6, Chrome, Firefox 2.0, and Firefox 4.0.

jsFiddle

$(document).ready(function(){
  $("li.animate").hover(
    function () {

      var param = '0 0';
      ids = setInterval(function() {
        if ( c >= 5 ) {
          c = 1;
          $('.animate').css('backgroundPosition', '0 0');
        }
        else {
          param = (-100 * c++ )+'px 0';
          //alert(param);
          $('.animate').css('backgroundPosition', param);
          if ( c > 4 ) $('.animate').css('backgroundPosition', '0 0');
        }
      }, 40);
    },
    function () {
      $('.animate').css('backgroundPosition', '0 0');
      clearInterval(ids);
    }
  );
});

First of all i mention again, that i would use a GIF animation to solve this and not a JS. Its a very suboptimal solution to a simple problem.

Your code in the code block is missing two var declarations you have in you jsFiddle example.

Here are the things i would change:

In the most cases, when you have to type the same value twice you do something wrong. Use variables to store your stuff. Makes code more maintainable, easy to read, faster in some cases etc... And give your variables names that speak for them selves. Since you don't write any comments, its even more important.

Cache your jQuery objects in variables. By selecting them again and again, your code gets slow and is hard to mainain. Here an example:

$animate = $("li.animate");
$animate.hover(/* blabalbal */);

Put all your code in a function and pass variable stuff (things that could change) as parameters. Always ask your self what could change if you want to reuse the same code for a different CSS / HTML configuration. (Like different size of the element)

var yourFunction = function( $elem ){
  $elem.each(function(){
   //do something
  });
};

$(function(){ //document ready
  yourFunction( $("whatever element you want to handle") );
});

Then you can call your function in the document ready and you don't have to write all your stuff into your document ready function.

In general split your code in as small generic functions as possible. (Morden browser compile functions that are used often, can make a huge performance differences)

Inside your hover functions you reselect $('.animate'). What are you doing if you have more then one element that has this class? You can refer to the element you are hovering with this inside your hover function:

   $(something).hover(function(){ $(this) //give you back a jQuery selection of the hovered element
   },function(){});

The biggest problem i see in your code is that you have declared your variables very globally, what are you doing if you need to apply this code twice on the same page?

Hope this helps.


Destructure array and use object shorthand assignment I have the following code:

let id = '';
let text = '';

// Note: `match` = [full match, id (first group), text (second group)]
if (match.length > 1) {
  [, id] = match;
}
if (match.length > 2) {
  [, id, text] = match;
}
return { id, text };

match could be null, or lengths between 1-3.

id and text need to be empty strings by default and only set if match is not null and of certain length.

How can I shorten this up?

Possibly avoiding the if statements.

I discovered that array destructing has the capability of providing default values.

const [, id = '', text = ''] = match;
return { id, text };

Array Destructuring


Graph generator for undirected graphs I am still pretty new to Haskell and am working on a graph generator for undirected unlabeled graphs containing loops and I have a bottleneck in the following functions. So far, I have not given any thought to performance and just looked for correctness.

I know this kind of question is not really popular, but I'd be interested in general guidelines for improving performance given some naive but correct implementation like the following.

For instance:

Where to enforce strictness?
Should I use vector/array instead of lists? When, where and why?
Should I improve single functions by replacing recursion using folds/maps/etc. or similar?

Additional info:

I'm using -O2  with ghc
profiling shows (when normalizing runtime of connections to 100%)

arqSeq 40% (with almost all time spend in boundsequences)
connectionCombinations 60% with a quarter of the time spend in occurences

I don't think you need to understand all the details.  I'm looking more for micro-improvements (the following determines arcs from a given node to equivalence classes of vertices grouped by their degree).

import Data.List
import Control.Monad

type Arcs = Int
type Count = Int
type Vertex = Int
type MSequence = [Int]
data EquivClass = EC { order :: Int, verts :: [Vertex] } deriving (Eq, Show)
type ECPartition = [EquivClass]
type NodeSelection = [(Arcs,Count)]
type ECNodeSelection = (EquivClass, NodeSelection)

-- number of occurences of unique elements in a list
occurences :: (Ord a) => [a] -> [(a, Int)]
occurences = map (\xs@(x:_) -> (x, length xs)) . group . reverse . sort

-- number of vertices in an equivalence class
ecLength :: EquivClass -> Int
ecLength = length . verts

-- for a given y = (y_1,...,y_n) and a bound m, find all vectors
-- x = (x_1,...,x_n) such that |x| = m and x_i <= y_i
boundSequences :: (Num a, Ord a, Enum a) => a -> [a] -> [[a]]
boundSequences m x | m <= sum x = (fByM . sequence . ranges) x
                   | otherwise = [[]]
    where fByM = filter (\x -> sum x == m)
          ranges = map (\x -> [0..x])

-- return m-sequences (combinations of number of arcs) for the given
-- order to an ECPartition
arcSeq :: Int -> ECPartition -> [MSequence]
arcSeq m x = boundSequences m (map ecProd x)
    where ecProd e = ecLength e * order e

-- return all the possible arc combinations  to an equivalence
-- class for a given number of arcs
connectionCombinations :: Int -> EquivClass -> [NodeSelection]
connectionCombinations arcs = map groupOcc . prune arcs . sequence . orderRep
    where orderRep (EC o v) = replicate (length v) [0..o]
          prune a = nub . map (reverse . sort) . filter ((== a) . sum)
          groupOcc = filter ((/= 0) . fst) . occurences

-- return all the possible lists of equivalence class node selections
-- from a Partition and a givne number of arcs
connections :: ECPartition -> Int -> [[ECNodeSelection]]
connections p order = concatMap (con p) $ arcSeq order p
     where con p arcs = map (zip p) $ zipWithM connectionCombinations arcs p

main = do
    -- small example. In the complete app this is called 10,000's of
    -- times in case of high degrees or vertex counts
    let cons = connections [EC 4 [1], EC 1 [2,3,4]] 5
    mapM_ (print) cons

-- for a given y = (y_1,...,y_n) and a bound m, find all vectors
-- x = (x_1,...,x_n) such that |x| = m and x_i <= y_i
boundSequences :: (Num a, Ord a, Enum a) => a -> [a] -> [[a]]
boundSequences m x | m <= sum x = (fByM . sequence . ranges) x
                   | otherwise = [[]]
    where fByM = filter (\x -> sum x == m)
          ranges = map (\x -> [0..x])

Excepting some fortunate cases, you are wasting a lot of work here. sequence . ranges produces (y_1+1) * ... * (y_n+1) lists you have to check. If you write a function to produce only the successful lists, you can gain a lot of performance for larger input (it won't do too much for short lists with small elements, but it will help for those too).
Shouldn't the case sum x < m return [] rather than [[]]?

-- return all the possible arc combinations  to an equivalence
-- class for a given number of arcs
connectionCombinations :: Int -> EquivClass -> [NodeSelection]
connectionCombinations arcs = map groupOcc . prune arcs . sequence . orderRep
    where orderRep (EC o v) = replicate (length v) [0..o]
          prune a = nub . map (reverse . sort) . filter ((== a) . sum)
          groupOcc = filter ((/= 0) . fst) . occurences

Instead of reverse . sort, use sortBy (flip compare). That won't make much difference for short lists, but it's cleaner, IMO. prune arcs . sequence contains another occurrence of the boundSequences problem, creating a lot of lists and filtering out most of them immediately. nub is not good, it is quadratic (worst case, O(total*distinct) in general). The things you're nubbing have an Ord instance, if the order doesn't matter, map head . group . sort is a much faster nub than nub (still better Data.Set.toList . Data.Set.fromList), if order matters, keep a Set of seen elements and for each new, output that and add it to the set of seen.

I've not yet profiled, but it may be that the profile is misleading because it attributes costs to the functions where results are forced rather than where they are calculated, that needs some investigation, check back later.

Okay, the given data doesn't run long enough to produce a meaningful profile, so I changed it to

let cons = connections [EC 4 [1 .. 6], EC 6 [2 .. 9]] 5

That runs long enough to collect a handful of samples. Unfortunately, the profile wasn't very informative:

    total time  =        7.90 secs   (395 ticks @ 20 ms)
    total alloc = 8,098,386,128 bytes  (excludes profiling overheads)
COST CENTRE                    MODULE               %time %alloc
connectionCombinations         Main                 100.0  100.0

So I inserted a couple of {-# SCC #-} pragmas. The biggo in that programme is the sequence . orderRep, unsurprisingly. A rewrite along the lines mentioned above,

boundSequences :: (Num a, Ord a, Enum a) => a -> [a] -> [[a]]
boundSequences m xs
    | sm < m    = []
    | sm == m   = [xs]
    | otherwise = go sm m xs
      where
        sm = sum xs
        go _ r []
            | r == 0 = [[]]
            | otherwise = []
        go _ r [y]
            | y < r     = []
            | otherwise = [[r]]
        go s r (y:ys) = do
            let mny | s < r+y   = r+y-s
                    | otherwise = 0
                mxy = min y r
            c <- [mny .. mxy]
            map (c:) (go (s-y) (r-c) ys)

and using that in connectionCombinations instead of prune arcs . sequence (requires a small change in orderRep),

connectionCombinations :: Int -> EquivClass -> [NodeSelection]
connectionCombinations arcs = map groupOcc . nub . map (sortBy (flip compare)) .
                                     boundSequences arcs . orderRep
    where orderRep (EC o v) = replicate (length v) o
          groupOcc = filter ((/= 0) . fst) . occurences

brought the running time (ghc -O2, no profiling) down significantly:

dafis@schwartz:~/Cairo> time ./orArcs > /dev/null

real    0m5.836s
user    0m5.593s
sys     0m0.231s
dafis@schwartz:~/Cairo> time ./arcs > /dev/null

real    0m0.008s
user    0m0.005s
sys     0m0.003s

Whether any further optimisations (e.g. the abovementioned nub) are necessary, and where most of the time in the actual programme is spent, so that one could identify the most important points, I cannot tell without more realistic data.


Shortest path in a grid between two points I have this problem where I have to find the shortest path in an NxM grid from point A (always top left) to point B (always bottom right) by only moving right or down. Sounds easy, eh? Well here's the catch: I can only move the number shown on the tile I'm sitting on at the moment. Let me illustrate:

2 5 1 2
9 2 5 3
3 3 1 1
4 8 2 7

In this 4x4 grid the shortest path would take 3 steps, walking from top left 2 nodes down to 3, and from there 3 nodes right to 1, and then 1 node down to the goal.

[2] 5  1  2
 9  2  5  3
[3] 3  1 [1]
 4  8  2 [7]

If not for the shortest path, I could also be taking this route:

[2] 5 [1][2]
 9  2  5  3
 3  3  1 [1]
 4  8  2 [7]

That would unfortunately take a whopping 4 steps, and thus, is not in my interest.
That should clear things out a bit. Now about the input.

The user inputs the grid as follows:

5 4      // height and width
2 5 2 2  //
2 2 7 3  // the
3 1 2 2  // grid
4 8 2 7  //
1 1 1 1  //

So what have I done? I figured the best way around this (to begin with) would be BFS. Voila, correct answer on every input. Hurray, it's too freakin' slow.

Anyway here's the code at the moment:

#include <iostream>
#include <vector>

struct Point {
    int y, x, depth;
    Point(int yPos = 0, int xPos = 0, int dDepth = 0) : y(yPos), x(xPos), depth(dDepth) { }
};

struct grid_t {
    int height, width;
    std::vector< std::vector<int> > tiles;

    grid_t() // construct the grid
    {
        std::cin >> height >> width; // input grid height & width

        tiles.resize(height, std::vector<int>(width, 0)); // initialize grid tiles

        for(int i = 0; i < height; i++)     //
            for(int j = 0; j < width; j++)  // input each tile one at a time
                std::cin >> tiles[i][j];    // by looping through the grid
    }
};

int go_find_it(grid_t &grid)
{
    std::vector<Point> openList, closedList;
    openList.push_back(Point(0, 0)); // (0, 0) is the first point we want to consult, of course

    do
    {
        closedList.push_back(openList[0]); // the tile we are at is good and checked. mark it so.
        openList.erase(openList.begin()); // we don't need this guy no more

        int y = closedList.back().y; // now we'll actually move to the new point
        int x = closedList.back().x; //
        int depth = closedList.back().depth; // the new depth

        if(y == grid.height-1 && x == grid.width-1) return depth; // the first path is the shortest one. return it

        int jump = grid.tiles[y][x]; // 'jump' is the number shown on the tile we're standing on.
        grid.tiles[y][x] = 0; // mark the tile visited

        if(y + jump < grid.height && grid.tiles[y+jump][x] != 0) // if we're not going out of bounds or on a visited tile
        {
            openList.push_back(Point(y+jump, x, depth+1)); // push in the new promising point along with the new depth
        }
        if(x + jump < grid.width && grid.tiles[y][x+jump] != 0) // if we're not going out of bounds or on a visited tile
        {
            openList.push_back(Point(y, x+jump, depth+1)); // push in the new promising point along with the new depth
        }
    }
    while(openList.size() > 0); // when there are no new tiles to check, break out and return false

    return 0;
}

int main()
{
    grid_t grid; // initialize grid

    int min_path = go_find_it(grid); // BFS

    std::cout << min_path << std::endl; // print the result
    //system("pause");
    return 0;
}

So what I want is some optimization hints, because I suck at it (and am relatively new to C++). Basically jumped into it a month ago with only as much knowledge from PHP and javascript). Should I make it A* instead of BFS? If I did that, how would I calculate the heuristic?

I think BFS is your algorithm, but I do have some cleanups for you. The reason A* won't work is because your estimation of how much farther you have to go (you could do a simple grid distance from where you are to the end point, also considering the jump value where you are sitting) could be wrong based on future jumps, and in your case, estimations are not acceptable. (Note: in some cases, for example if this was an AI in a game, estimation can be acceptable if performance is greatly improved)

Use a queue instead of a vector for openList. Every erase on the first item in a vector means you have to copy all the items to new memory positions!!! BFS algorithm and  std::queue are a married pair because your push and pop are constant time, and you won't accidentally push or pop at the wrong end.
You are keeping a list of visited points, but you aren't using this list, and you aren't using it because you don't need it. You can't go backwards in your path toward the end, so you don't have to keep track of where you have been.


Parsing input in form of 'CMD NUM NUM' (num could be special char '%') In the main() function of the following code, I wrote a very naive command parsing. Is there any more efficient way?

This program is aim to deal with ADD, SUB, MUL, DIV these four arithmetic operations for integers. The input would be in form like: ADD 1 1, MUL 12 90. And also there would be a special input character % which means use the last result. For example, ADD 1 1 would return 2, then ADD % 1 would return 3.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

typedef int (*calc_function)(int, int);

typedef struct operation {
        char *name;
        calc_function calculate;

} Operation;

int add(int a, int b) { return a+b; }
int sub(int a, int b) { return a-b; }
int mul(int a, int b) { return a*b; }
int divi(int a, int b) { return a/b; }

int calc(int a, int b, int (*c)(int, int)) {
        return c(a, b);
}

int main() {

        char *command = malloc(9);
        int result = 0;
        int a;
        int b;

        Operation ADD = {"ADD", add};
        Operation SUB = {"SUB", sub};
        Operation MUL = {"MUL", mul};
        Operation DIV = {"DIV", divi};

        Operation ops[4] = {ADD, SUB, MUL, DIV};

        while((command = fgets(command, 9, stdin)) != NULL) {

                for(int i = 0; i < 4; ++i) {
                        if (0 == strncmp(ops[i].name, command, 3)) {
                                command = strchr(command, ' ');
                                command++;
                                if (*command == '%') {
                                        a = result;
                                } else {
                                        sscanf(command, "%d", &a);
                                }
                                command = strchr(command, ' ');
                                command++;
                                if (*command == '%') {
                                        b = result;
                                } else {
                                        sscanf(command, "%d", &b);
                                }
                                result = ops[i].calculate(a, b);
                                printf("%d\n", result);
                        }
                }
        }
        free(command);

        return 0;
}

Also, any advice on improving the performence and style of this program would be highly appreciated!

Overall Impression

The code is pretty good. The structure of the code is good, good use of typedef. The code uses that safe version of string comparison strncmp(string1, string2, maxLength). Each Variable is declared on a separate line.

One overall observation is the code is not very extendable.

The program might be more understandable to the user if = is used to get the result rather than %. This would also possibly allow the addition of a modulus operation.

Error Checking

There are at least 2 places in the code where error checking should be performed, the first is always check the return value of malloc(size_t size). If malloc() fails for some reason, it returns NULL access through a null pointer results in UB (Unknown Behavior). Quite often it terminates the program in some nasty manner.

It's always best to check user input for errors so that they can be reported and the user can correct their error.

Magic Numbers

There are Magic Numbers in the main() function (9, 4 and 3), it might be better to create symbolic constants for them to make the code more readable and easier to maintain. These numbers may be used in many places and being able to change them by editing only one line makes maintenance easier.

Numeric constants in code are sometimes referred to as Magic Numbers, because there is no obvious meaning for them. There is a discussion of this on stackoverflow.

In the case of the array of Operations called ops you don't need to specify 4 in the array definition, and you can get the size after the creation of the array using this formula: sizeof(ops)/sizeof(*ops);

In the cases of using 9 and 3 define a constant values that can change the value throughout the program:

At the top of the file:

#define INPUT_COMMAND_SIZE 9
#define OPERATION_SIZE 3

Complexity

The function main() is too complex (does too much). As programs grow in size the use of main() should be limited to calling functions that parse the command line, calling functions that set up for processing, calling functions that execute the desired function of the program, and calling functions to clean up after the main portion of the program.

There is also a programming principle called the Single Responsibility Principle that applies here. The Single Responsibility Principle states:

  that every module, class, or function should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by that module, class or function.

Declare Variables When Needed

The original version of the C programming language did require variables to be defined at the top of the function, however, this hasn't been true since 1989. It is better to declare the variables when they are necessary to reduce the scope of the variables and make the code easier to understand and maintain.

Initialize Local Variables

The C programming language does not automatically initialize variables. Uninitialized variables can sometimes be the cause of UB. It is generally a best practice to always initialize variables when they are declared.

Horizontal Spacing

Code is much more readable when there is a space separating operators and operands in an expression: return a - b;.

System Macros for Exit Status

The code already includes stdlib.h for the definitions of malloc() and free(). Since stdlib.h is already included it would be better to use the system constants EXIT_SUCCESS and EXIT_FAILURE which can make the code more portable. These constants are defined in stdlib.h.

Example Using Suggestions

Note error checking on user input hasn't been added.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#define INPUT_COMMAND_SIZE 9
#define OPERATION_SIZE 3

typedef int (*calc_function)(int, int);

typedef struct operation {
    char *name;
    calc_function calculate;

} Operation;

int add(int a, int b)
{
    return a + b;
}

int sub(int a, int b)
{
    return a - b;
}
int mul(int a, int b)
{
    return a * b;
}

int divi(int a, int b)
{
    return a / b;
}

int programLoop(Operation *ops, int opsCount)
{
    int programStatus = EXIT_SUCCESS;
    char *command = malloc(INPUT_COMMAND_SIZE);
    if (command != NULL){
        while((command = fgets(command, INPUT_COMMAND_SIZE, stdin)) != NULL) {

            for(int i = 0; i < opsCount; ++i) {
                int result = 0;
                int a = 0;
                int b = 0;
                if (0 == strncmp(ops[i].name, command, OPERATION_SIZE)) {
                    command = strchr(command, ' ');
                    command++;
                    if (*command == '%') {
                        a = result;
                    } else {
                        sscanf(command, "%d", &a);
                    }
                    command = strchr(command, ' ');
                    command++;
                    if (*command == '%') {
                        b = result;
                    } else {
                        sscanf(command, "%d", &b);
                    }
                    result = ops[i].calculate(a, b);
                    printf("%d\n", result);
                }
            }
        }
        free(command);
    }
    else
    {
        fprintf(stderr, "Malloc returned NULL\n");
        programStatus = EXIT_FAILURE;
    }

    return programStatus;
}

int main() {
    Operation ADD = {"ADD", add};
    Operation SUB = {"SUB", sub};
    Operation MUL = {"MUL", mul};
    Operation DIV = {"DIV", divi};

    Operation ops[] = {ADD, SUB, MUL, DIV};

    int opsCount = sizeof(ops)/sizeof(*ops);

    return programLoop(ops, opsCount);
}


Date-filtering function Please check if this date filtering function is inefficient code. It seems so to me, but I can't figure out the other way around it.

function makeTime () {
    /////////////////////////////////
    //Create a filtered Time/////////
    var timesStamp = new Date();
    var time = timesStamp.toLocaleTimeString();
    var timeArray = time.split(":");
    var spaceDeliTime = timeArray.join(" ");
    var rawTime = spaceDeliTime.split(" ");
    var extrTime = rawTime.splice(2,1);
    var AMPM = rawTime.splice(2,1);
    return rawTime.join(":")+' '+AMPM;
}

You are right that slicing and dicing the string representation of a date is not the right way. Parsing a local time string without a specific locale is even worse, and asking for trouble.

A Date object in JavaScript has getHours() and getMinutes() methods.
The result will be in 24-hour format. If you need, you can convert that to am-pm format based on the hour. The resulting code will be more efficient and more predictable than parsing the string representation.

For example:

var timestamp = new Date();
var hours = timestamp.getHours();
var ampm;
if (hours < 12) {
    ampm = 'AM';
} else {
    ampm = 'PM';
    if (hours > 12) {
        hours -= 12;
    }
}

For the record, this code to get the time part in rawTime is very poor:

var timeArray = time.split(":");
var spaceDeliTime = timeArray.join(" ");
var rawTime = spaceDeliTime.split(" ");
var extrTime = rawTime.splice(2,1);

For one thing, the sequence of split by colons -> join by spaces -> split by spaces is really silly. Probably you could have achieved the same thing by simply taking a substring.

The most disturbing part is the splice at the end: the result is stored in extrTime but never used. On closer look it turns out the main purpose of that splice was its side effect of mutating the rawTime array. The logic would have been much more clear if you used array indexes to get the segments that you want rather than slicing and dicing an array.

If parsing the string representation, this would have been a simpler, more efficient, less confusing solution:

function makeTime () {
    var timesStamp = new Date();
    var time = timesStamp.toLocaleTimeString();
    var timePart = time.substr(0, time.indexOf(":", 2))
    var spaceAndAmPmPart = time.substr(time.lastIndexOf(' '))

    return timePart + spaceAndAmPmPart;
}

But I still recommend the different approach using getHours and getMinutes.


Random RPG Battle System in Lua This is my battle system that I wrote in Lua. Bear with me on this one, this is my first real program that does something besides write hello world. If there is anything that I could have done differently, drop an answer down there and I will take your advice for my next program. Huge shoutout to Jakuje for helping me make this, he refined my code for me and told me how to do things correctly.

math.randomseed( os.time() )
math.random(); math.random(); math.random()
local function battle()
  n = math.random(10) + 1 -- Everybody's HP, enemy HP randomly generated number from 10 to 100
  enemyhp = 10*n
  exp = enemyhp*0.5
  g_herohp = 100
  print("You have encountered an enemy!")
  io.write("Your HP: ")
  io.write(g_herohp)
  io.write(" ")
  io.flush()
  io.write("Enemy HP: ")
  io.write(enemyhp)
  io.write(" ")
  io.flush()
end
local function attack()  -- Attacking the enemy or running away
  print("|Attack|Flee| ")
  input = io.read()
  if input == "attack" then
    attackdamage = math.random(51)
    if attackdamage == 51 then
      print("Critical Hit! 100 damage!")
      enemyhp = enemyhp - 100
    else
      enemyhp = enemyhp - attackdamage
      io.write("You dealt ")
      io.write(attackdamage)
      io.write(" damage!")
    end
  elseif input == "flee" then
    escapechance = math.random(2)
    if escapechance == 1 then
    escape = true
    end
  end
end
local function enemyattack() -- Enemy attacks you with a random amount of damage
  print(" Enemy is attacking!")
  eattackdamage = math.random(40)
  g_herohp = g_herohp - eattackdamage
  io.write("Enemy dealt ")
  io.write(eattackdamage)
  io.write(" damage!")
end
local function battleinprogress() -- If I used battle then it would reset everyones health.
  if g_herohp <= 0 then
    print(" You have died!")
    end
  io.write(" Your HP: ")
  io.write(g_herohp)
  io.write(" ")
  io.flush()
  io.write("Enemy HP: ")
  io.write(enemyhp)
  io.write(" ")
  io.flush()
  if enemyhp <= 0 then
    print("You won!")
  end
  if escape == true then
  print("You have run away!")
  end
end
battle()
repeat
attack()
enemyattack()
battleinprogress()
until g_herohp <= 0 or enemyhp <= 0 or escape == true
end

Since you are making calls to math.random a lot, keep a local copy in your code:

local random = math.random

Why are you switching between io.write and print statements? Use a string.format to format the data as you want:

print( ("You dealt %d damage!"):format(attackdamage) )

The above is merely a representation. It can be made a lot better (by putting templates as variables etc.)

n = math.random(10) + 1

The above will generate n in the range of 2 to 11. From the docs:

  When called with two integers m and n, math.random returns a
  pseudo-random integer with uniform distribution in the range [m,n].
  [...] The call math.random(n) is equivalent to math.random(1,n).

And, no need for keeping a temporary variable (here n):

enemyhp = 10 * random(10) -- better yet, use random(10, 100)

escapechance = math.random(2)
if escapechance == 1 then
escape = true
end

can simply become:

escape = random(2) == 1

Convert your input to lowercase first. Players might enter Attack or ATTACK too.

if input:lower() == "attack"

or

local input = io.read():lower()

Most important

You should encapsulate your player, enemies etc. inside objects. Lua supports a rough implementation of OOP design using tables. Read the Lua PiL chapter #16


Detecting if two strings are anagrams I am trying to write a function that detects if two strings are anagrams of one another; more specifically, I want the function to be linear in its time complexity (\$O(n)\$ where \$n\$ is the length of the given string(s)).

My approach for doing this is to essentially maintain a dictionary of the characters and their occurrences. For the first string, I increment the key corresponding to a character; for the second string, I decrement the key corresponding to the character. At the end, I check if every key in the dictionary has a value of 0: if so, the number of chars in the first string 'cancel out' those in the second, which means the occurrences are equal and the strings are anagrams. Else, the strings are not anagrams.

Does this function run in linear time? When I iterate through the strings to add to the dictionary, that's linear I think. But does checking every key in the dictionary afterwards take linear time as well? If so, this would be quadratic in its complexity. How would I resolve the issue then, since I think to make this function linear the dictionary must be used but in a smarter way.

  def are_anagrams(s1,s2):
      """Returns True if s1 and s2 are anagrams of one another.
         False otherwise.
         Precondition: s1, s2 are both strings """

    if len(s1) != len(s2): #automatic failure
        return False

    #same length
    d = {}
    index = 0
    bound = len(s1)
    while index < bound: #iterate through each string and its chars
        s1_char = s1[index]
        s2_char = s2[index]

        #update d for s1
        if s1_char not in d:
            d[s1_char] = 1
        elif s1_char in d:
            d[s1_char] += 1

        #update d for s2
        if s2_char not in d:
            d[s2_char] = -1
        elif s2_char in d:
            d[s2_char] -= 1

        index += 1

    for key in d:
      if d[key]: #if d[key] != 0, one string has an additional/fewer
                 #character
          return False
    return True

As @Graipher suggested, why bother do this yourself when Python's builtins could give you the answer on a golden plate?

from collections import Counter

def are_anagrams(str1, str2):
    """Return True if str1 and str2 are anagrams and False otherwise."""
    return Counter(str1) == Counter(str2)

Some examples:

are_anagrams('123', '321')
True
are_anagrams('', '')
True
are_anagrams('aabbcc', 'bacac')
False

And that's it. Furthermore it's \$O(n)\$ as you wanted.

And don't think it's something hard to grasp underneath, it's basically just a dictionary with its items as keys and counts as values.

Regarding your code, there are a few style issues that I'd like to point out:

avoid useless/obvious comments like # automatic failure
use 4 spaces per indentation level. (always - not two, not one, not 10. Always 4)
there should be one space after the comma (def are_anagrams(s1, s2):)
don't forget the dot at the end of the docstring (OCD, sorry)
you should always put a space after #

Instead of doing this:

for key in d:
    if d[key]:
        ...

You can do (look also for iteritems() for other python versions):

for key, value in d.items():
    if value:
        ...

To iterate over a string's chars it's recommended to use Python's builtin enumerate (instead of that while) like this:

for index, char in enumerate('test'):
    print('char: {}; index: {}'.format(char, index))

char: t; index: 0
char: e; index: 1
char: s; index: 2
char: t; index: 3


'Countdown' Numbers round - combine numbers arithmetically to reach a target Countdown is a British gameshow where contestants compete in word and number challenges. During the numbers round, six numbers are chosen semi-randomly and the task is to combine them using addition, subtraction, multiplication and division in order to reach a target number.

For example, the numbers might be [75; 25; 7; 2; 5; 8] and the target 491.

One winning solution is ((75 + 25) * 5) - 7 - 2 = 491

Below is some F# code that I wrote to try and improve my functional skills. The compute function accepts an int list and an int and returns an Expr which is the closest it could find to the target.

First, declaring the recursive type and functions to evaluate and print them

module Numbers

type Expr =
| Number of int
| Add of Expr * Expr
| Subtract of Expr * Expr
| Multiply of Expr * Expr
| Divide of Expr * Expr

let rec eval = function
| Number x        -> x
| Add (x, y)      -> eval x + eval y
| Subtract (x, y) -> eval x - eval y
| Multiply (x, y) -> eval x * eval y
| Divide (x, y)   -> eval x / eval y

// string representation of an expression, with evaluated result
let write n =
    let rec write' = function
    | Number x        -> x.ToString()
    | Add (x, y)      -> "(" + write' x + " + " + write' y + ")"
    | Subtract (x, y) -> "(" + write' x + " - " + write' y + ")"
    | Multiply (x, y) -> "(" + write' x + " * " + write' y + ")"
    | Divide (x, y)   -> "(" + write' x + " / " + write' y + ")"
    write' n + " = " + (eval n).ToString()

This function that takes an Expr list (in the first case, a list of Numbers) and produces a seq<Expr list> which is calculated by all the possible applications of a single arithmetic operator (+*/-) to any/all pairs of expressions.

let generate exprs =
    // a specialised choose2 function that returns the pair plus the remainders
    // uses indices for comparison in case of duplicate values
    let pairTwo exprs =
        let indexed = List.mapi (fun i x -> i,x) exprs
        [ for (i,x) in indexed do
          for (j,y) in indexed do
          if j > i then
            yield (x,y), [ for (k,z) in indexed do if k <> i && k <> j then yield z ] ]

    // for two given expressions, get possible combinations
    let getExpressions x y =
        let isDivisible x y =
            let denom = eval y
            denom <> 0 && eval x % denom = 0

        let a = max x y
        let b = min x y
        seq {
            yield Multiply (x, y)
            yield Add (x, y)
            yield Subtract(a, b) // don't bother with negatives
            if isDivisible a b
                then yield Divide(a, b)
            yield x // yielding each partial result prevents near-misses getting lost
            yield y // although expands the search tree
        }

    seq {
        for (x,y),zs in pairTwo exprs do
        for expr in getExpressions x y do
        yield expr :: zs }

Finally the compute function which drives the calculation, iterating over all candidate expressions until a match is found or there are no more possibilities

let compute numbers target =
    let distance = eval >> ((-) target) >> abs
    let fitness = Seq.map distance >> Seq.min

    let iterate =
        Seq.collect generate
        >> Seq.sortBy fitness

    let rec compute' candidates =
        let first = Seq.head candidates
        match first, fitness first with
        | x::[], _  -> x // no more iterations, this is the best one
        | exprs,  0 -> Seq.minBy distance exprs // woop, got it
        | _,     _  -> candidates |> iterate |> compute'
    seq { yield numbers |> List.map Number }
    |> compute'

I'd really like feedback on my use of closures, pattern matching, choices of list vs seq, and if anyone can think of a nicer way to implement the pairTwo function I would like to see it

I have slightly simplified the generate function by merging the pairTwo function into the resulting seq computation expression. I refrained from merging the getExpressions function, as its functionality is distinct enough to warrant a separate function IMO.

let generate exprs =
    // for two given expressions, get possible combinations
    let getExpressions x y =
        let isDivisible x y =
            let denom = eval y
            denom <> 0 && eval x % denom = 0

        let a,b = if x > y then x,y else y,x
        seq {
            yield Multiply (x, y)
            yield Add (x, y)
            yield Subtract(a, b) // don't bother with negatives
            if isDivisible a b
                then yield Divide(a, b)
            yield x // yielding each partial result prevents near-misses getting lost
            yield y // although expands the search tree
        }

    let indexed = List.mapi (fun i x -> i,x) exprs

    seq {
        for (i,x) in indexed do
        for (j,y) in indexed do
        if j > i then
            for expr in getExpressions x y do
            yield expr :: [ for (k,z) in indexed do if k <> i && k <> j then yield z ] }

I would still very much appreciate any and all critique!


Find an intersection point of 2 line segments I want to find an intersection point of two line segments, if one exists. Each line segment is represented by two endpoints. Each endpoint is represented as an ordered pair of numbers. There are no guarantees regarding the line segments (e.g., one or both line segments can be vertical or horizontal).

If multiple points of intersection exist, randomly return one of those intersection points. If no intersection exists, return None.

Pointing out any errors or potential improvements would be really appreciated.

from __future__ import division
from collections import namedtuple

Point = namedtuple('Point', 'x y')
Segment = namedtuple('Segment', 'ep1 ep2')

def find_slope(segment):
    """
    Return the slope of the line segment, if it is defined. If it is
    undefined, return None.
    """
    p1, p2 = segment.ep1, segment.ep2
    if p2.x - p1.x == 0.0:
        return None
    else:
        return (p2.y-p1.y) / (p2.x-p1.x)

def find_y_intercept(slope, point):
    """
    Return the y-intercept of an infinite line with slope equal to slope that
    passes through point. If slope does not exist, return None.
    """
    if slope is None:
        return None
    else:
        return point.y - slope*point.x

def order_segment(segment):
    """
    Order endpoints in segment primarily by x position, and secondarily by y
    position.
    """
    ep1, ep2 = segment.ep1, segment.ep2
    if (ep1.x > ep2.x or ep1.x == ep2.x and ep1.y > ep2.y):
        segment.ep1, segment.ep2 = segment.ep2, segment.ep1

def order_segments(segments):
    """
    Order segments by each segment's first endpoint. Similar to order_segment,
    order primarily by first endpoint's x position, and secondarily by first
    endpoint's y position.
    """
    seg1, seg2 = segments
    if (seg1.ep1.x > seg2.ep1.x or seg1.ep1.x == seg2.ep1.x
            and seg1.ep1.y > seg2.ep1.y):
        segments[0], segments[1] = segments[1], segments[0]

def on(point, segment):
    """
    Return True if point lies on segment. Otherwise, return False.
    """
    return (within(segment.ep1.x, point.x, segment.ep2.x) and
            within(segment.ep1.y, point.y, segment.ep2.y))

def within(p, q, r):
    """
    Return True if q is between p and r. Otherwise, return False.
    """
    return p <= q <= r or r <= q <= p

def find_intersection(segment1, segment2):
    """
    Return an intersection point of segment1 and segment2, if one exists. If
    multiple points of intersection exist, randomly return one of those
    intersection points. If no intersection exists, return None.
    """
    [s1, s2] = [find_slope(l) for l in [segment1, segment2]]
    [k1, k2] = [find_y_intercept(s, l[0])
                for s, l in [(s1, segment1), (s2, segment2)]]

    if s1 == s2:
        if k1 != k2:
            return None
        #  at this point, the two line segments are known to lie on the same
        #  infinite line (i.e. all of the endpoints are collinear)
        segments = [segment1, segment2]
        for segment in segments:
            order_segment(segment)
        order_segments(segments)
        intersection = segments[1].ep1
    else:
        #  assume segment 1 has slope and segment 2 doesn't
        s, x, k = s1, segment2.ep1.x, k1
        #  assumption wrong, segment 1 doesn't have a slope, but segment 2 does
        if s1 is None:
            s, x, k = s2, segment1.ep1.x, k2
        #  assumption wrong, segments 1 and 2 both have slopes
        elif s2 is not None:
            x = (k2-k1) / (s1-s2)
        y = s*x + k
        intersection = Point(x, y)

    if on(intersection, segment1) and on(intersection, segment2):
        return intersection
    else:
        return None

I've not written much python, so I'm not qualified to comment on your coding style, etc. However, I do a lot of graphics work, and I can tell you that this is a lot more code than you should have to write for this task. It would be silly of me to repeat it all, but check out the answers to this question on Stack Overflow. You can see that it doesn't take more than a single function of about 10-20 lines.


Efficient data-driven finite state machine In creating this answer to a recent question, I thought I might write a fully worked example that implements a finite state machine in C.

The purpose is to implement the following function:

  Given a const char *, search for a string conforming to the pattern WWWDD where WWW is one of { "Mon", "Tue", "Wed, "Thu", "Fri", "Sat", "Sun" } and DD is an integer in the range \$[1, 31]\$ (inclusive).  If the pattern exists within the string, return the offset; otherwise return -1.

I implemented this state machine: using simple table structure.  This is the code, which includes a simple driver with test vectors.

#include <string.h>
#include <stdlib.h>
#include <ctype.h>

static const struct fsmNode {
    const char target;
    const struct fsmNode *next;
} fsm[] = {
    { 'T' , &fsm[6] },    // 0
    { 'M' , &fsm[9] },    // 1
    { 'S' , &fsm[11] },   // 2
    { 'W' , &fsm[12] },   // 3
    { 'F' , &fsm[14] },   // 4
    { '\0' , NULL },      // 5
    // second letter
    { 'h' , &fsm[18] },   // 6
    { 'u' , &fsm[20] },   // 7
    { '\0' , NULL },      // 8
    { 'o' , &fsm[22] },   // 9
    { '\0' , NULL },      // 10
    { 'a' , &fsm[24] },   // 11
    { 'u' , &fsm[22] },   // 12
    { '\0' , NULL },      // 13
    { 'e' , &fsm[26] },   // 14
    { '\0' , NULL },      // 15
    { 'r' , &fsm[27] },   // 16
    { '\0' , NULL },      // 17
    // third letter
    { 'u' , &fsm[30] },   // 18
    { '\0' , NULL },      // 19
    { 'e' , &fsm[30] },   // 20
    { '\0' , NULL },      // 21
    { 'n' , &fsm[30] },   // 22
    { '\0' , NULL },      // 23
    { 't' , &fsm[30] },   // 24
    { '\0' , NULL },      // 25
    { 'd' , &fsm[30] },   // 26
    { '\0' , NULL },      // 27
    { 'i' , &fsm[30] },   // 28
    { '\0' , NULL },      // 29
    // first digit
    { '0' , &fsm[36] },   // 30
    { '1' , &fsm[35] },   // 31
    { '2' , &fsm[35] },   // 32
    { '3' , &fsm[46] },   // 33
    { '\0' , NULL },      // 34
    // second digit
    { '0' , NULL },       // 35
    { '1' , NULL },       // 36
    { '2' , NULL },       // 37
    { '3' , NULL },       // 38
    { '4' , NULL },       // 39
    { '5' , NULL },       // 40
    { '6' , NULL },       // 41
    { '7' , NULL },       // 42
    { '8' , NULL },       // 43
    { '9' , NULL },       // 44
    { '\0' , NULL },      // 45
    { '0' , NULL },       // 46
    { '1' , NULL },       // 47
    { '\0' , NULL },      // 48
};

int findWeekdayDate(const char *str) {
    const struct fsmNode *state = fsm;
    const char *curr;
    for (curr = str ; *curr; ++curr) {
        for ( ; state->target && *curr != state->target; ++state)
        { /* do nothing */ }
        if (state->next == NULL) {
            if (state->target) {
                // found a valid string
                return curr-str-4;
            } else {
                state = fsm;
            }
        } else {
            state = state->next;
        }
    }
    return -1;
}

// Test code follows...
static const struct test_s {
    const char *string;
    int expected;
} test[] = {
    { "abcSun24Def", 3},
    { "abcMun24Def", -1},
    { "abcSun091", 3},
    { "MON31", -1},
    { "Mon31", 0},
    { NULL, -1 }    // last item
};

#include <stdio.h>

#define RED    "\033[31m"
#define GREEN  "\033[32m"
#define WHITE  "\033[39m"

int main() {
    for (size_t i=0; test[i].string != NULL; ++i) {
        int val = findWeekdayDate(test[i].string);
        printf("%s: \"%s\", expected %d, got %d\n",
            (val == test[i].expected ? GREEN " OK" WHITE: RED "BAD" WHITE),
            test[i].string, test[i].expected, val
            );
    }
}

Comments on efficiency, style, performance, data structure choice and implementation are all of interest.

Nice work.

On findWeekdayDate() return, I'd expect a value that is enumerated like fWD_SUCCESS, fWD_FAIL, fWD_SUCCESS_for_foo_reason, fWD_FAIL_for_bar_reason, etc. Rather than magic numbers -1,0,3,....  It could end up being a simple bool.
As fsm[] is const struct fsmNode, I see no value in making a field const like const char target.  char target is simpler and sufficient.  OTOH, if there is some value, then next should also be const as in const struct fsmNode * const next.
Delay/remove #includes.

#include <string.h>  // Needed?
#include <stdlib.h>  // OK, for NULL
#include <ctype.h>   // Needed?

Style inconsistency.  Recommend empty for body to match other indentations.

// for ( ; state->target && *curr != state->target; ++state)
//{ /* do nothing */ }
for ( ; state->target && *curr != state->target; ++state) {
  /* do nothing */
}

Use an auto formatter.  The extra space betrays manual formatting as it is inconsistent with the rest of code.  Life is too short for manual formatting.

//             v
for (curr = str ; *curr; ++curr) {

Suggest adding link to software used to create FSM.


FizzBuzz example in Python for game manual I'm developing an implementation of the FizzBuzz game as part of an edutainment game for a retro video game platform. In its manual, I want to mention the history of writing a bot for the FizzBuzz game as a programmer screening test since Jeff Atwood's blog post, including sample answers in a few popular programming languages. But I don't want to confuse readers with poorly written code.

So is there anything unclear or unpythonic about my approach to FizzBuzz?

# Each of several divisors has a name. For each number,
# print the names of all divisors that divide the number,
# if any.  Otherwise print the number itself.
divs = [
    (3, "fizz"),
    (5, "buzz"),
]
for n in range(1, 101):
    # Make a string containing names of divisors of n
    ndivs = ''.join(name for (divisor, name) in divs
                    if n % divisor == 0)
    # If the string is empty, print the number instead
    print(ndivs or n)

Nicely done! The great thing about this implementation that it's easy to extend (for example if you want to add (7, "jazz"). I have only some minor nitpicks.

Tuples are immutable, as opposed to lists.
Since the list of fizz-buzz-jazz definitions don't change during the run of the program,
the divs list can be a tuple.
Tuples are faster than lists.

divs = (
    (3, "fizz"),
    (5, "buzz"),
)

The parentheses are unnecessary around (divisor, name):

ndivs = ''.join(name for (divisor, name) in divs
                if n % divisor == 0)

This may be a matter of taste,
but I'd find this line more readable on a single line:

ndivs = ''.join(name for divisor, name in divs if n % divisor == 0)


Return render in Ruby on Rails Is it bad to do return render? I am doing this to NOT also allow a redirect.

def create
    @patient = Patient.new(params[:patient])
    if  !@patient.save
      @patient.user.password = ''
      @patient.user.password_confirmation = ''
      return render is_admin? ? 'new_admin' : 'new'
    else
      #Fire off an e-mail
      PatientMailer.welcome_email(@patient).deliver
    end

    if current_user == @patient
      sign_in @patient
    else
      redirect_to patients_path
    end
  end

Writing it like this makes it look the render returns a meaningful value that is then returned by create and used by the code that calls create, which is not the case. So instead I would write:

render is_admin? ? 'new_admin' : 'new'
return

This makes it clear that render is solely used for its side effects and create does not return a value (other than nil).


read line endings sequentially This is my first C program and I'm just looking for some constructive criticism before I start trying to do more with it. Am in quite unfamiliar territory here and am uncertain if I'm releasing all my resources, handling errors, and whatnot "properly".

#include <errno.h>
#include <share.h>
#include <stdbool.h>
#include <stdio.h>

#define BUFFER_SIZE 4096
#define DATA_FILE_NAME "X"

static inline bool fillBuffer(FILE *, char [], size_t *, size_t *);
static inline bool nextCharEquals(FILE *, char [], size_t *, size_t *, char);

int main(void) {
    char buffer[BUFFER_SIZE];
    FILE *dataFile = NULL;
    size_t numBytesRead = 0;
    size_t numBytesUsed = 0;
    size_t statusCode = 0;

    dataFile = _fsopen(DATA_FILE_NAME, "rbS", _SH_DENYWR);

    if (!dataFile) { goto error; }

    while ((numBytesUsed < numBytesRead) || fillBuffer(dataFile, buffer, &numBytesRead, &numBytesUsed)) {
        char c = buffer[numBytesUsed++];

        if (('\n' == c) || ('\r' == c)) {
            if ((c == '\r') && nextCharEquals(dataFile, buffer, &numBytesRead, &numBytesUsed, '\n')) {
                numBytesUsed++;
            }

            continue; // TODO: something useful...
        }
    }

error:
    if (errno) {
        strerror_s(buffer, BUFFER_SIZE, errno);
        fprintf(stderr, "%s\n", buffer);

        statusCode = -1;
    }

exit:
    if (dataFile) {
        fclose(dataFile);
    }

    return statusCode;
}

static inline bool fillBuffer(FILE *file, char buffer[], size_t *numBytesRead, size_t *numBytesUsed) {
    *numBytesRead = fread(buffer, 1, BUFFER_SIZE, file);
    *numBytesUsed = 0;

    return (0 < *numBytesRead);
}
static inline bool nextCharEquals(FILE *file, char buffer[], size_t *numBytesRead, size_t *numBytesUsed, char value) {
    return (((*numBytesUsed < *numBytesRead) || fillBuffer(file, buffer, numBytesRead, numBytesUsed)) && (buffer[*numBytesUsed] == value));
}

static inline bool fillBuffer(FILE *, char [], size_t *, size_t *);

It'd be nice to provide names for these function parameters, especially since two of them have the same type (size_t *).

It's fairly unusual to see a pointer parameter declared with [] instead of * — I infer your rationale, something like "This pointer is expected to point to an array of contiguous objects, not just one object," but the inconsistency (and risk of some maintainer forgetting and writing sizeof buffer just once) is too costly IMHO. The fact that char *buffer is immediately followed by size_t bufferlen should be a dead giveaway that buffer points to an array.

...Oh, what do you mean char *buffer isn't followed by size_t bufferlen?!

static inline bool fillBuffer(FILE *file, char buffer[], size_t *numBytesRead, size_t *numBytesUsed) {
    *numBytesRead = fread(buffer, 1, BUFFER_SIZE, file);

Yep, that's a buffer overflow waiting to happen. Your fillBuffer function takes a pointer (buffer) and unconditionally writes up to BUFFER_SIZE (4096) bytes into it. If the caller passes in a smaller buffer... boom.
So what we want to see here is more like the API to fread itself:

static inline bool fillBuffer(FILE *file, char *buffer, size_t bufferlen, size_t *numBytesRead, size_t *numBytesUsed) {
    *numBytesRead = fread(buffer, 1, bufferlen, file);
    *numBytesUsed = 0;

    return (0 < *numBytesRead);
}

I know that fread returns numBytesRead as its return value, so I'm suspicious of the way fillBuffer returns numBytesRead via an out-parameter. Out-parameters are a code smell. So I look at what we're using our return value for.

    return (0 < *numBytesRead);

Comparing a size_t against 0 should generally be done with !=, not <, since the only value of type size_t that is not > 0 is 0 itself.

    return (0 != *numBytesRead);

But this function returns bool, and size_t converts to bool in the obvious way, so:

    return *numBytesRead;

Oh look! We just eliminated that out-parameter!

static inline size_t fillBuffer(FILE *fp, char *buffer, size_t bufferlen, size_t *numBytesUsed) {
    size_t numBytesRead = fread(buffer, 1, bufferlen, fp);
    *numBytesUsed = 0;
    return numBytesRead;
}

Now simplify:

static inline size_t fillBuffer(FILE *fp, char *buffer, size_t bufferlen, size_t *numBytesUsed) {
    *numBytesUsed = 0;
    return fread(buffer, 1, bufferlen, fp);
}

Now pause. Notice that numBytesUsed is not actually used by this function at all, except to unconditionally set it to 0. The Single Responsibility Principle suggests that perhaps setting this arbitrary size_t to 0 could be done by the caller, so as not to bother us with an essentially unrelated task. But then our whole function becomes a tail-call to fread(...), which seems ridiculous. Let's eliminate this function. Find its caller...

static inline bool nextCharEquals(FILE *file, char buffer[], size_t *numBytesRead, size_t *numBytesUsed, char value) {
    return (((*numBytesUsed < *numBytesRead) || fillBuffer(file, buffer, numBytesRead, numBytesUsed)) && (buffer[*numBytesUsed] == value));
}

Oh dear me. Let's rewrite this mess using if and else:

static inline bool nextCharEquals(FILE *file, char buffer[], size_t *numBytesRead, size_t *numBytesUsed, char value) {
    if (*numBytesUsed >= *numBytesRead) {
        if (fillBuffer(file, buffer, numBytesRead, numBytesUsed) == 0) {
            return false;
        }
    }
    return buffer[*numBytesUsed] == value;
}

Okay, now for the inlining step!

static inline bool nextCharEquals(FILE *fp, char *buffer, size_t bufferlen, size_t *numBytesUsed, char value) {
    if (*numBytesUsed >= *numBytesRead) {
        *numBytesRead = fread(buffer, 1, bufferlen, fp);
        *numBytesUsed = 0;
        if (*numBytesRead == 0) {
            return false;
        }
    }
    return buffer[*numBytesUsed] == value;
}

Now we can start to make out where the real problems lie. For example, it is tempting to observe that if (*numBytesRead == 0) is equivalent to if (*numBytesUsed >= *numBytesRead) (since we just set *numBytesUsed to 0 on the previous line). So we are tempted to rewrite this function as

static inline bool nextCharEquals(FILE *fp, char *buffer, size_t bufferlen, size_t *numBytesUsed, char value) {
    while (*numBytesUsed >= *numBytesRead) {
        *numBytesRead = fread(buffer, 1, bufferlen, fp);
        *numBytesUsed = 0;
        // TODO: check errno? check feof(fp)?
    }
    return buffer[*numBytesUsed] == value;
}

Of course as written that would be an infinite loop on EOF; but it makes us ponder in what situations we expect to see fread(...) == 0... which starts us thinking about error handling.

Meanwhile, we can apply the Single Responsibility Principle again to figure out that perhaps char value doesn't belong in this function. Why would our caller want to write

nextCharEquals(dataFile, buffer, &numBytesRead, &numBytesUsed, '\n')

when they could write

nextChar(dataFile, buffer, &numBytesRead, &numBytesUsed) == '\n'

instead?

Any time you're spelling out the name of a built-in operator (==) in your function name, it's a good sign that your function is doing more than it should be. Let the built-in operator pull its own weight!

One more random tip: Instead of hard-coding BUFFER_SIZE all over your program, try to use sizeof buffer.

char buffer[BUFFER_SIZE];
foo(buffer, BUFFER_SIZE);  // bad, repetitive, easy to typo

char buffer[4096];
foo(buffer, sizeof buffer);  // good, clean, hard to typo

It'll also train you to pass your buffer sizes around properly, since you won't have all your buffers' sizes so promiscuously available in "global variables." And that will help you prepare for the world of multi-source-file programs.


File-to-fully qualified class name function My instincts are telling me a seasoned Scala programmer will find my code less than optimal. As will no doubt be obvious, I'm a Java guy who has just started in with Scala. Is there a more 'functional' way to accomplish this that I'm missing?

/**
   * takes a file object and returns a string representing the fully qualified
   * class name represented by that file.
   *
   * @param file
   * @return string representation of the fully qualified class name
   */
  def makeFqClassName(file: File): String = {
    // pull out the file name - we'll use this for our class name
    val fNameNoExtS = FilenameUtils.removeExtension(file.getName)

    // extract only the path
    val fPathOnlyS =
      file.getPath.substring(0, file.getPath.lastIndexOf(File.separator))

    // create a list of the path name, minus the delimiter
    val fPathDelimList: List[String] = fPathOnlyS.split("/").toList

    // truncate the list to include on the class name info
    val fqNameList: List[String] =
      fPathDelimList.drop(fPathDelimList.lastIndexOf("solutions"))

    // convert the list to a string representation of a class name and return
    // to caller
    fqNameList.mkString(".") + "." + fNameNoExtS
  }

Expected input: .java file object where the file name and the class name enclosed in the file are the same.

  Sp01.scala

Expected output: fully qualified class name:

  solutions.working_with_lists.Sp01

Note that if we were working with one of the problems from another group, the class name would be something like:

  solutions.arithmetic.Sp31

The reason for all this is that I'm new to Scala and working my way through the 99 Scala problems set. This method is part of a test runner that trolls through the project files, picks up solution files, then uses reflection to instantiate an instance of the solution class and executes its solver method. This way I can organize the project files in a way that makes sense - in a way that matches the manner in which the problem set is structured - as well as work on whatever problem I want to work on while simultaneously giving me the freedom to reorganize the package structure w/o having to refactor the runner code.

My current assumption is that the string returned by file.getPath is going to look something like ..\solutions\some-parent-dir\some-scala-file.scala

If this is the case then you can try the following function:

def makeFqClassName(file: File): String = {
  val fileName :: _ = file.getName.split('.').toList
  val upDir :: upUpDir :: _ = file.getParent.split(File.separator(0)).toList.reverse
  s"$upUpDir.$upDir.$fileName"
}

I've introduced a smidgen of strange syntax just to show you a possibility. As a way of explaining what is going on, consider that the following are functionally equivalent:

  val foo :: _ = List(0, 1, 2, 3)
  val goo = List(0, 1, 2, 3).head

From the POV of operations performed they're unequivalent.
foo is defined from the result of a pattern match that discards the tail of the list. goo as you know is the result of the invocation of the head method. (I suppose it could be the case that the compiler is smart enough to translate a pattern match on a list that only takes the head of the list into an invocation of the head method, but lets just ignore that for now :^) And a final example:

val a :: b :: c :: d :: ez = ('a' to 'z').toList

Here a , b , c , and d  are assigned the expected character values and ez is a list consisting of the characters e through z.

If I've misunderstood what your asking for please let me know and I'll update this accordingly.


Check whether '.' and '/' are escaped properly I'm currently using a function to check if a URL has correctly escaped two different characters: . and /.
To escape a character within a string, it requires 2 preceding \'s in the string (i.e.: \\. or \\/).
Here is the code I'm currently using. As you can see I'm matching the string with some regex, and then for every element returned I check if the first character of the string is \.
I feel this could be simplified, how can I make this better and avoid looping through all elements returned by match?

function isValidEscapedString(value) {
  return (value.match(/.?[./]/g)||[]).every((escape) => escape[0] === '\\');
}

console.log(isValidEscapedString('')); // true
console.log(isValidEscapedString('https://stackoverflow.com')); // false
console.log(isValidEscapedString('https:\/\/stackoverflow\.com')); // false
console.log(isValidEscapedString('https://stackoverflow\\.com')); // false
console.log(isValidEscapedString('https:\\/\\/stackoverflow\\.com')); //true

The examples in the code are working as intended.

What about, instead of search for every . and / and check the predecessor character, you write a regexp that find a . and / that don't have \\ before?
This way, you can avoid the loop and the short circuit to an array

function isValidEscaped(str) {
  const pattern = /[^\\]{2}[./]/g;
  return str.search(pattern) === -1;
}


Convert Set to JSONArray in Java 8 Can this be optimized for Java 8 and can it be more efficient? I am using streams and lambdas.

public String LanguageString(Set<Locale> languageSet) throws Exception {
JSONObject json = new JSONObject();
JSONObject tempj = new JSONObject();
JSONArray jArr = new JSONArray();
try {
  for (Locale locale : languageSet) {
    if (locale != null) {
      tempj = new JSONObject();
      tempj.put("lcode", locale.toLanguageTag());
      tempj.put("ldisplay", locale.getDisplayName());
      jArr.put(tempj);
    }
  }
  json.put("root", jArr);
} catch (JSONException e) {
  //
}
return json.toString();
}

You should not catch exceptions and discard them silently. This code has no chance of throwing an exception, therefore you should just let the exception bubble up and let someone else catch it. If it should ever happen.
Your variable names don't express their intention. The word temp should not be used at all, you mix terminology between languages and locale. Sometimes you put the j at the beginning of the name, sometimes at the end; this is inconsistent.
Declare your variables as late as possible (json).
Declare your variables in the smallest possible scope (tempj).
Method names in Java start with a lowercase letter.
Don't declare throws Exception when your code doesn't actually do that.
It is unusual that the languages set actually contains null. So in general you should leave out the if (language != null) check.

After following all these hints, the code may look like this:

public String createLanguagesJson(Set<Locale> languages) {
    JSONArray array = new JSONArray();
    for (Locale language : languages) {
        array.put(new JSONObject()
                .put("lcode", language.toLanguageTag())
                .put("ldisplay", language.getDisplayName()));
    }

    return new JSONObject()
            .put("root", array)
            .toString();
}


Motorcycle rental station with flyweight pattern The definition of flyweight pattern:

  Flyweight design pattern uses sharing to support large numbers of objects that have part of their internal state common and other parts of state can vary.

Please let me know whether I implemented this pattern correctly. I also want to know whether the map structure is always needed when implement this pattern. Anything related to the pattern is also welcomed.

Bike: Define the interface for clients to ride the bike at the speed they want.
Motorcycle: Implementation of Bike. Has internal state BikeType, which is decided when created.
BikeType: Two types of bikes

Scooter
Sports bike

FlyweightFactory: The bike rental store. I chose this name to make it clear of the role it plays in the flyweight pattern.

Factory is a singleton, only one factory.
Choose the BikeType and get the bike.

Bike.java

public interface Bike
{
    // speed is external state.
    public void rideBike(int speed);
}

Motorcycle.java

public class Motorcycle implements Bike
{
    // BikeType is internal state.
    private BikeType bikeType;

    public Motorcycle(BikeType bikeType) {
        this.bikeType = bikeType;
    }

    @Override
    public void rideBike(int speed) {
        System.out.format("Riding %s at speed %d mps%n",
            getBikeType().getTypeName(),
            speed);
    }

    //
    private BikeType getBikeType() {
        return this.bikeType;
    }
}

BikeType.java

public enum BikeType
{
    SPORTS("Sports Bike"), SCOOTER("Scooter");

    private String typeName;

    private BikeType(String typeName) {
        this.typeName = typeName;
    }

    public String getTypeName() {
        return this.typeName;
    }
}

FlyweightFactory.java

I implemented the flyweight rental store with singleton pattern:

public class FlyweightFactory
{
    // static fields & methods
    private static volatile FlyweightFactory factory;

    public static FlyweightFactory getFactory() {
        if (factory == null) {
            synchronized (FlyweightFactory.class) {
                if (factory == null) {
                    factory = new FlyweightFactory();
                }
            }
        }

        return factory;
    }

    // Instance fields & methods
    private Map<BikeType, Bike> bikes;

    private FlyweightFactory() {
        this.bikes = new HashMap<>();
    }

    public Bike getBike(BikeType bikeType) {
        Bike bikeReturn = getBikes().get(bikeType);

        if (bikeReturn == null) {
            bikeReturn = new Motorcycle(bikeType);
            getBikes().put(bikeType, bikeReturn);
        }

        return bikeReturn;
    }

    // setters & getters
    private Map<BikeType, Bike> getBikes() {
        return this.bikes;
    }
}

App.java

Example program to test the flyweight factory works well:

public class App
{
    public static void main(String[] args) {

        FlyweightFactory ff = FlyweightFactory.getFactory();

        Bike sportBike = ff.getBike(BikeType.SPORTS);
        System.out.println(sportBike);
        System.out.println(ff.getBike(BikeType.SPORTS));
        sportBike.rideBike(20);

        System.out.println(" --- ");

        Bike scooter = ff.getBike(BikeType.SCOOTER);
        System.out.println(scooter);
        scooter.rideBike(30);
    }
}

Output:

Motorcycle@7c17831
Motorcycle@7c17831
Riding Sports Bike at speed 20 mps
 ---
Motorcycle@26cf402f
Riding Scooter at speed 30 mps

The flyweight pattern exists to minimize memory usage. I don't think it is what you need here. At least, wha you have done looks like strategy.

But because your are asking about the flyweight let me try to explain a good usage of it. The goal of the flyweight is to reduce memory usage. The idea is to have something like a date stamp that you can reuse to print differents dates. You take your stamp, roll the numbers to have the expected date, apply it and then forgot or use the same for another date.

public class DateStamp {
  private int year, month, day;
  // setters
  public void apply() {
    System.out.printf("%1$d/%2$d/%3$d", year, month, day);
  }
}

// ..
DateStamp stamp = new DateStamp();
stamp.set..
stamp.apply();

stamp.set..
stamp.apply();

In many systems today, this pattern is rarely used due to the hughe amount of availables memory. Creating new instances is almost no cost.
But when the creation of a new instance can be costly or when you have hundreds instances of the same type at the same time it can be interesting to use it.

About your code and implementation of the factory, there is nothing wrong with the map. It is the best structure to keep a "relation" between two things.

Another implementation, would be to use an abstract Bike as explained by BKSpurgeon.