File parser to Java type design The basic requirement of my code is to accept a file input, parse it, convert it to a java type (A DTO essentially) for use in another component of the system.

I should also note that I am not actually parsing a file from a filesystem, the FileParser will be used by a Message Driven Bean that has the contents of the file as a String already.

I am looking for input and general design/code review, thanks for your time.

I have 4 classes:

FileParser
Message
MessageFactory
MessageFieldTypes

The format of the file is as follows:

FIELDONETYPE/FIELDONEVALUE
FIELDTWOTYPE/FIELDTWOVALUE
FIELDTHREETYPE/FIELDTHREEVALUE
FIELDFOURTYPE/FIELDFOURVALUE
FIELDFIVETYPE/FIELDFIVEVALUE

etc.

There are roughly 20 fields.

My initial thought was to parse the file into a hashmap and then create the object. Here is the code listing for the parser:

final public class FileParser
{
    public static final Message parse(String text) {

    Map<MessageFieldTypes, String> map = new LinkedHashMap<MessageFieldTypes, String>();

    for(String line : text.split(" *\n *")) {
        String[] keyValuePair = line.split(" */ *");
        map.put(MessageFieldTypes.valueOf(keyValuePair[0]),
                keyValuePair.length == 1 ? "" : keyValuePair[1]);
    }

    return MessageFactory.createFromHashMap(map);
    }
}

Here is the code listing for the factory:

public class MessageFactory {

    private static final DateTimeFormatter DATE_TIME_FORMATTER = DateTimeFormat.forPattern("ddMMMyyyy");

    public static Message createFromHashMap(Map<MessageFieldTypes, String> map) {
        Message message = new Message();

        message.setFieldOne(new FieldOneType(map.get(MessageFieldTypes.FIELDONE),
                            map.get(MessageFieldTypes.FIELDTWO),
                            DATE_TIME_FORMATTER.parseDateTime(map.get(MessageFieldTypes.FIELDTHREE)).toLocalDate()));
        message.setFieldFour(new FieldFourType(map.get(MessageFieldTypes.FIELDFOUR)));
        message.setFieldFive(map.get(MessageFieldTypes.FIELDFIVE)));

        return message;
    }
}

And the Enum:

public enum MessageFieldTypes {
    FIELDONE,
    FIELDTWO,
    FIELDTHREE,
    FIELDFOUR,
    FIELDFIVE
}

If you can change your file format slightly (to type=value or type:value) I'd suggest using the Java Properties class. It can parse the file from a stream for you (Untested code):

final public class FileParser
{
    public static final Message parse(String text) {
        text = text.replace("/", ":");
        Properties typeValues = new Properties();
        typeValues.load(new StringReader(text));

        return MessageFactory.createFromHashTable(typeValues);
    }
}

Note that Properties extends HashTable, not HashMap. They chose HashTable because HashMap is not thread-safe. They work pretty much the same way, though.

On structure: I like the factory format, and it makes sense to use it here. You didn't mention whether the field types are known; if they are your factory code is fine. (It looks like they are, since you call several different constructors with fixed numbers of arguments.)


Selecting kids for a Christmas play with similar heights I am doing this problem on SPOJ:

  My kid's kindergarten class is putting up a Christmas play.  (I hope he gets the lead role.)  The kids are all excited, but the teacher has a lot of work.  She has to produce costumes for a scene with \$K\$ soldiers.  She wants to buy all the costumes in the same size, allowing for some small amount of length alteration to be done by the kids' parents later.  So she has taken all the kids' height measurements.  Can you help her select \$K\$ kids from her class of \$N\$ to play the soldier role, such that the height difference between the tallest and shortest in the group is minimized, and alternations will be easiest?  Tell her what this minimum difference is.

  INPUT

  The first line contains the number of test cases \$T\$. \$T\$ test cases follow each containing 2 lines.

  The first line of each test case contains 2 integers \$N\$ and \$K\$.
  The second line contains \$N\$ integers denoting the height of the \$N\$ kids.

  OUTPUT

  Output \$T\$ lines, each line containing the required answer for the corresponding test case.

  CONSTRAINTS

  \$T \le 30\$
  \$1 \le K \le N \le 20000\$
  \$1 \le \text{height} \le 1000000000\$

My approach

I am first storing all the heights in the array and then sorting the array using quicksort (I also tried std::sort), and after that finding the minimum difference using sliding window (I do not exactly know the name of algorithm, I heard it is known as sliding window).

After optimizing my code to the best that I can, my code takes 0.05s to execute all test cases, but when I see the best result, it is 0.00s or 0.02s, so less than 0.05s.

By the way, I had also tested with std::sort but it was only marginally faster (0.05 s instead of 0.06 s). How can I optimize it more?

#include <cstdio>
#include <algorithm>
#include<iostream>
int partition(int *arr, const int left, const int right) {
const int mid = left + (right - left) / 2;
const int pivot = arr[mid];
// move the mid point value to the front.
std::swap(arr[mid],arr[left]);
int i = left + 1;
int j = right;
while (i <= j) {
    while(i <= j && arr[i] <= pivot) {
        i++;
    }

    while(i <= j && arr[j] > pivot) {
        j--;
    }

    if (i < j) {
        std::swap(arr[i], arr[j]);
    }
}
std::swap(arr[i - 1],arr[left]);
return i - 1;
}

void quicksort(int *arr, const int left, const int right, const int sz)
{

if (left >= right) {
    return;
}

int part = partition(arr, left, right);

quicksort(arr, left, part - 1, sz);
quicksort(arr, part + 1, right, sz);
}

using namespace std;

int main(){
int T,N,K,h[20000];

scanf("%d",&T);

while(T--){
    scanf("%d %d",&N,&K);

    for(int i = 0;i < N;++i) scanf("%d",&h[i]);
    quicksort(h,0,N-1,N);

    int ans = h[K - 1] - h[0];

    for(int i = 1;i + K - 1 < N;++i)
        ans = min(ans,h[i + K - 1] - h[i]);

    printf("%d\n",ans);
}

return 0;
}

Time is mostly spent in I/O

I played with your program and found that most of the time was spent doing I/O.  However, from your comments, it appears you already improved your I/O so that your best solution is now 0.02 seconds instead of 0.05 seconds.

I took your improved I/O code and took it one step further.  What I did was read the entire input into a buffer using a single fread(), and then instead of using getchar_unlocked(), I just advanced a pointer through the buffer using *p++.

Why is *p++ faster than getchar_unlocked()?  Here is what getchar_unlocked() does (not exactly but something close to this):

char getchar_unlocked()
{
    if (stdin.ptr >= stdin.buffer_len) {
        refill_stdin_buffer();
        stdin.ptr = 0;
    }
    return stdin.buffer[stdin.ptr++];
}

So for every character you read, it does a check to see if it needs to reload the buffer.  If you read the entire input from stdin to a local buffer, you are essentially skipping and end of buffer check for each character you read.

Update: Later I found that my I/O changes didn't matter.  Both yours and mine were able to achieve 0.00 time, as long as radix sort was used (see next section).

Radix sort

Radix sort is 0.02 seconds faster than std::sort for this test.  I know because I tried it both ways.  First I tried radix sort, which took 0.00 seconds overall.  Then I replaced radix sort with std::sort and got 0.02 seconds.

In general, radix sort should be faster than quicksort for large arrays of integers.  The radix sort I used operates on bytes at a time, so for 32-bit integers it uses exactly 4 rounds.  On each round it does a counting pass followed by a rearranging pass.  So in total, it does approximately \$8n\$ operations to sort an array of \$n\$ integers, plus a fixed overhead of 1024 operations to deal with the buckets.

From my own testing, I found that radix sort is 5x faster than std::sort for sorting arrays of 10 million integers.  For 20000 integers, it is around 4x faster.

The final program, which achieved 0.00 seconds

Here is my program.  You can see in the SPOJ status that I got 0.00 seconds with it.  FYI, my first entry was a C program that got 0.00 seconds, but I changed it to the following C++ program:

#include <bits/stdc++.h>
#include <cstdio>

#define MAX                20000

// 30 testcases, 11 characters per number, 20000 numbers
char buf[MAX*11*30];
int array    [MAX];
int arrayTemp[MAX];

static inline int readInt(char **pPtr)
{
    char *p   = *pPtr;
    int   ret = 0;
    char  c   = *p++;

    while (c < '0')
        c = *p++;
    do {
        ret = (ret << 3) + (ret << 1) + c - 48;
        c   = *p++;
    } while (c >= '0');

    *pPtr = p;
    return ret;
}

void radixsort(int *a, int len)
{
    int *space       = arrayTemp;
    int *current     = a;
    int *scratch     = space;
    int *tmp         = NULL;
    int  radixByte   = 0;
    int  i           = 0;
    int  bucket[256];

    // Iterate once per byte.
    for (radixByte=0;radixByte<4;radixByte++) {
        int shift = (radixByte << 3);
        memset(bucket, 0, sizeof(bucket));

        // Count how many of each byte.
        for (i=0;i<len;i++)
            bucket[(current[i] >> shift) & 0xff]++;

        // Change bucket to be cumulative count.
        for (i=1;i<256;i++)
            bucket[i] += bucket[i-1];

        // Copy from current to scratch, using bucket counts.
        for (i=len-1;i>=0;i--)
            scratch[--bucket[(current[i] >> shift) & 0xff]] = current[i];

        // Switch arrays
        tmp     = current;
        current = scratch;
        scratch = tmp;
    }
}

int main(void)
{
    char *p = buf;
    fread(buf, 1, sizeof(buf), stdin);

    int t = readInt(&p);
    while (t-- > 0) {
        int n = readInt(&p);
        int k = readInt(&p);

        for (int i = 0; i < n; i++)
            array[i] = readInt(&p);

        if (k == 1) {
            puts("0");
            continue;
        }

        radixsort(array, n);

        int diff = INT_MAX;
        n -= k;
        k--;
        for (int i = 0; i <= n; i++) {
            int newDiff = array[i+k] - array[i];
            if (newDiff < diff)
                diff = newDiff;
        }
        printf("%d\n", diff);
    }
    return 0;
}


Return list with numbers of color occurrences in another list I'm solving "mastermind game" problem from CIS haskell course. Here is the link to assignment pdf if someone is interested.

List of possible colors is defined like this:

data Peg = Red | Green | Blue | Yellow | Orange | Purple
     deriving (Show, Eq, Ord)
type Code = [Peg]
colors :: [Peg]
colors = [Red, Green, Blue, Yellow, Orange, Purple]

And I had to write a function that will take a list with color guesses and return the occurrence count for each available color in the form of a list.

countColors :: Code -> [Int]
countColors a        = map (\(x,y) -> length $ filter (x==) y) combo
      where combo    = zip colors (take 6 $ repeat a)

It works fine but I'd like to know if there is better/more expressive way to write it (using most basic available functions).

I'm not sure what's good practice in Haskell; that is my main problem. I have tried to write in point free notation at first but it looked overly complicated.

My worry is that code is not clear enough. I like to have code that is as obvious as possible.

Here are test cases in case it helps:

countColors [Red, Blue, Yellow, Purple] == [1, 0, 1, 1, 0, 1]
countColors [Green, Blue, Green, Orange] == [0, 2, 1, 0, 1, 0]

I think you're tripping yourself up by position-encoding the colors. Position-encoding is usually a code smell in Haskell, what you probably want is an association. Consider the difference between [Int] and [(Peg, Int)], and just how much more expressive of intent the latter is.

(You could be even more specific at the type level with something like type Count = Int countColors :: Code -> Map Peg Count but that's probably excessive for a small game.)

So then how do you write a function with type Code -> [(Peg, Int)]? As you noticed the [Peg] may be in any order, so to compensate you can filter for each individual color, or you could leverage the Ord Peg instance and sort the original list to group identical colors together. After the colors are in order, what's left is to simply run-length encode the list.

countColors :: Code -> [(Peg, Int)]
countColors = map (\shade -> (head shade, length shade)) . group . sort


Searching for a sum of any series in array I'm given this method, and requested to analyze what it does, and minimize its complexity (both time and space).
All values are known to be larger than zero.

public static boolean search(int[] a, int num) {
    for(int i = 0; i < a.length; i++)
        for(int j = i; j < a.length; j++) {
            int res = 0;
            for(int k = i; k <= j; k++)
                res += a[k];

            if(res == num)
                return true;
        }
    return false;
}

From what it looks to me it's searching for any series of consecutive numbers in the given array, that sum up to num, with an O(n3) complexity.

I've tried to rewrite it like this:

public static boolean search(int[] a, int num) {
    for (int i = 0; i < a.length; i++) {
        int sum = 0;
        for (int j = i; j < a.length; j++) {
            sum += a[j];
            if (sum == num)
                return true;
        }

        if (num > sum)
            break;
    }
    return false;
}

Which has a complexity of  (BTW, is there a better way to denote this complexity?).

What other strategies can I take to minimize the complexity even further? Can I reach O(n)?

Both the original and your revision have the same problem.  They reset sum/res to zero on every iteration and then recalculate it.  To avoid this, store the sums in an array.

int[] sums = new int[a.length + 1];
sums[0] = 0;
for (int i = 0; i < a.length; i++) {
    sums[i + 1] = sums[i] + a[i];
}

This takes \$\mathcal{O}(n)\$ time to calculate.

For any i, sums[i] is the sum of the first i elements of the array.

Since we know that all values are positive, adding a value always increases the sum.  And removing a value decreases it.  So how do we use that?  Consider

int left = 0;
int right = 0;
while (right < sums.length) {
    int sum = sums[right] - sums[left];
    if (sum == target) {
        return true;
    }

    if (sum > target) {
        // if the sum is too big, reduce it by dropping a value from it
        left++;
    } else {
        // if the sum is too small, increase it by adding a value to it
        right++;
    }
}

I changed the name of num to target as being more descriptive of what it represents.

This uses the sums stored in our array to calculate the sum from left + 1 to right in the original array.

On each iteration of the loop, we either return or increment one of the indexes.  The worst case is where the sum of the elements before the last is too small but the last element itself is equal to or larger than the target.  In that case, we increment right sums.length times and we increment left sums.length times.  That's \$2n + 2\$ total, which is \$\mathcal{O}(n)\$.

This takes \$\mathcal{O}(n)\$ space as well.  If you need constant space, you can skip the first part and track the sum as an integer.  See here.

If you can't see the deleted answer from mtj, it basically says to do something like

int left = 0;
int right = 0;
int sum = 0;
while (right < a.length) {
    if (sum > target) {
        // if the sum is too big, reduce it by dropping a value from it
        sum -= a[left];
        left++;
    } else {
        // if the sum is too small, increase it by adding a value to it
        sum += a[right];
        right++;
    }

    if (sum == target) {
        return true;
    }
}

return false;

Assumes that target and all the values in a are positive.

You can see a solution to a similar problem here as well.


FizzBuzzWoofFooBar - follow-up I took all your suggestions to heart and I think it improved the code a lot.

The Map got it's own typedef: IntStrDict. It's a perfectly applicable name, but I can't help but feel it's not according to best practices.

The code now accepts arguments. If no arguments are given, the code fails. If one or more argument is given it accepts the first argument as number and ignores the rest. If a non-number is inserted as first argument it should break. Recommended argument is 15016 (the result of 3*5*7*11*13).

I'm not sure my FizzDict in main() is according to best practices either. I'm still looking for a way to make it more generic. Perhaps OO-style with a getFizzDict would be appropriate. An argument which allows you to choose between dictionaries would be fancy, but could get very ugly very fast as well.

Another difference is I finally split up the code into a .cpp and a .h. Many functions are split up now.

Is my exception handling up to standards?

I'm trying to follow SRP as much as sensible.

FizzBuzz.cpp

#include "FizzBuzz.h"

int main(int argc, char* argv[])
{
    int iMax;
    try {
        iMax = atoi(argv[1]);
    }
    catch (int e) {
        std::cout << "Exception " << e << '\n';
    }
    if (argc < 2) {
        throw 0;
    }

    const static IntStrDict FizzDict = {
        { 3, "Fizz" },
        { 5, "Buzz" },
        { 7, "Woof" },
        { 11, "Foo" },
        { 13, "Bar" }
    };

    for (int i = 1; i < iMax; ++i)
    {
        printString(FizzBuzzer(i, FizzDict));
    }
}

FizzBuzz.h

#ifndef _fizzbuzz_h_
#define _fizzbuzz_h_

#include <iostream>
#include <string>
#include <map>

typedef std::map<int, std::string> IntStrDict;

void printString(std::string tString)
{
    std::cout << tString << '\n';
}

std::string ifEmpty(int i, std::string tString)
{
    if (tString.empty())
    {
        tString += std::to_string(i);
    }
    return tString;
}

std::string checkModZero(int i, IntStrDict tDict)
{
    std::string tString;
    for (auto const &j : tDict)
    {
        if (i % j.first == 0)
        {
            tString += j.second;
        }
    }
    return tString;
}

std::string FizzBuzzer(int i, IntStrDict tDict)
{
    std::string print;

    print = checkModZero(i, tDict);
    print = ifEmpty(i, print);
    return print;
}

#endif // fizzbuzz

Here are some things I see that could help you improve your code.

Provide some guidance for the user

The user of the program might not know that this is expecting a numeric argument.  Instead of throwing an exception (and by the way, neither atoi nor std::atoi throw), you might instead simply check for the number of arguments like this:

if (argc < 2) {
    std::cout << "Usage: " << argv[0] << " maxint\n";
    return 0;
}

Provide a reasonable default argument

As you've noted in your comments, a reasonable default would be the product of all of the numbers.  That is, 3 * 5 * 7 * 11 * 13, but that's actually 15015 and not 15016 as your comments indicate.  Why not let the computer supply that?  Here's one way to do that:

const static int defaultMax = std::accumulate(
        FizzDict.cbegin(),
        FizzDict.cend(),
        1,
        [](int a, const std::pair<const int, std::basic_string<char> >& b){
            return a*b.first;
        }
      );
iMax = defaultMax;
if (argc > 1)
    iMax = std::stoi(argv[1]);

for (int i = 1; i <= iMax; ++i)
{
    printString(FizzBuzzer(i, FizzDict));
}

Note that the lambda could be shortened by making the second argument decltype(*FizzDict.cbegin()) b but we still need to know that it's a std::pair so that we can get the first element (integer), so it's not clear that it's really a win.

Separate interface from implementation

The interface goes into a header file and the implementation (that is, everything that actually emits bytes including all functions and data) should be in a separate .cpp file.  The reason is that you might have multiple source files including the .h file but only one instance of the corresponding .cpp file.  In other words, split your existing FizzBuzz.h file into a .h file and a .cpp file.

Make sure you have all required #includes

The code uses atoi but doesn't #include <cstdlib>.  Also, carefully consider which #includes are part of the interface (and belong in the .h file) and which are part of the implementation per the above advice.

Encapsulate helper functions within a class

Right now, there's probably little reason to use ifEmpty or checkModZero outside the context of your FizzBuzzer.  To that end, you could make those private member functions of a FizzBuzzer class and have your FizzBuzzer function be a public member function instead.  I'd probably either have the FizzDict map be a static member of that class or maybe an argument to the constructor.


Async Task with multiple requests in android I am using this AsyncTask class to update two different tables on SQL Server. So far this code works fine, I'm interested in a better and more sufficient code structure for this class, specially in doinbackground().

Is it okay to call multiple webservice methods in a single thread? Any other feedback?

    private class Update extends AsyncTask<Void, Void, Integer> {
    private final int FAILED_INVALID_RESPONSE = 0;
    private final int SUCCESS_GET_DATA = 1;
    ProgressDialog progress;
    private String _phoneno;
    private String _ticket;
    UpdateTicket(String phoneno,String ticket){
        _phoneno=phoneno;
        _ticket=ticket;
    }
    @Override
    protected void onPreExecute() {
        super.onPreExecute();
        progress = ProgressDialog.show(XYZ.this, "",
                "In Progress...", false);
    }

    @Override
    protected Integer doInBackground(Void... params) {
        method1(_phoneno);
        return  method2(_phoneno,_ticket);
    }
    @Override
    protected void onPostExecute(Integer result) {
        progress.dismiss();
        switch (result) {
            case FAILED_INVALID_RESPONSE:
                Toast.makeText(XYZ.this,"Please Check your Internet Connection.",Toast.LENGTH_SHORT).show();
                break;
            case SUCCESS_GET_DATA:
                Toast.makeText(XYZ.this, "Success!", Toast.LENGTH_SHORT).show();
                break;
        }
    }
    int  method1(String phoneno,String tickets)
    {
        final  String methodname = "firstmethod";
        final  String NAMESPACE ="http://tempuri.org/";
        final  String URL="www.sampleurl.com";
        final  String SOAP_ACTION="http://tempuri.org/firstmethod";
        int success=0;
        StrictMode.ThreadPolicy policy = new StrictMode.ThreadPolicy.Builder().permitAll().build();
        StrictMode.setThreadPolicy(policy);
        SoapObject request = new SoapObject(NAMESPACE, methodname);
        SoapSerializationEnvelope envelope = new  SoapSerializationEnvelope(SoapEnvelope.VER11);
        envelope.dotNet = true;
        request.addProperty("phoneno", phoneno);
        request.addProperty("tickets", tickets);
        envelope.setOutputSoapObject(request);

        HttpTransportSE androidHttpTransport = new HttpTransportSE(URL);
        try{
            androidHttpTransport.call(SOAP_ACTION,envelope);
            SoapObject response = (SoapObject) envelope.bodyIn;
        if(response!=null){
            success=1;
        }
        }
        catch (Exception e)
        {
            e.printStackTrace();

        }

        return success;
    }
    int  method2(String Phone) {
        final  String methodname = "secondmethod";
        final  String NAMESPACE ="http://tempuri.org/";
        final  String URL="www.sampleurl.com";
        final  String SOAP_ACTION="http://tempuri.org/secondmethod";
        int success=0;
        StrictMode.ThreadPolicy policy = new StrictMode.ThreadPolicy.Builder().permitAll().build();
        StrictMode.setThreadPolicy(policy);
        SoapObject request = new SoapObject(NAMESPACE, methodname);
        SoapSerializationEnvelope envelope = new  SoapSerializationEnvelope(SoapEnvelope.VER11);
        envelope.dotNet = true;
        request.addProperty("phoneno", phoneno);
        envelope.setOutputSoapObject(request);
        HttpTransportSE androidHttpTransport = new HttpTransportSE(URL);
        try{
            androidHttpTransport.call(SOAP_ACTION,envelope);
            SoapObject response = (SoapObject) envelope.bodyIn;
        if(response!=null){
            success=1;
        }
        }
        catch (Exception e)
        {
            e.printStackTrace();

        }

        return success;
    }

}

If you want to call the services in sequence, use IntentService.

if you are getting confused, check out the video link below and other related videos by Google. They have explained it how and when to use different APIs for threading work.

https://www.youtube.com/watch?v=9FweabuBi1U&list=PLWz5rJ2EKKc9CBxr3BVjPTPoDPLdPIFCE&index=7


Car crash reaction time In physics class we are doing an investigation thing. Some of the numbers might seem plucked out of thin air but they aren't. The maths is right(I think) I just want some feedback on the code.

import datetime
import random
import time
import math

player_1 = input("Player one: ")
speed = int(input("Speed in km/h: "))
speed_ms = ((speed)/3.6)
conditions = input("Wet or dry conditions? (all lowercase)")

if conditions == "wet":
    friction_coefficient = 0.4
else:
    friction_coefficient = 0.6

time.sleep(0.5)
print("Get ready..")
time.sleep(random.randint(1,12))
then = datetime.datetime.now()
t = input("GO!! ")
now = datetime.datetime.now()
diff = then-now
reaction_time = round(abs(diff.total_seconds()), 2)

Rt = (math.sqrt((reaction_time*98)/490))
print('Your final reaction time was: ', (Rt), '!\n')

Rd = (Rt * speed_ms)
print('Your final reaction distance was: ', (Rd), '!\n')

Bd = (((speed_ms)**2) / 19.6 * (friction_coefficient))
print('Your final breaking distance was: ', (Bd), '!\n')

Sd = ((Rd)+(Bd))

############################################################################

print("\n\n---===========FINAL STOPPING DISTANCE===========---")
print('--------------', (Sd), 'meters', '------------')

Some of the numbers might seem plucked out of thin air but they aren't

Still better have them explained. I can understand 3.6 and even 19.6 (it is \$2g\$, isn't it?), but I have no idea what 98/490 stands for. Declare them as symbolic constants with meaningful names.
I don't see a need for abs in

    reaction_time = round(abs(diff.total_seconds()), 2)

Here diff may not be negative. If it is, I'd rather throw an exception, because the result is meaningless anyways.
You are rounding too early. There is no penalty computing Rt with reaction time not being rounded, but rounding leads to loss of precision. Only round when printing results.
You have too many parenthesis. Sd = Bd + Rd is much cleaner than Sd = ((Rd)+(Bd)).


Removing duplicates from a file containing IP addresses I have a big txt file containing some specific IP addresses, and all of them appear more than once (the file has around 13 million lines). The file looks like this (the first octet is set to zero for anonimity):

0.169.252.233
0.140.165.113
0.112.0.44
0.89.149.36

What I want is to read all those addresses and print in an output file only the unique values of each IP, removing the duplicates. Below is the code I am using

int main(){
    FILE *in;
    FILE *out;

    in = fopen("in.csv", "r");
    out = fopen("out.csv", "w");
    char line[17];
    char IPdone[100000][17];   // IP addresses already in the output file
    int nIPs = 0;              // number of IPs in the output file
    int k = 0;
    int bol = 0;

    if(in == NULL) printf("ERROR");

    while (fgets(line, sizeof(line), in)) {    //reads input file line by line
        for (k=0; k<100000; k++){              // checks if the IP is already in the output file
            if(!strcmp(IPdone[k], line)){
                bol=1;
                break;
            }
            bol=0;
        }

        if(bol==0){      // if the IP is not in the output file, print and add to IPdone
            fprintf(out, line);
            strcpy(IPdone[nIPs], line);
            nIPs++;
        }else{          // if it is, continue
            continue;
        }
    }

    fclose(in);
    fclose(out);
}

I am not usually a programmer guy and, although I know this code is not efficient at all, I could not implement any other algorithm with success. This code does work, however, it takes really long to do its job (really really long), and I know there is a much better way of doing it. Also, this code limits the number of IP addresses to 100000 (size of IPdone), and I would rather have a code that can deal with limitless number of different addresses.

I tried the idea of sorting the in file but I had the problem: to sort the file, I need to store all the lines in a vector (right? not sure), but creating a vector[13000000][17] gave me memory problems.

As I said, I am not much into programming, so I would appreciate a help.

For all practical reasons, you should

    > sort in.csv | uniq > out.csv

from the command line, unless the goal is the programming exercise.

As mentioned in the comments, the bottleneck is a linear search over the list of encountered IPs. It drives time complexity quadratic. Sorting is a way to reduce the complexity.

If the file is indeed too large to fit into RAM (which I seriously doubt; these days 13M records shouldn't be a problem), use an external sort (merge works the best). You can also greatly reduce the memory consumption by keeping IPs as numbers (4 bytes per IP), instead of strings:

    in_addr_t addr;
    while(fgets(line, sizeof(line), in)) {
        bool rc = inet_aton(line, &addr);
        if (!rc) {
            // Deal with malformed address
        } else {
            // Business as usual. Work with addr.
        }
    }


java if `String` begins with "red" or "blue" Given a String, if the String begins with "red" or "blue" return that color String, otherwise return the empty String

public String seeColor(String str) {
  if(str.length() <= 3){
    if(str.equals("red")){
      return "red";
    }
    return "";
  }
  else if(str.substring(0, 3).equals("red")){
    return "red";
  }
  else if(str.substring(0, 4).equals("blue")){
    return "blue";
  }
  return "";
}

I really don't like the fact that I'm using same code for "red" twice but I can't get to think out another way. If str is equal to 3, I have to make the check for "red", right?
Example input: "redxx" - output "red",
"xxred" - output "",
"blueAvenue" - output "blue"

Why aren't you using String.startsWith(prefix)?  This should be a one-liner.

return str.startsWith("red")  ? "red"  :
       str.startsWith("blue") ? "blue" : "";


Storing hashed passwords I'm a beginner and am self taught and was hoping I could get help from others more educated than me to show me what bad practices I might be forming before I go on further.

P.S. This program is just for me to learn, I'm not actually storing passwords with it.

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

def openPassFile():
    try:
        passwordFile = open('hashwork.txt', 'a')
        return passwordFile
    except IOError as e:
        print("I/O Error({0}): {1}".format(e.errno, e.strerror))
        quit

def closePassFile(passwordFile):
    try:
        passwordFile.close()
    except IOError as e:
        print("I/O Error({0}): {1}".format(e.errno, e.strerror))
        quit

def randomValue(length):
    import random
    salt_chars = 'abcdefghijklmnopqrstuvwxyz0123456789'
    return ''.join(random.choice(salt_chars) for x in range(length))

def askforUsername():
    while True:
        print("Please enter the username you would like to use:")
        username = raw_input()
        return username

def askforPassword():
    import getpass, hashlib
    while True:
        print("What password would you like to create?")
        salt = randomValue(16)
        hashedPassword1 = hashlib.sha256(salt+getpass.getpass()).hexdigest()
        print("\nPlease enter password again.")
        hashedPassword2 = hashlib.sha256(salt+getpass.getpass()).hexdigest()
        if hashedPassword1 == hashedPassword2:
            return hashedPassword2, salt
            break
        else:
            print("Your passwords do not match. Please retry")

def storeInfo(username, hashedPass, salt):
    passwordFile = openPassFile()
    passwordFile.write(username + " | " + hashedPass + " | " + salt + "\n")
    closePassFile(passwordFile)

username = askforUsername()
hashedPass, salt = askforPassword()
storeInfo(username, hashedPass, salt)
sys.exit()

SHA256 is not a secure password hash because it is fast. See How to securely hash passwords? on security.se for details.
It's not obvious that random is good enough to generate salts. While the requirments for salts are lower than those for keys, you still need a decent RNG. I recommend using SystemRandom.


How to tweak Tape Equilibrium assignment on Codility to be of complexity O(N)? Why Codility detects my solution for Tape Equilibrium to have complexity of \$O(n^2)\$. How can I make my code better?

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.stream.IntStream;

class Solution {
    public int solution(int[] A) {
        boolean eachIntegerIsWithinValidRange = IntStream.of(A).allMatch(i -> (i >= -1000 && i <= 1000));
        boolean numberOfElementsIsWithinDesignatedBounds = A.length >= 2 && A.length <= 100000;

        if (numberOfElementsIsWithinDesignatedBounds && eachIntegerIsWithinValidRange) {
            List<Integer> differences = new ArrayList<Integer>();
            if (A.length < 3) {
                Arrays.sort(A);
                return A[0];
            }

            for (int j = 0; j < A.length - 1; j++) {
                int[] subArrayOne = Arrays.copyOfRange(A, 0, j + 1);
                int[] subArrayTwo = Arrays.copyOfRange(A, j + 1, A.length);
                int sumOne = IntStream.of(subArrayOne).sum();
                int sumTwo = IntStream.of(subArrayTwo).sum();

                differences.add(Math.abs(sumOne - sumTwo));
            }
            Collections.sort(differences, (o1, o2) -> o1.compareTo(o2));
            return differences.get(0);
        } else if (!eachIntegerIsWithinValidRange) {
            throw new RuntimeException("There is/are integer that is out of designated bound: -1000 to 1000");
        } else if (!numberOfElementsIsWithinDesignatedBounds) {
            throw new RuntimeException("Invalid array size. Allowed number of elements is more than 1 and less than 100,001");
        }
        return 0;
    }
}

General improvements

Your method can be made static
Your variable name A would be better as input
By using "early return" or "early throw", your intendation level can be improved a bit
Collections.sort(differences, (o1, o2) -> o1.compareTo(o2)); can be just Collections.sort(differences);
You are using A.length < 3 as an edge case, I would just consider A.length <= 1 as an edge case, the rest should be possible to be handled by your algorithm.

Why is your approach \$O(n^2)\$?

Consider what this code is actually doing:

for (int j = 0; j < A.length - 1; j++) {
    int[] subArrayOne = Arrays.copyOfRange(A, 0, j + 1);
    int[] subArrayTwo = Arrays.copyOfRange(A, j + 1, A.length);
    int sumOne = IntStream.of(subArrayOne).sum();
    int sumTwo = IntStream.of(subArrayTwo).sum();

    differences.add(Math.abs(sumOne - sumTwo));
}
Collections.sort(differences, (o1, o2) -> o1.compareTo(o2));

For every index in your array it does the following things:

Create an subarray, which is an \$O(n)\$ operation
Calculate the sum of the two newly created arrays, which is an \$O(n)\$ operation

These operations makes your code become \$O(n^2)\$

Additionally, it then afterwards does a Collections.sort operation which is \$O(n * log(n))\$

How to do it faster

Initialize a variable sum to be the sum of your whole array, and a sum2 to be 0
For every index, decrease sum with the current value, and increase sum2 with the current value. Then create a diff for sum and sum2 and see if it is lower than the previous lowest record.

Spoiler alert: Here is what I ended up with.

public static int solution(int[] input) {
    boolean eachIntegerIsWithinValidRange = IntStream.of(input).allMatch(i -> (i >= -1000 && i <= 1000));
    boolean numberOfElementsIsWithinDesignatedBounds = input.length >= 2 && input.length <= 100000;

    if (!eachIntegerIsWithinValidRange) {
        throw new RuntimeException("There is/are integer that is out of designated bound: -1000 to 1000");
    }
    if (!numberOfElementsIsWithinDesignatedBounds) {
        throw new RuntimeException("Invalid array size. Allowed number of elements is more than 1 and less than 100,001");
    }
    if (input.length == 0) {
        return 0;
    }

    int sum = IntStream.of(input).sum() - input[0];
    int sum2 = input[0];
    int lowestDiff = Math.abs(sum - sum2);
    for (int j = 1; j < input.length - 1; j++) {
        int value = input[j];
        sum -= value;
        sum2 += value;
        int diff = Math.abs(sum - sum2);
        if (diff < lowestDiff) {
            lowestDiff = diff;
        } else {
            return lowestDiff; // diff will not get any lower
        }
    }
    return lowestDiff;
}


Show and hide posts using setTimeout I have made an effect that has the h1 move and then reveals p using .addClass, .removeClass and CSS3 transition. It works perfectly but I think I am repeating too much of the code and I think there may be a way to write it better. I need postShow and postHide to be different because I need setTimeout to be in postShow for timing. Please review the code below and give me any pointers.

jQuery(document).ready(function($){

    var post          = $('div.post');

    var postShow = function(item) {
            $(item).closest('a').find('img.postimg').addClass('imggrow');
            $(item).closest('a').find('h1').addClass('bannerhover');
            setTimeout(function(){
                $(item).closest('a').find('p.excerpt').addClass('phover');
            },500);
        };

    var postHide = function(item) {
            $(item).closest('a').find('img.postimg').removeClass('imggrow');
            $(item).closest('a').find('h1').removeClass('bannerhover');
            $(item).closest('a').find('p.excerpt').removeClass('phover');
    };

//-----------------------------------------------------------

    if ($(window).width() > 1000) {

        post.hover(function() {
            $(this).find('a').find('p.excerpt').show(0, function() {
                postShow(this);
            });
        },function() {
            $(this).find('a').find('p.excerpt').hide(0, function() {
                postHide(this);
            });
        });

    }

//--------------------------------
});

Firstly, you should add a class to the a element instead, since that's what all the other elements are contained in.

That way, you don't have to add/remove classes on all the child elements separately. You can just add/remove 1 class, and set up your CSS to style the child elements accordingly.

You also decouple your JS from the HTML to greater extent; now you can control things in just the CSS instead of having the HTML, the CSS, and the JS all concern themselves with the exact structure of your page.

Given your current code, however, I would suggest something like this:

jQuery(function($) { // shortcut for $(document).ready(...)
  function showPost() {
    var a = $(this).closest("a"); // get this once, use it thrice
    a.find('img.postimg').addClass('imggrow');
    a.find('h1').addClass('bannerhover');
    a.find('p.excerpt').delay(500).addClass('phover'); // note the delay() call
  }

  function hidePost() {
    var a = $(this).closest("a"); // same as above
    a.find('img.postimg').removeClass('imggrow');
    a.find('h1').removeClass('bannerhover');
    a.find('p.excerpt').removeClass('phover');
  }

  if ($(window).width() > 1000) {
    // No need to use hide/show with a timeout of zero, as far as I can tell.
    // Just pass the two functions directly
    $('div.post').hover(showPost, hidePost);
  }
});


Is this approach correct when testing an Angular controller? I'd like someone to review these tests to make sure I'm along the right lines in Angular testing.

Note - They do all pass (great) but I feel like I'm struggling to understand the finer points of Angular testing.

Any thoughts?

var headerControllers = angular.module('header.controllers', []);
headerControllers.controller('headerController', ['$scope', 'headerService', function($scope, headerService){

this.headerService = headerService;
this.headerTitle = headerService.headerTitle;
this.show = false;

$scope.$watch('header.headerService.headerTitle', function (newVal, oldVal, scope) {
    if(newVal) {
        $scope.header.headerTitle = headerService.headerTitle;
        $scope.header.show = true;
    } else {
        $scope.header.show = false;
    }
});
$scope.header = this;
}]);

Above is the controller to be tested and below are the tests:

describe( "Unit Test:  Header Bar Controller", function() {

var scope, headerService, headerBarController;
var title = "Test header title";

beforeEach( module('Header.controllers'));
beforeEach( module('Header.services') );
beforeEach( inject( function( $injector, $rootScope, $controller, _headerService_ ) {

    scope = $rootScope.$new();

    headerService = $injector.get( 'headerService' );
    headerService.setTitle( title );

    headerBarController = $controller( 'HeaderController',
    {
       $scope: scope,
       headerService: headerService
    });

}));

it( "should have the controller defined", function() {
    expect( headerBarController ).not.toBe( undefined );
});

it( "should receive and load the header-service", function() {
    expect( headerService ).not.toBe( undefined );
    expect( headerBarController.headerService ).toBe( headerService );
});

it( "should have a header title set to the value received from the header-service", function() {
    expect( headerBarController.headerTitle ).toBe( title );
});

it( "should have an initial show value of false", function() {
    expect( headerBarController.show ).toBe( false );
});

it( "should set the header object show value to true when the title is changed", function() {

    headerService.headerTitle = title;
    scope.$apply();

    expect( scope.header.show ).toBe( true );
});

it( "should have header object show value of false if title is not set", function() {

    expect( scope.header.show ).toBe( false );
});

});

It doesn't look to me like proper Unit test.
You are injecting the real dependency but are not mocking.
Unit test is about testing your single Unit of functionality in isolation, with all it dependencies mocked.

Here is example from of my own test code.
This is a proper mock - I am producing a fake Constant:

beforeEach(module(function($provide) {
  $provide.constant('hotelsArray', [{i: 1, n: 'h1'}, {i: 2, n: 'h2'}, {n: 'h3'}, "11"]);
}));

And this is injecting the actual Service:

beforeEach(inject(function(_Hotels_) {
  Hotels = _Hotels_;
}));


Speed up Crawling Every File C:\ and hashing using Java 8 I am wondering if anyone can help me I am trying to read every file in every directory and create a hash of the file. The code works fine but is extremely slow, I wondering what I can do increase the speed and the performance of the below

    public static void main(String[] args) throws IOException, NoSuchAlgorithmException {
    Test fw = new Test();
    fw.crawl("c:\\");
}

public void crawl(String path) throws IOException, NoSuchAlgorithmException {

    File root = new File(path);
    File[] list = root.listFiles();

    if (list == null) {
        return;
    }

    for (File f : list) {

        if (f.isDirectory()) {
            crawl(f.getAbsolutePath());
            System.out.println("Name:" + f.getName());
            System.out.println("Dir:" + f.getAbsoluteFile());
        } else {
            System.out.println("Name:" + f.getName());
            System.out.println("File:" + f.getAbsoluteFile());

            InputStream input = new BufferedInputStream(new FileInputStream(f));
            MessageDigest md5 = MessageDigest.getInstance("MD5");
            int tempByte;
            while ((tempByte = input.read()) != -1) {
                md5.update((byte) tempByte);
            }
            System.out.println("Hash:" + Arrays.toString(md5.digest()));
        }
    }
}

The main drawbacks of the solution you are trying to apply are:

The I/O streams are never closed: resources are leaking severely.
The InputStream reads data per byte and updates the MessageDigest with a single byte, but it would be more efficient when using a buffer.
The main logic body is using older File API, but there are newer tools since Java 7.
md5 instance is created for each file, but it can be reused by calling md5.reset().

Here what I can suggest to improve this solution.

The main call will be ridiculously short:

public void crawl2(String path) throws IOException {
  Files.walkFileTree(Paths.get(path), new HashingFileCrawler());
}

Files.walkFileTree needs an instance of FileVisitor and it executes all the boilerplate of traversing the file hierarchy, we need only to implement our logic. Let's call our implementor HashingFileCrawler:

public class HashingFileCrawler extends SimpleFileVisitor<Path> {

  private final MessageDigest md5; // only one instance

  public HashingFileCrawler() {
    try {
      this.md5 = MessageDigest.getInstance("MD5");
    } catch (NoSuchAlgorithmException ex) {
      // if we enter this catch block, there is definitely something wrong with the running JVM. "MD5" algo should exist by default.
      throw new IllegalStateException(ex);
    }
  }

  @Override
  public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) {
    System.out.println("Dir: " + dir.toString());
    return FileVisitResult.CONTINUE;
  }

  @Override
  public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException {
    System.out.println("File: " + file.toString());
    final byte[] buffer = new byte[2048];
    try (InputStream fis = Files.newInputStream(file);
         BufferedInputStream bis = new BufferedInputStream(fis)) {
      int nbRead = bis.read(buffer); // read file data into the buffer for exchange with md5
      while (nbRead > -1) {
        md5.update(buffer, 0, nbRead);
        nbRead = bis.read(buffer);
      }
      System.out.println("Hash:" + Arrays.toString(md5.digest()));
      md5.reset();  // reset MD for reuse for next file
      return FileVisitResult.CONTINUE;
    }
  }
}

preVisitDirectory is called before crawling the files in a directory: it corresponds to the original code under condition if (f.isDirectory()).

visitFile wraps the main logic of producing a hash for a file, under else in the original code.

The size of the byte buffer (2048 here) can be adjusted: try to play with different values to see whether the perfs change.


Transforming a list of maps representing bands I have the following code, it takes a list of maps representing bands and transforms the band properties via the pipeline_each method.

As you can see in the code, I am dealing with a lot of Options being returned when I modify the band data in the map - I am using an immutable map object and therefore need to return a new map each time the function is called.

I am also not happy about the type of the bands data structure being a List[Map[String, Any]] - particularly the Any part because I need to convert the values I get out of the maps to String types in order to modify them. I am stuck with this data structure though.

How can I make this code better, specifically addressing my concerns above?

    object Exercise {

  val bands: List[Map[String, Any]] = List(Map("name" -> "sunset rubdown", "country" -> "UK", "active" -> true),
  Map("name" -> "women", "country" -> "Germany", "active" -> false),
  Map("name" -> "a silver mt. zion", "country" -> "Spain", "active" -> true))

  def set_canada_as_country(band: Map[String, Any]): Map[String, Any] = {
    Map("name" -> band.get("name").get, "country" -> "Canada", "active" -> band.get("active").get )
  }

  def strip_punctuation_from_name(band: Map[String, Any]): Map[String, Any] = {
    Map("name" -> band.get("name").get.toString.replace(".", ""), "country" -> band.get("country").get, "active" -> band.get("active").get )
    //toString above is kinda gross and could cause a class cast exception
    //do I have to handle all the case matching on some/none for all the above calls to get?
  }

  def capitalize_names(band: Map[String, Any]): Map[String, Any] = {
    Map("name" -> band.get("name").get.toString.toUpperCase, "country" -> band.get("country").get, "active" -> band.get("active").get )
    //same again - don't like the toString
  }

  def pipeline_each(data: List[Map[String, Any]], fnList: List[Map[String, Any] => Map[String, Any]]): List[Map[String, Any]] = {
    fnList.foldLeft(data) {(bandList, currFn) => bandList.map(currFn)}
  }

}

I execute the above code via the REPL like so:

Exercise.pipeline_each(Exercise.bands,
  List(
    Exercise.set_canada_as_country,
    Exercise.strip_punctuation_from_name,
    Exercise.capitalize_names))

You might consider using map on Option to more cleanly access the values.

  def set_canada_as_country(band: Map[String, Any]): Map[String, Any] = {
    band.get("name").map( name => band.get("active").map(active =>
      Map("name" -> name, "country" -> "Canada", "active" -> active))
  }

At this point, you could reasonable change the resulting type to Map[String, String].

Rather than using a list of functions, you might want to compose a new function using andThen:

Exercise.pipeline_each(Exercise.bands, Exercise.set_canada_as_country andThen
  Exercise.strip_punctuation_from_name andThen
  Exercise.capitalize_names)

That way you are only applying a single filtering function.


Best way to store the sum of all node depths? To sum up all the nodes' depths in any given binary tree, I've written the following recursive algorithm:
def nodeDepths(root):
    final=[0]
    helper(root,0, final)
    return final[0]

def helper(node,d, final):
    if not node:
        return
    final[0]+= d
    helper(node.left,d+1, final)
    helper(node.right,d+1, final)

class BinaryTree:
    def __init__(self, value):
        self.value = value
        self.left = None
        self.right = None

My thinking was: as I see each node, add the depth of that node to the final sum, then recursively call on the left and right with final list as an argument. At the end of the recursive call stack, final[0] should have the right value.
Is there a better way to do this? I have concerns about thread safety in general with global variables but is it a better practice to use global variables in this case?

PEP8
nodeDepths should be node_depths in snake_case.
Function naming
helper is not a helpful name for a function. If I were to guess what this does, it should maybe be called recurse_node_depths.
Instance methods
As it stands, BinaryTree does not deserve to have an __init__. It would be better-suited as a @dataclass or maybe a named tuple. That said, it probably makes more sense for node_depths to be an instance method where self replaces root.
Integer-by-reference
My first read of this code was wrong. final is only ever going to have one member. My guess is that you did this to effectively pass an integer by reference, but this is a gross hack. Instead, just return the evolving sum as an integer from your recursion, and the uppermost return will be the total that you need.
Slots
Another way to squeeze performance out of this is to initialize __slots__ for BinaryTree based on the three known members.
Recursion
Recursion is not a great idea in Python, for at least two reasons:

Given that there is no indication to the maximum depth of your input tree, you may blow the stack.
Since Python does not have tail recursion optimization, recursion is slower than some other languages.

So you should attempt to reframe this as an iterative implementation.


Reduce the calls to database for validations in rails I was building a rails web, and used form object to separate the validation logics per form.

I had to find_referral_code twice in order to store the user_id of referrer. I wonder how I can improve it.

after_sign_up_form.rb

def get_referred_by_id
    User.where(referral_code: referer_referral_code).first.id
end

def validates_presence_of_referer_referral_code
    u = User.find_by_referral_code(referer_referral_code)
    if u.nil?
        errors.add :referer_referral_code, "Not a valid Referral Code"
    else
        if @user.referral_code == referer_referral_code
            errors.add :referer_referral_code, "Can't use your own referral code"
        end
    end
end

def validates_unique_primary_email
    # errors.add :primary_email, "Email is taken"
end

def email_is_self
    (@user.identities.pluck(:email) << @user.email).flatten.include? primary_email
end

You can memorize result using ||=

def referred_by_id
    @reffered ||= User.where(referral_code: referer_referral_code).first.id
end

def validates_presence_of_referer_referral_code
    u = referred_by_id
    if u.nil?
        errors.add :referer_referral_code, "Not a valid Referral Code"
    else
        if @user.referral_code == referer_referral_code
            errors.add :referer_referral_code, "Can't use your own referral code"
        end
    end
end

You shouldn't name your method with get_ prefix.

EDITED:

It is ruby naming convention to not use prefixes like get_ nor set_ (according to Rubocop and Ruby style guide). I think it is because, every method is a getter by default (always returns last line).
When you call referred_by_id method, it will query a db just once and then it will memorize result. So it will improve performance if this method is called more then once in this class.


Java 2D camera movement and zoom I would appreciate some help with camera movement and zoom. Is there any easier way with cleaner code, or simply how can I improve it?

Window size: 800x600
Image size: 1200x900

This is my code so far (you can move with the camera only around the image and use zoom):

package game.engine.gui;

import java.awt.Graphics2D;
import java.awt.Point;
import java.awt.event.MouseEvent;
import java.awt.event.MouseWheelEvent;

public class Camera {

public boolean isEnabled = true;
public int offsetX = 0;
public int offsetY = 0;
public float zoom = 1;
private Point mouseLocation;
private int maxOffsetXZoom = 200;
private int maxOffsetYZoom = 150;
private final int OFFSET_X_MAX = 200;
private final int OFFSET_Y_MAX = 150;
private final float ZOOM_MAX = 1.2F;
private final float ZOOM_MIN = 0.7F;

public Camera() { }

public void draw(Graphics2D g) { }

public void update() { }

public void mousePressed(MouseEvent e) { mouseLocation = e.getPoint(); }

public void mouseReleased(MouseEvent e) { }

public void mouseMoved(MouseEvent e) { }

public void mouseDragged(MouseEvent e)
{
    int newoffsetX = offsetX - (mouseLocation.x - e.getX());
    int newoffsetY = offsetY -  (mouseLocation.y - e.getY());

    if(newoffsetX > maxOffsetXZoom) offsetX = maxOffsetXZoom;
    else if(newoffsetX  < -maxOffsetXZoom + (OFFSET_X_MAX-maxOffsetXZoom) * 4) offsetX = -maxOffsetXZoom + (OFFSET_X_MAX-maxOffsetXZoom) * 4;
    else offsetX = newoffsetX;

    if(newoffsetY  > maxOffsetYZoom) offsetY = maxOffsetYZoom;
    else if(newoffsetY < -maxOffsetYZoom - 25 + (OFFSET_Y_MAX-maxOffsetYZoom) * 4) offsetY = -maxOffsetYZoom - 25 + (OFFSET_Y_MAX-maxOffsetYZoom) * 4;
    else offsetY = newoffsetY;

    mouseLocation = e.getPoint();
}

public void mouseScrolled(MouseWheelEvent e)
{
    if (e.getWheelRotation() < 0) {
        if(zoom<ZOOM_MAX) zoom += 0.05;
    } else {
        if(zoom>ZOOM_MIN) zoom -= 0.05;
    }
    maxOffsetXZoom = (int) (OFFSET_X_MAX * zoom);
    maxOffsetYZoom = (int) (OFFSET_Y_MAX * zoom);
    if(offsetX > maxOffsetXZoom) offsetX = maxOffsetXZoom;
    else if(offsetX  < -maxOffsetXZoom + (OFFSET_X_MAX-maxOffsetXZoom) * 4) offsetX = -maxOffsetXZoom + (OFFSET_X_MAX-maxOffsetXZoom) * 4;

    if(offsetY  > maxOffsetYZoom) offsetY = maxOffsetYZoom;
    else if(offsetY < -maxOffsetYZoom - 25 + (OFFSET_Y_MAX-maxOffsetYZoom) * 4) offsetY = -maxOffsetYZoom - 25 + (OFFSET_Y_MAX-maxOffsetYZoom) * 4;
}
}

It works, but I think it's a huge mess and also if the image size was different it would require a lot of changes. I am using my own engine.

I'm going to assume your indentation got messed up on posting, but just in case, you should indent all of the code inside of the Camera class an additional time.

First off, I like that you used constants at the top of your code!  To ensure that they look and act like constants, you should make them private, static, final, and named in UPPERCASE_FORMAT.  Furthermore, I would recommend separating your constants from your class variables (such as mouseLocation and offsetX).

I'm not entirely sure how your functions are being called.  It looks like you are implementing MouseMotionListener, so I would definitely add that to your class declaration.

If that is true, then I don't see the purpose of the drag variable.  The mouseDragged function is only going to get called if the mouse is actually being dragged, so assigning a boolean when the mouse is pressed and released appears redundant.

Also, I'd recommend considering whether you want to use BigDecimal.  If round needs to be fast, then you don't want to use BigDecimal at all.  There are other techniques for rounding to the nth digit  that can be found here


Finding the maximum row of a 4x4 matrix I have written a recursive function that I believe will return the 'maximum' row of the 4x4 matrix it is fed. By 'maximum' I mean that the maximum row of the matrix.

1 2 3 4
2 3 4 1
3 4 1 2
4 1 2 3

is row 4 (on account of the first column's values); the maximum row of the matrix

1 2 3 4
2 3 4 1
4 4 1 2
4 1 2 3

is row 3 (on account of the first and second columns' values); the maximum row of the matrix

1 2 3 4
2 3 4 1
4 4 1 2
4 4 2 3

is row 4 (on account of the first, second, and third columns' values); the maximum row of the matrix

1 2 3 4
2 3 4 1
4 4 1 2
4 4 1 3

is row 4 (on account of the values in every column); and the maximum row of the matrix

1 2 3 4
2 3 4 1
4 4 1 3
4 4 1 3

is row 3 (on account of the values in every column - pick the topmost row).

This function is a lot more involved than I expected when I sat down to write it. Please tell me there's an easier way to express this algorithm in C!

(The function is employed in a program written by a Pascalite; please forgive the fact that the arrays ignore their 0th member!)

Here's the function (together with its helper indexOf(), which is never fed an array in which val fails to appear!) Apologies for variable indentation - bit of a nightmare just getting it to this state!

int indexOf(int a[], int val)
{ /* returns index at which val occurs in a[] */
    int i=1;

    while(a[i] != val)
        i++;

    return i;
}

int maxRow (int a[5][5], int index)
{
  int store [5] = {-1, a[1][index], a[2][index], a[3][index], a[4][index] };
  int store2[5] = {-1, a[1][index], a[2][index], a[3][index], a[4][index] };

  int i, j, k, max, i1=0, i2=0, i3=0, i4=0;
  int arr[5]={-1,-1,store2[2],store2[3],store2[4]};

  if (index==5)
  {
    if(a[1][4]>=0)
        return 1;
    else if(a[2][4]>=0)
        return 2;
    else if(a[3][4]>=0)
        return 3;
    else if(a[4][4]>=0)
        return 4;
  }

  /* order store[] from max -> min: */

  for(i=1; i<=3; i++)
  {
    max=store[i];
    j=i;
    for(k=i+1; k<=4; k++)
        if(store[k]>max)
        {
            max=store[k];
            j=k;
        }
    store[j]=store[i];
    store[i]=max;
  }

  i1 = indexOf(store2, store[1]);

  /* now we must indicate which (if any ) rows to recurse on... */

  if (store[1]==store[2])
  {
    if(i1==1)
        i2 = indexOf(arr,store[1]);
    else if (i1==2)
    {
        arr[2] = -1;
        i2 = indexOf(arr, store[1]);
    }
    else if (i1==3)
        i2 = 4;

    /* note: if i2==4 then can't have store[1] equal to any other members of store[] */

    if(store[1]==store[3])
    {
        /* i2 = 2 or 3 */

        if(store2[4] <= store2[3])
            i3 = 3;
        else i3=4;

        if (store[1]==store[4])
        {
            i4=4;
        }
    }
  }

    if (i2==0)
        return i1;

    else if(i3==0)
    {
          i = indexOf(store2,store[4]);

          for(j=0;j<5;j++)
            a[i][j]=-1;

          i = indexOf(store2,store[3]);

          for(j=0;j<5;j++)
            a[i][j]=-1;

          return maxRow(a, index+1);
    }
    else if (i4==0)
    {
      i = indexOf(store2,store[4]);

      for(j=0;j<5;j++)
            a[i][j]=-1;
      return maxRow(a, index+1);
    }
    else /* all i are non-zero */
      return maxRow(a, index+1);
}

The first thing I'd do if I were refactoring this code is get rid of recursion.

You should be able to do it with a variable to keep track of "best row number so far", and a simple "for each row" loop. Inside the loop would be code that compares the current row with the best row so far (and does "best row so far = current row" if the current row is better).

Example:

int maxRow(int a[4][4]) {
    int i,j;
    int bestRow = 0;

    for(i = 1; i < 4; i++) {
        for(j = 0; j < 4; j++) {
            if(a[i][j] !=  a[bestRow][j]) {
                if(a[i][j] > a[bestRow][j]) {
                    bestRow = i;
                }
                break;
            }
        }
    }
    return bestRow;
}


indexOf() + 1 vs indexOf() != -1 When checking for existence of a substring I have been doing this:

var that = "ok hello cool";
if( that.indexOf('hello') + 1 ) {
}

Instead of:

if( that.indexOf('hello') != -1 ) {
}

Am I overlooking something or is there a reason not to do this.

Update:

Yes, I was indeed unaware of the even simpler method of:

if ( ~that.indexOf( 'hello' ) ) {

}

You can read about the ~ bitwise operator and the other queer bitwise operators here:
https://developer.mozilla.org/en/JavaScript/Reference/Operators/Bitwise_Operators

Well, I believe the second one is more obvious what's going on...

if( that.indexOf('hello') != -1 ) {
}

That's all to it however, both expressions are valid and perfectly ok.


SQL Server stored procedure boilerplate What would you do to improve upon this boilerplate empty stored procedure, being mindful of the delicate balance between length, complexity, performance and clarity?

-- =============================================
-- Author:      The usual suspects
-- Create date: 10/06/2011
-- Description:
--
-- Nice long description about the procedure
--
-- =============================================
CREATE PROCEDURE

[dbo].[My_Stored_Proc]
(
-- exampleParam is an example parameter.
@exampleParam INT = 30
)
AS
BEGIN   -- main
SET NOCOUNT ON
BEGIN TRY
    DECLARE @crlf varchar(2)
    SET @crlf = CHAR(13) + CHAR(10)

    -- *** DO YOUR STUFF HERE ***

END TRY
BEGIN CATCH
    -- Error handler
    DECLARE @ErrorNumber INT
    DECLARE @ErrorSeverity INT
    DECLARE @ErrorState INT
    DECLARE @ErrorProcedure NVARCHAR(4000)
    DECLARE @ErrorLine INT
    DECLARE @ErrorMessage NVARCHAR(4000)

    DECLARE @ErrorDescription NVARCHAR(4000)

    -- retrieve error info
    SELECT
        @ErrorNumber = ERROR_NUMBER(),
        @ErrorSeverity = ERROR_SEVERITY(),
        @ErrorState = ERROR_STATE(),
        @ErrorProcedure = ERROR_PROCEDURE(),
        @ErrorLine = ERROR_LINE(),
        @ErrorMessage = ERROR_MESSAGE();

    -- build custom error description
    SELECT @ErrorDescription = @crlf + @crlf + 'Base Error:\t[' + CAST(@ErrorNumber AS VARCHAR) + '] ' +
        @ErrorMessage + @crlf + @crlf +
        'exampleParam:\t' + CAST(@exampleParam AS VARCHAR) + @crlf +
        'Application:\t' + APP_NAME() + @crlf +
        'User:\t' + SYSTEM_USER + @crlf +
        'Database:\t' + DB_NAME() + @crlf +
        'Procedure:\t' + @ErrorProcedure + @crlf +
        'Line:\t' + CAST(@ErrorLine AS VARCHAR) + @crlf +
        'Severity:\t' + CAST(@ErrorSeverity AS VARCHAR) + @crlf +
        'State:\t' + CAST(@ErrorState AS VARCHAR);

    RAISERROR(@ErrorDescription, @ErrorSeverity, 1)
    RETURN @@ERROR
END CATCH
END     -- main

For instance, is there a nice way to move that error handler out of the stored proc so the logic can be shared and not duplicated inside each procedure?  Notice it can be nice to include the values of the parameters in that error message (exampleParam above).

Do you agree or disagree with my stance on handling transaction rollbacks in stored procs?  (lack thereof)

Do you have or can you write an example of better boilerplate along with a description of where my approach falls short and why your version might be a better starting point

What about SET XACT_ABORT {ON | OFF}?  Which option for XACT_ABORT would be a best practice?

All in all it’s a good idea. Have you considered moving your crlf and CATCH logic; or a portion of it; to a reusable function? This would help ensure; wherever it’s used; that it remain consistent and you don’t have the same code all over the place.


Topological sort using STL I am trying to implement topological sort using STL. I have used vector of lists to store adjacency list. Indegree is an array which stores indegree of vertices.

Indegree0 is a queue implemented using STL list to store vertices with indegree 0. Can someone please help me improve my code?

# include <iostream>
# include <bits/stdc++.h>
using namespace std;

vector<int> topologicalsort(vector<list<int> > adjacencylist,int  indegree[],int v,int e)
{

    vector<int>ans(v+1);
    int k=1;
    //list to store vertices with indegree 0
    list<int>indegree0;
    for(int i=1;i<=v;i++)
    {
        if(indegree[i]==0)
            indegree0.push_back(i);
    }
    while(!indegree0.empty())
    {
        int x=indegree0.front();
        indegree0.pop_front();
        ans[k]=x;
        k++;
        list<int>::iterator itr=adjacencylist[x].begin();
        while(itr!=adjacencylist[x].end())
        {
            indegree[*itr]--;
            if(indegree[*itr]==0)
                indegree0.push_back(*itr);
            itr++;
        }
    }
    return ans;
}

int main()
{
    int v,e;
    cin>>v>>e;
    vector<list<int> > adjacencylist(v+1);
    int indegree[v+1];
    for(int i=1;i<=v;i++)
        indegree[i]=0;
    for(int i=1;i<=e;i++)
    {
        int v1,v2;
        cin>>v1>>v2;
        adjacencylist[v1].push_back(v2);
        indegree[v2]++;
    }
    //cout<<adjacencylist.size()<<endl;
    for(int i=1;i<=v;i++)
    {
        printf("adjacencylist[%d] :",i);
        list<int>::iterator itr=adjacencylist[i].begin();
        while(itr!=adjacencylist[i].end())
        {
            cout<<*itr<<"  ";
            itr++;
        }
        cout<<endl;
    }
    vector<int> ans=topologicalsort(adjacencylist,indegree,v,e);
    for(int i=1;i<=v;i++)
        cout<<ans[i]<<"  ";

}

Headers and namespaces

#include "bits/stdc++.h"

That's not a standard C++ header; prefer to use standard headers so that others can use your code.

using namespace std;

Bringing all names in from a namespace is problematic; namespace std particularly so. See Why is “using namespace std” considered bad practice?.

Used but not included: <cstdio>, <vector>,  <list>.  I'd recommend you stick to one of <cstdio> and <iostream>.

Function signature

std::vector<int> topologicalsort(std::vector<std::list<int> > adjacencylist,
                                 int indegree[], int v,
                                 int e)

Why do you require e?  It's never used.

Passing indegree as a pointer and length is very C-style; prefer a C++ range (e.g. a pair of iterators, or a standard container).

There's no need to pass adjacencylist by value: a const-ref is more appropriate.

Result vector

Instead of creating v+1 elements in the result vector and having to keep track of our insert position with k, we can create it empty and push_back each result as we find it.  We can avoid reallocation by using reserve().

Looping over elements

Instead of the error prone iterator-based loop, prefer to use range-based for when applicable:

    for (int i: adjacencylist[x]) {
        if (--indegree[i] == 0)
            indegree0.push_back(i);
    }

Variable-length arrays

This is a non-standard extension:

int indegree[v+1];

It's not clear why you ignore the 0th element of these arrays; that probably warrants a comment.

Cleaned code

#include <iostream>
#include <list>
#include <vector>

std::vector<int> topologicalsort(const std::vector<std::list<int>>& adjacencylist,
                                 std::vector<int> indegree)
{
    std::vector<int> ans;
    ans.reserve(indegree.size()+1);
    //list to store vertices with indegree 0
    std::list<int> indegree0;
    for (size_t i = 1;  i < indegree.size();  i++) {
        if (indegree[i] == 0)
            indegree0.push_back(i);
    }
    while (!indegree0.empty()) {
        const int x = indegree0.front();
        indegree0.pop_front();
        ans.push_back(x);
        for (const int i: adjacencylist[x]) {
            if (--indegree[i] == 0)
                indegree0.push_back(i);
        }
    }
    return ans;
}

int main()
{
    int v, e;
    std::cin >> v >> e;
    std::vector<std::list<int>> adjacencylist(v+1);
    std::vector<int> indegree(v+1);
    for (int i = 1;  i <= e;  i++) {
        int v1, v2;
        std::cin >> v1 >> v2;
        adjacencylist[v1].push_back(v2);
        ++indegree[v2];
    }
    for (int i = 1;  i <= v;  i++) {
        std::cout << "adjacencylist[" << i << "%d] :";
        for (int j: adjacencylist[i]) {
            std::cout << j << "  ";
        }
        std::cout << std::endl;
    }
    auto const& ans = topologicalsort(adjacencylist, std::move(indegree));
    for (int i: ans)
        std::cout << i << "  ";
}


JDBC wrapper in a functional manner I implemented a simple Java JDBC wrapper in Kotlin with Arrow functional library.

/**
 * Executes simple SQL statement that takes no arguments.
 * E.g. "CREATE TABLE" statement.
 *
 * @return IO with either an exception or boolean result
 */
fun String.executeStatement(dataSource: DataSource): IO<Either<Throwable, Boolean>> {
    val connection = dataSource.connection
    return IO { connection.createStatement().execute(this) }.attemptAndCloseConnection(connection)
}

/**
 * Executes prepared SQL statement (a statement that takes arguments).
 * E.g. "INSERT, UPDATE, DELETE..." statements.
 *
 * @return IO with either an exception or with result of execution as an integer
 */
fun String.executePreparedStatement(dataSource: DataSource, vararg args: Any): IO<Either<Throwable, Int>> {
    val connection = dataSource.connection
    return IO {
        val preparedStatement = connection.prepareStatement(this)
        args.forEachIndexed() { idx, arg -> preparedStatement.setObject(idx + 1, arg) }
        preparedStatement.executeUpdate()
    }.attemptAndCloseConnection(connection)
}

/**
 * Executes SQL query
 */
fun <E> String.executeQuery(dataSource: DataSource, resultSetMapper: (ResultSet) -> E, vararg args: Any): IO<Either<Throwable, List<E>>> {
    val connection = dataSource.connection
    return IO {
        val list = mutableListOf<E>()
        val preparedStatement = connection.prepareStatement(this)
        args.forEachIndexed() { idx, arg -> preparedStatement.setObject(idx + 1, arg) }
        val resultSet = preparedStatement.executeQuery()
        while (resultSet.next()) {
            list.add(resultSetMapper(resultSet))
        }
        list.toList()
    }.attemptAndCloseConnection(connection)
}

private fun <A> IO<A>.attemptAndCloseConnection(connection: Connection?) =
        this.attempt().map { connection?.close(); it }

Every result of execution is wrapped in an instance of IO to represent side-effects.

Functions are invoked directly on strings which produces an instance of Either<Throwable, T> when run:

val dataSource = HikariDataSource()
// add config for data source...

"""CREATE TABLE cities (
            id INTEGER PRIMARY KEY AUTO_INCREMENT,
            name VARCHAR(255) NOT NULL);""".executeStatement(dataSource)
                .unsafeRunSync()

"INSERT INTO cities (name) VALUES (?)".executePreparedStatement(dataSource, "New York").unsafeRunSync()

My major concerns are:

Passing an instance of DataSource upon invocation of every function. E.g. with Scala, I could pass data source implicitly which would declutter invocations.
Mutable list in executeQuery function. Is it even ok?
Closing connection (attemptAndCloseConnection function) - is there some better way to do this?
Other proposals?

You can pass context by defining extenstions inside the class + by using "Builder API". But I recommend you to avoid extensions like String.smt().

object ConnectionPool{
    fun useConnection(body: Connection.() -> Unit){
        val connection = Connection()
        connection.body()
    }
}

class Connection {
    val field = 42
    fun String.execute() {
        println(field)
    }
    fun execute2(query: String){
        println(field)
    }
}

fun sample() {
    ConnectionPool.useConnection {
        "CREATE TABLE".execute()
        execute2("CREATE TABLE")
    }
}

Also you can avoid mutablelist if you use generateSequence{} and map{}

generateSequence { if (resultSet.next()) resultSet else null }
   .map{ resultSetMapper(it) }.toList()

And I would reccomend to put close connection to useConnection().


Hackerrank Insertion Sort Part 2 I have started learning Java recently and was looking into the challenges on sorting on Hackerrank.I solved the following problemHackerrank on Insertion Sort.
The challenge was :

In Insertion Sort Part 1, you sorted one element into an array. Using the same approach repeatedly, can you sort an entire unsorted array?
Guideline: You already can place an element into a sorted array. How can you use that code to build up a sorted array, one element at a time? Note that in the first step, when you consider an element with just the first element - that is already "sorted" since there's nothing to its left that is smaller.
In this challenge, don't print every time you move an element. Instead, print the array after each iteration of the insertion-sort, i.e., whenever the next element is placed at its correct position.
Since the array composed of just the first element is already "sorted", begin printing from the second element and on.
Input Format
There will be two lines of input:

the size of the array
a list of numbers that makes up the array

Output Format :
On each line, output the entire array at every iteration.

Sample Input:
6
1 4 3 5 6 2

Sample Output:
 1 4 3 5 6 2
 1 3 4 5 6 2
 1 3 4 5 6 2
 1 3 4 5 6 2
 1 2 3 4 5 6

I am trying to get better at writing good code for my solutions..Please give me your suggestions .
import java.util.Scanner;

public class InsertionSortPart2 {

public static void main(String[] args) {

    Scanner keyboard = new Scanner(System.in);
    int size = keyboard.nextInt();

    int[] array = new int[size];

    for (int i = 0; i < array.length; i++) {
        array[i] = keyboard.nextInt();

    }

    array = insertionsort(array);

}

private static int[] insertionsort(int[] a) {

    for (int i = 1; i < a.length; i++) {
        // set key to value at index i
        int key = a[i];
        // set j as i-1 to look for previous element
        int j = i - 1;
        while (j >= 0 && a[j] > key) {
            a[j + 1] = a[j];
            j--;
        }

        a[j + 1] = key;
        for (int f = 0; f < a.length; f++) {
            System.out.print(a[f] + " ");
        }
        System.out.println();
    }

    return a;
  }
}

Your implementation is quite fine. Some minor improvements are possible.

The insertionsort method doesn't need to return the array. It's not required by the problem, and it's pointless anyway, as the method modifies the input array.

In the main method you called the array array. That's not a very useful name, too generic.  Inside insertionsort it's even worse just the letter a. A better name in both places would be for example nums.

The inner while loop would be better as a for loop. The reason is, with a for loop it's easier to not forget to decrement the loop variable:

for (; j >= 0 && nums[j] > key; --j) {

Also, you set j = i - 1, and then you have to remember to use a[j + 1] in two places. It would be slightly simpler to use j = i and rearrange the rest accordingly.

Lastly, the printing of the array really belongs to a separate helper method.


Index into array as if it is circular The problem:

Treat an array as if it is circular; out-of-bounds indices "wrap around" the array to become in-bounds.

Current solution:

// roudedArray.cpp
#include <iostream>

template <typename T, int N>
int wrapAroundArray(int i, T(&ThisArray)[N])
{
  // "wrap" i around ThisArray, landing on a valid index
  while (i < 0) { i += N; }
  while (i >= N) { i -= N; }
  return i;
}

std::string words[] = { "this", "and", "that" };

int main()
{
  for (int i = -9; i < 10; i++)
  {
    int adj = wrapAroundArray(i, words);
    std::cout << "i: " << i;
    std::cout << ", adjusted index: " << adj;
    std::cout << ", word: " << words[adj] << std::endl;
  }
}

Output (compile with g++ roundedArray.cpp):

i: -9, adjusted index: 0, word: this
i: -8, adjusted index: 1, word: and
i: -7, adjusted index: 2, word: that
i: -6, adjusted index: 0, word: this
i: -5, adjusted index: 1, word: and
i: -4, adjusted index: 2, word: that
i: -3, adjusted index: 0, word: this
i: -2, adjusted index: 1, word: and
i: -1, adjusted index: 2, word: that
i: 0, adjusted index: 0, word: this
i: 1, adjusted index: 1, word: and
i: 2, adjusted index: 2, word: that
i: 3, adjusted index: 0, word: this
i: 4, adjusted index: 1, word: and
i: 5, adjusted index: 2, word: that
i: 6, adjusted index: 0, word: this
i: 7, adjusted index: 1, word: and
i: 8, adjusted index: 2, word: that
i: 9, adjusted index: 0, word: this

I'd prefer an arithmetic (constant-time) solution that produces the same results. Any suggestions?

The arithmetic solution to this is relatively simple as we're talking modulo here:

return i % N;

This should produce the same results for your adjusted index.

When I ran a few calculations on the modulo operator through WolframAlpha, I got the following results:

for \$F(x) = x \mod 3\$

\$ 5 \mapsto 2 \$
\$ 4 \mapsto 1 \$
\$ 3 \mapsto 0 \$
\$ 2 \mapsto 2 \$
\$ 1 \mapsto 1 \$
\$ 0 \mapsto 0 \$
\$-1 \mapsto 2 \$
\$-2 \mapsto 1 \$
\$-3 \mapsto 0 \$

The pattern we see here is exactly what you describe.

@Schism pointed out that this behavior is only that of "mathematical modulo". It seems that some implementations are unfriendly in that regard. We can thus assume that we need to actually do the switching of negative numbers ourselves.

This leads to the following implementation:

if (i < 0) {
   //assuming the behavior described by Schism (-1 % 3 = -1)
   i = N + (i % N);
} else {
   i = i % n;
}

Unfortunately this is still implementation-dependent, so now we need to eliminate the sign in our modulo calculations to have the same behavior, regardless of implementation:

bool wasNegative = false;
if (i < 0) {
    wasNegative = true;
    i = -i; //there's definitely a bithack for this.
}
int offset = i % N;
return (wasNegative) ? (N - offset) : (offset);


Is this Repository/UOW design workable? I'm using VS 2013/EF 6/WEB API 2.

This application of mine which is working on will eventually grow to be big project and I'm in the very beginning stage and need your expertise to make it simple as possible without over-complication.

My goal is to make it KISS solution and I'm trying my best.  After spending a lot of time thinking and researching I have come up with this design.

PS: I'm very open for suggestions and willing to change my design please don't be harsh with me.

//DAL project

public interface IRepository<T>
{
    IEnumerable<T> GetAll();
}

//Impl

public class HostRepository : IRepository<Host>
    {
        public IEnumerable<Host> GetAll()
        {
            using (SRDbContext context = new SRDbContext())
            {
                var hosts = from host in context.Hosts
                            select new Host()
                            {
                                FirstName = host.FirstName,
                                LastName = host.LastName,
                                Email = host.Email
                            };
                return hosts.AsEnumerable().ToList();
            }
            return null;
        }
    }

//UOW

public class UnitOfWork : IDisposable
    {
        private SRDbContext context = new SRDbContext();
        private GenericRepository<Host> hostRepository;

        public GenericRepository<Host> HostRepository
        {
            get
            {
                if (this.hostRepository == null)
                {
                    this.hostRepository = new GenericRepository<Host>(context);
                }
                return hostRepository;
            }
        }
     }

//GenericRepository

public class GenericRepository<T> where T : class
    {
        internal DbContext context;
        internal IDbSet<T> IDbSet;

        public GenericRepository(DbContext context)
        {
            this.context = context;
            this.IDbSet = context.Set<T>();
        }

        public virtual T GetByID(object id)
        {
            return IDbSet.Find(id);
        }

        public IEnumerable<T> GetAll()
        {
            IQueryable<T> query = IDbSet;
            return query;
        }
    }

//Service project

public class HostService
    {
        //HostRepository reqRepo = new HostRepository();
        private UnitOfWork _uow = null;
        protected UnitOfWork UOW
        {
            get
            {
                if (_uow == null)
                {
                    _uow = new UnitOfWork();
                }
                return _uow;
            }
        }
        public IEnumerable<Host> GetAll()
        {
            return UOW.HostRepository.GetAll();
        }
   }

//WebAPI project

[ResponseType(typeof(HostDTO))]
    [Route("hosts")]
    [HttpGet]
    public async Task<IHttpActionResult> Get()
    {
        IEnumerable<HostDTO> host = hs.GetAll();

        if (host == null)
        {
            return NotFound();
        }

        return Ok(host);
    }

Entity Framework already implements UoW+Repository for you.
To KISS, your code would simply "wrap" the DbContext with an interface, as explained here, here and here, and possibly elsewhere too. Shortly put: UoW+Repository is an abstraction. Entity Framework is an abstraction. Abstracting an abstraction by wrapping EF with UoW+Repository isn't going to KISS.
That said, there are a number of things to say about your code.

public interface IRepository<T>
{
    IEnumerable<T> GetAll();
}

That seems pretty.. naked. Limited, to say the least. What if you only need to fetch a single T? You're going to GetAll and filter them on the client side with Linq-to-Objects? Linq-to-Entities has much more to offer than that. I'd expect a repository interface to support all CRUD operations!
I don't understand why GenericRepository<T> doesn't implement IRepository<T>. Looks like you can get rid of the HostRepository class

If your entity type is a Host, then this is over-complicated:

var hosts = from host in context.Hosts
            select new Host()
            {
                FirstName = host.FirstName,
                LastName = host.LastName,
                Email = host.Email
            };
return hosts.AsEnumerable().ToList();

It could be written like this:
return context.Hosts.ToList();

There's no need to project a selection of a given type into ..instances of that same type. There's no need to call AsEnumerable at all to return an IEnumerable<T>, because IQueryable<T> already implements IEnumerable<T>, which means this:
return context.Hosts;

Would return the same thing - the difference is when EF takes your query, translates it into T-SQL and sends it over to the database to materialize the results. You'll probably want to keep Linq-to-Entities within your DAL, which means you'll want to materialize the results before returning from the GetAll method - that's what ToList will do, so returning context.Hosts.ToList() is all you need to do here.

It's not clear why you're using internal, protected and virtual modifiers. A generic repository that needs to be inherited (virtual implies an eventual derived type that will override such a property/method), is missing its purpose I find - the very idea of having a generic repository is to avoid having to implement a repository for every entity type IMO.


Point where maximum intervals overlap I am solving the "Point where max intervals overlap" problem on geeks for geeks, https://www.geeksforgeeks.org/find-the-point-where-maximum-intervals-overlap/.
Following the solution in the article, I used two lists to store start and end times of the intervals. This is my solution in Python:

def maxIntervalOverlap(intervals):

    # break intervals into two lists
    first = lambda x: x[0]
    end = lambda x: x[1]

    enter = list(map(first, intervals))
    exit = list(map(end, intervals))

    # sort lists
    enter.sort()
    exit.sort()

    i = 1
    j = 0
    time = enter[0]

    n = len(enter)

    max_guests = 1
    guests_in = 1

    # process events in sorted order
    while i < n and j < n:

        # next event is arrival, increment count of guests
        if enter[i] <= exit[j]:

            guests_in += 1

            if guests_in > max_guests:
                max_guests = guests_in
                time = enter[i]

            i += 1

        else:
            guests_in -= 1
            j += 1

    print(f"Point where maximum number of intervals overlap is {time}")

and below is a test case

intervals = [[1, 4], [2, 5], [10, 12], [5, 9], [5, 12]]
maxIntervalOverlap(intervals)

How can I solve this problem without having to create the two extra lists ?

Here's a solution that only uses one extra data structure.  I use the heapq library to maintain a heap, where it easy to check the smallest item and remove it when it's not needed any longer.  You could also use SortedList from the wonderful sortedcontainers library.

The basic idea is to loop over the intervals sorted by start time. If some intervals have the same start time, then sort those by end time.  Fortunately, Python's sort() method or sorted() function will take care of that.

For each start-end interval, the end time is saved in the heap.

For any end times in the heap that are earlier than the current start time, we reduce the number of guests and remove the end time from the heap.

The start time represents the arrival of a guest, so increment the number of guests and check if it is a new maximum number.

After the last start time, the number of guests can only decrease, so we don't need to process any left over end times.

Here is the code:

import heapq

    def maxIntervalOverlap(intervals):
        guests = 0
        maxguests = 0
        maxtime = None

        heap = []

        for start, end in sorted(intervals):
            heapq.heappush(heap, end)

            # handle intervals that ended before 'start' time
            while heap[0] < start:
                heapq.heappop(heap)
                guests -= 1

            # add the guest that just arrived at 'start' time
            guests += 1
            if guests > maxguests:
                maxguests = guests
                maxtime = start

        print(f"Time with maximum guests is {maxtime}.")


Executing a task at a particular time in the morning using ScheduledExecutorService I have a task which I need to run every 6 in the morning. I have the below code which does the job and uses ScheduledExecutorService.

ScheduledExecutorService scheduler = Executors.newScheduledThreadPool(1);
Date date = new Date();
Calendar calendar = Calendar.getInstance();
calendar.setTime(date);

int hour = calendar.get(Calendar.HOUR_OF_DAY);
int delayInHour = hour < 6 ? 6 - hour : 24 - (hour - 6);

System.out.println("Current Hour: "+hour);
System.out.println("Comuted Delay for next 5 AM: "+intDelayInHour);

scheduler.scheduleAtFixedRate(new MyTask(), intDelayInHour, 24, TimeUnit.HOURS);

I am opting for code review to see whether it can be improved or not.

According to the javadoc, a Calendar instance is initialized with the current date and time. So instead of this:

Date date = new Date();
Calendar calendar = Calendar.getInstance();
calendar.setTime(date);
int hour = calendar.get(Calendar.HOUR_OF_DAY);

You don't need to create a separate Date object,
so you can simplify like this for the same effect:

Calendar calendar = Calendar.getInstance();
int hour = calendar.get(Calendar.HOUR_OF_DAY);

It would be good to split this code into multiple reusable and testable functions.
For example you could have a function that returns the hours until some target hour:

private int getHoursUntilTarget(int targetHour) {
    Calendar calendar = Calendar.getInstance();
    int hour = calendar.get(Calendar.HOUR_OF_DAY);
    return hour < targetHour ? targetHour - hour : targetHour - hour + 24;
}

Now the target hour is a parameter, this is reusable and flexible, and eliminates the magic number 6 that was duplicated at multiple places.
Also note that I changed the ternary expression a bit, I think targetHour - hour + 24 is more natural, and easier to understand due it's symmetry with targetHour - hour.

You can move the scheduling to another method, which can now have a single responsibility (scheduling) and not worry about calculating hours.

Here's a unit test to verify that getHoursUntilTarget works well:

@Test
public void testGetHoursUntilTarget() {
    Calendar calendar = Calendar.getInstance();
    int hour = calendar.get(Calendar.HOUR_OF_DAY);
    assertEquals(24, getHoursUntilTarget(hour));
    assertEquals(1, getHoursUntilTarget(hour + 1));
    assertEquals(23, getHoursUntilTarget(hour - 1));
}

Note that at 5:59 AM, the hours until target will be 1, even though 6 AM is only one minute away. This is how it was in your original code, I hope that's ok with you.

There is some noise in your post:

System.out.println("Comuted Delay for next 5 AM: "+intDelayInHour);
scheduler.scheduleAtFixedRate(new MyTask(), intDelayInHour, 24, TimeUnit.HOURS);

"next 5 AM" is not consistent with the rest of your code (you used 6 everywhere else),
and you don't have a variable intDelayInHour but you have delayInHour.
Review more carefully before you post.


GameLoop with requestAnimationFrame I am creating a HTML game where you fight monsters and stuff and this is part of the game loop for dealing damage:

var hp = 1000000000;
var speed = 10;
var power = 5;

var lastCalledTime;
var fps;

var GameLoop = function() {
    if(!lastCalledTime) {
           lastCalledTime = new Date().getTime();
           fps = 0;
           return;
    }
    delta = (new Date().getTime() - lastCalledTime)/1000;
    lastCalledTime = new Date().getTime();
    fps = 1/delta;

    hp = hp - (5 * speed * 0.1) / fps;
};

var animFrame = window.requestAnimationFrame ||
                window.webkitRequestAnimationFrame ||
                window.mozRequestAnimationFrame    ||
                window.oRequestAnimationFrame      ||
                window.msRequestAnimationFrame     ||
                null ;

if ( animFrame !== null ) {
    var recursiveAnim = function() {
        GameLoop();
        animFrame( recursiveAnim );
    };
    animFrame( recursiveAnim );
} else {
    setInterval( GameLoop, 33.33 );
}

http://jsfiddle.net/u0k7tbv4/2/

Power is how much you deal with each attack and each level of speed gives 0.1 attack / second. With 5 Power and 10 Speed it should deal 5 damage every second and that seems to be the case in the fiddle.

But I'm not sure, I only used requestAnimationFrame because I heard setInterval is unstable and so it would be nice if someone could validate the code.

Instead of var GameLoop = function() { ... };, you could just define a function the "natural" way.  Calling it gameLoop() would respect capitalization conventions better.

You neglected to use the var keyword when defining delta, so it becomes a global variable.

The GameLoop() function sets fps as a side-effect.  If you don't actually need fps anywhere else, then it should be local.  Furthermore, if you don't need fps, then there is no point in setting it to the reciprocal of delta.

I don't see much harm in initializing lastCalledTime right away, to eliminate the one-time-use special case inside GameLoop() — even if you technically didn't call GameLoop() at the time.

Calling getTime() twice is ugly.  If might also cause a tiny bit of clock skew.

var lastCalledTime = new Date().getTime();

function gameLoop() {
    var now = new Date().getTime();
    var delta = (now - lastCalledTime) / 1000;
    lastCalledTime = now;

    hp -= 0.5 * speed * delta;
};

Alternatively, if you would like lastCalledTime neatly tucked inside the gameLoop() function rather than dangling outside, you could write:

function gameLoop() {
    var now = new Date().getTime();
    if (!arguments.callee.lastCalledTime) {
        arguments.callee.lastCalledTime = now;
    }
    var delta = (now - arguments.callee.lastCalledTime) / 1000;
    arguments.callee.lastCalledTime = now;

    hp -= 0.5 * speed * delta;
};


List of integer factorization into n terms I want the fastest way in Mathematica 7 to generate a list of the factors (not prime factors) of an integer into a specified number of terms, with each factor greater than one.

For example, 240 into three terms:

{{2,2,60}, {2,3,40}, {2,4,30}, {2,5,24}, {2,6,20}, {2,8,15}, {2,10,12},
 {3,4,20}, {3,5,16}, {3,8,10}, {4,4,15}, {4,5,12}, {4,6,10}, {5,6,8}}

My starting code is based on a recursive method the source of which I have forgotten, but probably someone on projecteuler.net:

f[n_, 1, ___] := {{n}}

f[n_, k_, x_: 2] :=
 Join @@ Table[
   If[q < x, {}, {q, ##} & @@@ f[n/q, k - 1, q]],
   {q, # ~Take~ ⌈Length@#/k⌉ & @ Divisors @ n}
 ]

f[240, 3]

Memoization can be applied in several places.  In the case of large integers this is much faster, at the expense of memory usage of course.  Here, with memoization at three points:

Clear[f, div]

div@n_ := div@n = Divisors@n

div[n_, k_] := div[n, k] = # ~Take~ ⌈Length@#/k⌉ & @ div @ n

f[n_, 1, ___] := {{n}}

f[n_, k_, x_: 2] := f[n, k, x] =
 Join @@ Table[
   If[q < x, {}, {q, ##} & @@@ f[n/q, k - 1, q]],
   {q, n~div~k}
  ]

Compared to Sasha's code on a large composite integer:

f[10080^2, 5] // Length // Timing

   {0.891, 103245}

FactorIntoFixedTerms[10080^2, 5] // Length // Timing

   {25.594, 103245}

Can this be further improved?

Because Divisor is quite efficient at its job, the only optimization I see, is to avoid unnecessary calls to it, by using memoization technique:

FactorIntoFixedTerms[in_Integer, terms_Integer] := Block[{f, div},
  div[n_] := (div[n] = Divisors[n]);
  f[n_, 1] := {{n}};
  f[n_, k_] :=
   Join @@ Table[{q, ##} & @@@
      Select[f[n/q,
        k - 1], #[[1]] >=
         q &], {q, #[[2 ;; \[LeftCeiling]Length@#/
             k\[RightCeiling]]] &@div@n}];
  f[in, terms]
  ]

This can lead to substantial timing reduction:

In[683]:= FactorIntoFixedTerms[1453522322112240, 6] // Length // Timing

Out[683]= {0.047, 47}

In[685]:= foriginal[1453522322112240, 6] // Length // Timing

Out[685]= {0.39, 47}


Invert bits of binary representation of number This is the code I came up with.

I added comments to make the solution more verbose.

int findComplement(int num) {
    // b is the answer which will be returned
    int b = 0;

    // One bit will be taken at a time from num, will be inverted and stored in n for adding to result
    int n = 0;

    // k will be used to shift bit to be inserted in correct position
    int k = 0;

    while(num){
        // Invert bit of current number
        n = !(num & 1);

        // Shift the given number one bit right to accesss next bit in next iteration
        num = num >>1 ;

        // Add the inverted bit after shifting
        b = b + (n<<k);

        // Increment the number by which to shift next bit
        k++;
    }
    return b;
}

Is there any redundant statment in my code which can be removed? Or any other better logic to invert bits of a given integer

int n = 0; This initialization is not used.  It could simply be int n;, or could be int n = !(num & 1); inside the loop, to restrict the scope of n.

This loop:

int k = 0;
while (num) {
    ...
    k++;
}

could be written as:

for(int k = 0; num; k++) {
    ...
}

Since you are doing bit manipulation, instead of using addition, you should probably use a “binary or” operation to merge the bit into your accumulator:

    b = b | (n << k);

or simply:

    b |= n << k;

Bug

You are not inverting the most significant zero bits.  Assuming an 8-bit word size, the binary compliment of 9 (0b00001001) should be 0b11110110, not 0b00000110.   And the compliment of that should return to the original number (0b00001001), but instead yields 0b00000001.

And, as mentioned by @Martin R, you could simply return ~num;