Serializing types with a common superclass I have written code responsible for saving data to a file using Serialization. I think that a natural way to avoid repetition is to use inheritance but I'm not sure if it would work because of generics.

How can I remove all the repetition in the below code?

Some general information:

There is a class StoredItem and classes PipeCable, Element, ValveMotor which extend StoredItem.
There is Data class in which are ObservableLists of PipeCable, Element, ValveMotor

The code that I'd like to improve but I don't know how:

private void savePipeCableList(Data data) {
    try {
        Path path = Paths.get(Strings.dataPipeCableFileName);
        OutputStream fos = Files.newOutputStream(path);
        ObjectOutputStream oos = new ObjectOutputStream(fos);
        oos.writeObject(new ArrayList<PipeCable>(data.getPipeCableList()));
        oos.close();
    } catch (FileNotFoundException e) {
        e.printStackTrace();
    } catch (IOException e) {
        e.printStackTrace();
    }
}

private void saveElementList(Data data) {
    try {
        Path path = Paths.get(Strings.dataElementFileName);
        OutputStream fos = Files.newOutputStream(path);
        ObjectOutputStream oos = new ObjectOutputStream(fos);
        oos.writeObject(new ArrayList<Element>(data.getElementList()));
        oos.close();
    } catch (FileNotFoundException e) {
        e.printStackTrace();
    } catch (IOException e) {
        e.printStackTrace();
    }
}

private void saveValveMotorList(Data data) {
    try {
        Path path = Paths.get(Strings.dataValveMotorFileName);
        OutputStream fos = Files.newOutputStream(path);
        ObjectOutputStream oos = new ObjectOutputStream(fos);
        oos.writeObject(new ArrayList<ValveMotor>(data.getValveMotorList()));
        oos.close();
    } catch (FileNotFoundException e) {
        e.printStackTrace();
    } catch (IOException e) {
        e.printStackTrace();
    }
}

I will appreciate any suggestions.

I think in this case you want a generic method, something on this general order:

static <T> private void saveGeneric(ArrayList<T> data, Path path) {
    try {
        OutputStream fos = Files.newOutputStream(path);
        ObjectOutputStream oos = new ObjectOutputStream(fos);
        oos.writeObject(data);
        oos.close();
    } catch (FileNotFoundException e) {
        e.printStackTrace();
    } catch (IOException e) {
        e.printStackTrace();
    }
}

Then you might want to write the individual wrappers to match the functions you started with:

private void savePipeCableList(Data data) {
    saveGeneric(new ArrayList<PipeCable>(data.getPipeCableList()),
                Paths.get(Strings.dataPipeCableFileName));
}

private void saveElementList(Data data) {
    SaveGeneric(new ArrayList<Element>(data.getElementList()),
                Paths.get(Strings.dataElementFileName));
}

private void saveValveMotorList(Data data) {
    SaveGeneric(new ArrayList<ValveMotor>(data.getValveMotorList()),
                Paths.get(Strings.dataValveMotorFileName));
}

As for the code otherwise, the point that seems most obvious to me would be to replace the mostly manual opening and closing of the stream with a try with-resources statement:

// Note: I generally avoid Java, so I probably got at least a few details of
// the syntax wrong here.
OutputStream fos = Files.newOutputStream(path);
try (ObjectOutputStream oos = new ObjectOutputStream(fos))
{
    oos.WriteObject(data);
}
catch(FileNotFoundException e) {
    // ...

This will automatically close oos upon exit from the try block, regardless of whether you exit normally or via an exception. With the code as you've written it, if an exception is thrown while writing, oos.close() won't execute.

It's not immediately clear whether fos might benefit from similar treatment--without knowing how File.newOutputStream works and what it does, I can't really even guess about that (but perhaps you now know more about what to look for and consider). Right now, you have the Files.newOutputStream inside the try block (which I don't) but if you're just getting an existing object that you know exists, that won't matter. If it could throw an exception, you may have a more difficult problem, where you'd need nested try blocks (the inner of the "with resources" variety) to ensure correct behavior.

One other point. I hesitate to mention it, because I might just be demonstrating my ignorance of Java, but at least offhand, it seems like you probably don't need to create a complete copy of the data in memory before you write it to the output stream:

oos.writeObject(new ArrayList<ValveMotor>(data.getValveMotorList()));

At least offhand, it seems like it should be sufficient to use:

oos.writeObject(data.getValveMotorList());

The obvious reason we'd copy the data before serializing it would be if writeObject were asynchronous, and we need want to ensure we get a stable snapshot of the data at the time we started the serialization. If memory serves, writeObject is synchronous though.


20×20 Game of Life in C I am new to C, but have experience with Python and R. The following code implements the Game of Life and represents my first finger exercise in C. I would like to have some feedback regarding the coding style and conventions in the C community. (I commented out the two Windows specific lines, in case someone wants to run it on another operating system)

//gcc -Wextra -Wall -Werror -O3 -march=native -std=c99 -o test.exe gameOfLife.c

#include <stdio.h>
#include <stdbool.h>
#include <string.h>
#include <time.h>
#include <stdlib.h>
//#include <Windows.h>

void initField(bool field[],
               const size_t rows,
               const size_t cols,
               const float fill)
{
    // fill everything with zeros
    memset(field, 0, rows*cols*sizeof(bool));

    // fill the desired amount of entries with ones
    srand(time(0));
    for(size_t r = 1; r < rows-1; ++r)
        for(size_t c = 1; c < cols-1; ++c)
            if(rand() / (float)RAND_MAX < fill)
                field[r*rows + c] = 1;
}

unsigned countNeighbors(bool field[], const size_t rows, const size_t entry)
{
    return field[entry-rows-1] + field[entry-rows] + field[entry-rows+1] +
           field[entry-1] +                          field[entry+1] +
           field[entry+rows-1] + field[entry+rows] + field[entry+rows+1];
}

void updateField(bool field[], const size_t rows, const size_t cols)
{
    // create a new field filled with zeros
    bool tmp[rows * cols];
    memset(tmp, 0, rows*cols*sizeof(bool));

    // apply update rules and set some entries to one
    for(size_t r = 1; r < rows-1; ++r)
        for(size_t c = 1; c < cols-1; ++c) {
            size_t entry = r*rows + c;
            unsigned neighbors = countNeighbors(field, rows, entry);
            // update entry based on neighbors
            if(neighbors == 3 || (neighbors == 2 && field[entry]))
                tmp[entry] = 1;
        }

    // update the actual field
    memcpy(field, tmp, rows*cols*sizeof(bool));
}

void printField(bool field[], const size_t rows, const size_t cols)
{
    for(size_t r = 0; r < rows; ++r) {
        for(size_t c = 0; c < cols; ++c)
            printf("%i", field[r*rows + c]);
        printf("\n");
    }
    printf("\n");
}

int main()
{
    // actual dimensions are (rows-1)*(cols-1), but for convenience
    // we are going to create a border around it filled with zeros
    const size_t rows = 21;
    const size_t cols = 21;
    bool field[rows * cols];

    // initialize the field with ~20% of entries set to 1
    initField(field, rows, cols, 0.2);

    // print and update the field for some iterations
    for(unsigned i = 0; i < 100; ++i) {
        printField(field, rows, cols);
        updateField(field, rows, cols);
        //Sleep(500);
    }

    return 0;
}

//gcc -Wextra -Wall -Werror -O3 -march=native -std=c99 -o test.exe gameOfLife.c

This should probably be put into some Makefile (well... the Makefile can be written more idiomatically, but I'm not the person to really ask about Makefiles...)

srand(time(0));

There may be better alternatives to this as a random number generator. This answer seems to suggest that Mersenne Twisters produce higher quality pseudo random numbers and that it is faster. However, informally speaking Mersenne Twisters are no longer consider "high quality enough".

To clarify, I am suggesting the alternative not so much because the output is a "higher quality" random number, but because of speed concerns.

This:

if(rand() / (float)RAND_MAX < fill)

this:

for(size_t c = 0; c < cols; ++c)

and this:

if(neighbors == 3 || (neighbors == 2 && field[entry]))

They all don't use braces. Please use braces. To put this in to context, there was a OpenSSL bug in Mac OSX. IF they had used braces, the double goto fail would not have mattered*. It caused a lot of issues. So I would advise at the very least you put the statements on the same line if you don't use brackets.

* Well, it could have mattered, but autocompletion of brackets in an IDE would mitigate the chances. Better safe than sorry!


Query to find users from Australia After a discussion of which users were from Australia, I wrote my first SQL query to find out:
SELECT u.DisplayName'Display Name', u.Reputation'Rep', u.Location'Location'
FROM Users u
WHERE u.Location LIKE '%Australia%'
ORDER BY 'Rep' DESC

As always, please tell me the good, the bad, and the ugly.
The query can be found here

I don't like how you're specifying the column aliases. I expect a whitespace between the column and the alias.

I like that you're not specifying the optional AS keyword though - I find it only adds clutter when it's there.

Also I would have used [square brackets] instead of single quotes, and layout the field names on separate lines, like this:

SELECT
     u.DisplayName [Display Name]
    ,u.Reputation [Rep]
    ,u.Location [Location]

That way you can easily add, reorder, or comment-out a column if you need to.


Pascal's Triangle - Remove hack I had a thought last night on how I could easily write a Pascal's Triangle generator basically just using partition and map. I tried writing it this morning, and it got slightly more complicated than I originally thought it would, although it's still pretty nice.

My main concern is the wrap "hack" that I'm using. I found two similar, but different ways of writing it. I can either wrap the input to the iteration function in 0s, or I can wrap the result of the iteration function in 1s. Both give the same result, but the need for a wrapping helper pollutes an otherwise nice ->> chain. If anyone can think of how I can avoid a separate anonymous function here, and ideally avoid wrapping the input/output in 0s/1s, I'd appreciate it. I'll take any other critique as well, although there isn't much else going on here.

Usage:

(let [pre-t (pascals-triangle-pre) ; Wrapping pre-processing
      post-t (pascals-triangle-post) ; Wrapping post-processing
      n 5]
  (println (take n pre-t))
  (println (take n post-t)))

([1] [1 1] [1 2 1] [1 3 3 1] [1 4 6 4 1])
([1] [1 1] [1 2 1] [1 3 3 1] [1 4 6 4 1])

(ns minesweeper.irrelevant.pas-tri)

(defn pascals-triangle-post []
  (let [wrap #(vec (concat [1] % [1]))]
    (iterate #(->> %
                   (partition 2 1)
                   (mapv (partial apply +'))
                   (wrap))
             [1])))

(defn pascals-triangle-pre []
  (let [wrap #(vec (concat [0] % [0]))]
    (iterate #(->> %
                   (wrap)
                   (partition 2 1)
                   (mapv (partial apply +')))
             [1])))

There's a simpler way.

To get the next line

take two copies of the line,
extend them respectively with 0 at the start and with 0 at the end, and
add the corresponding elements.

In Clojure,

(defn pascal []
  (iterate
    #(mapv + (cons 0 %) (conj % 0))
    [1]))

=> (take 5 (pascal))
([1] [1 1] [1 2 1] [1 3 3 1] [1 4 6 4 1])

This avoids the partitioning.

Not sure if it's what you're looking for, but showing off was irresistible.


Adding list of customcontrols This is the basic class that I am trying to display:

public class Day
{
    public DateTime Date { get; private set; }

    public SpecialDay SpecialDay { get; set; }

    public DateTime TimeIn { get; private set; }

    public DateTime TimeOut { get; private set; }

    public DayOfWeek DayOfWeek

    public TimeSpan TotalHours
}

It is contained inside a list in in following class:

public class Month
{
    private int _month { get; set; }
    private int _year { get; set; }

    public DateTime Date

    public List<Day> Days { get; private set; }

    public TimeSpan TotalHoursInMonth
}

This is the load function of the main display form

private void MonthDataForm_Load(object sender, EventArgs e)
    {
        int bottomOfPreviousDay = -1;

        foreach (var item in MonthDisplaying.Days)
        {
            DayView dayDisplay = new DayView(item);

            if (bottomOfPreviousDay == -1)
            {
                dayDisplay.Top = this.Top + 10;
            }
            else
            {
                dayDisplay.Top = bottomOfPreviousDay + 2;
            }

            bottomOfPreviousDay = dayDisplay.Bottom;
            this.Controls.Add(dayDisplay);
        }

        this.WindowState = FormWindowState.Maximized;
    }

The form shows a list of days for a given month like so

Each line is a separate instance of the Day class, and in the next stage of development there will be buttons to perform different actions on each instance.

Is there a better way for calculating the position of the user controls that are being added?  I don't like the way the if/else looks but I am not sure how else to do it.

As you seem to specifically ask for: Is there a better way for calculating the position of the user controls that are being added? I don't like the way the if/else looks but I am not sure how else to do it.

My answer will address only this. You can make a better use of the variable and instead of -1 use Top + 10 and later = Bottom + 2. The code:

    int nextTop = this.Top + 10;

    foreach (var item in MonthDisplaying.Days)
    {
        DayView dayDisplay = new DayView(item);

        dayDisplay.Top = nextTop;

        nextTop = dayDisplay.Bottom + 2;

        this.Controls.Add(dayDisplay);
    }


Pig Latin translator with too many `push_str`s I'm learning Rust and one of the challenges in the book was to build a pig latin translator.

  Convert strings to pig latin. The first consonant of each word is moved to the end of the word and “ay” is added, so “first” becomes “irst-fay.” Words that start with a vowel have “hay” added to the end instead (“apple” becomes “apple-hay”).

I came up with the code below and I'm looking for ways to improve it, specifically reducing the push_strs and improving the starts_with_a_vowel method. How can the code be improved?

main.rs

extern crate unicode_segmentation;
use unicode_segmentation::UnicodeSegmentation;

fn main() {
    println!("{}", pig_latinify("String me".to_string()));
}

fn transform_word(word: &str) -> String {
    let mut new_word = String::new();
    new_word.push_str(&word[1..]);
    new_word.push_str(&word[..1]);
    if starts_with_a_vowel(word) {
        new_word.push_str("h");
    }
    new_word.push_str("ay");
    new_word
}

fn starts_with_a_vowel(word: &str) -> bool {
    let vowels = ["a", "e", "i", "o", "u"];
    for vowel in &vowels {
        if word.starts_with(vowel) {
            return true;
        }
    }
    false
}

fn pig_latinify(statement: String) -> String {

    let mut transformed_statement = String::new();
    if !statement.is_ascii() {
        panic!("statement must be ascii");
    }
    let iter = statement.split_word_bounds();
    for word in iter {
        if word.trim().len() > 0 {
            transformed_statement.push_str(&transform_word(word));
            transformed_statement.push_str(" ");
        }
    }
    transformed_statement.trim();
    transformed_statement
}

Cargo.toml

...
[dependencies]
unicode-segmentation = "1.2.0"

Write tests

Unfortunately, your piglatify doesn't follow your rules. apple gets transformed into applyaay, but it should get transformed into apple-hay. Also, first should get transformed into irst-fay, but you will transform it into irstfay.

Rust makes it really easy to add tests to your code:

#[cfg(test)]
mod test {
    use transform_word;

    #[test]
    fn empty_string() {
        assert_eq!(transform_word(""), "");
    }

    #[test]
    fn apple() {
        assert_eq!(transform_word("apple"), "apple-hay");
    }

    #[test]
    fn first() {
        assert_eq!(transform_word("first"), "irst-fay");
    }
}

While this only uses the given strings from the exercise, it already tells us that there is something amiss if we run the tests with cargo test:

running 3 tests
test test::empty_string ... FAILED
test test::first ... FAILED
test test::apple ... FAILED

failures:

---- test::empty_string stdout ----
    thread 'test::empty_string' panicked at 'byte index 1 is out of bounds of ``', /checkout/src/libcore/str/mod.rs:2217:9
note: Run with `RUST_BACKTRACE=1` for a backtrace.

---- test::first stdout ----
    thread 'test::first' panicked at 'assertion failed: `(left == right)`
  left: `"irstfay"`,
 right: `"irst-fay"`', src/main.rs:57:9

---- test::apple stdout ----
    thread 'test::apple' panicked at 'assertion failed: `(left == right)`
  left: `"ppleahay"`,
 right: `"apple-hay"`', src/main.rs:52:9

Use with_capacity if possible

You can use String::with_capacity in transform_word. The new word has the same length as the old one plus four:

fn transform_word(word: &str) -> String {
    if word.is_empty() {
        return String::new();
    }
    let mut new_word = String::with_capacity(word.len() + 4);

    // still buggy, see above
    new_word.push_str(&word[1..]);
    new_word.push_str(&word[..1]);
    if starts_with_a_vowel(word) {
        new_word.push_str("h");
    }
    new_word.push_str("ay");
    new_word
}

However, we still need to fix the bug mentioned before:

fn transform_word(word: &str) -> String {
    if word.is_empty() {
        return String::new();
    }
    let mut new_word = String::with_capacity(word.len() + 4);

    if starts_with_a_vowel(word) {
        new_word.push_str(&word);
        new_word.push_str("-hay");
        new_word
    } else {
        new_word.push_str(&word[1..]);
        new_word.push('-');
        new_word.push_str(&word[..1]);
        new_word.push_str("ay");
        new_word
    }
}

Alternative variants

Truth be told, that looks somewhat silly. We can just use += instead of push_str, or even +:

fn transform_word(word: &str) -> String {
    if word.is_empty() {
        return word.to_owned();
    }
    if starts_with_a_vowel(word) {
        word.to_owned() + "-hay"
    } else {
        word[1..].to_owned() + "-" + &word[..1] + "ay"
    }
}

Speaking about alternatives, if you use any then starts_with_a_vowel can be written in a single line:

fn starts_with_a_vowel(word: &str) -> bool {
    "aeiou".chars().any(|vowel| word.starts_with(vowel))
}

Note that we use a Char as pattern, not a String. starts_with works with both, but this variant shows our intend to match only a single character.

Ownership

It's not necessary to take ownership of the given String in pig_latinify.


Setting alternative records true if its even count else leave last record I have an attendance list like this.

problem: I need to set In and Out alternatively. but if I have only 3 records then I need to set 0 record In true. 1 record out is true. and 3 record has is both In Out is false. I tried like this its working fine. but I was checking is there any better way

ObservableCollection<EmployeeAttandance> attendancesPerDay = new ObservableCollection<EmployeeAttandance>();
if (attendancesPerDay.Count % 2 == 0)
{
    int counter = 0;
    foreach (var attendance in attendancesPerDay)
    {
        if (counter % 2 == 0)
            attendance.In = true;
        else
            attendance.Out = true;
        counter++;
    }
}
else
{
    int counter = 0;
    foreach (var attendance in attendancesPerDay)
    {
        if (attendancesPerDay.IndexOf(attendance) == attendancesPerDay.Count - 1)
            continue;
        if (counter % 2 == 0)
            attendance.In = true;
        else
            attendance.Out = true;
        counter++;
    }
}

In the else clause it might be better to have an if statement with a block of code than a continue statement:

    foreach (var attendance in attendancesPerDay)
    {
        if (attendancesPerDay.IndexOf(attendance) != attendancesPerDay.Count - 1)
        {
            if (counter % 2 == 0)
                attendance.In = true;
            else
                attendance.Out = true;
            counter++;
        }
    }

The logic is basically the same and it is easier to see the scope of the code.

To reduce the repetition of the code there could be a short function that takes 2 parameters, counter and attendance and performs the assignment.

As a personal choice I prefer to specify the real type in a foreach loop because it is self documenting, the user of var hides too much.


Regexes for Google App Engine I want to review the URL routing for my appengine webapp:

routes = [
        (r'/', CyberFazeHandler),
        (r'/vi/(eyes|mouth|nose)', CyberFazeHandler),
        (r'/realtime', RealtimeHandler),
        (r'/task/refresh-user/(.*)', RefreshUserHandler),
        ('/ai', FileUploadFormHandler),
        ('/serve/([^/]+)?', ServeHandler),
        ('/upload', FileUploadHandler),
        ('/generate_upload_url', GenerateUploadUrlHandler),
        ('/file/([0-9]+)', FileInfoHandler),
        ('/file/set/([0-9]+)', SetCategoryHandler),
        ('/file/([0-9]+)/download', FileDownloadHandler),
        ('/file/([0-9]+)/success', AjaxSuccessHandler),
        ]

app = webapp2.WSGIApplication(routes,
            debug=os.environ.get('SERVER_SOFTWARE', '').startswith('Dev'
            ))

Does it look alright to you? Can you recommend an improvement? Should I use the 'r prefix to my regexes?

Have you considered using named Route templates instead of capturing regular expressions? It could make the code more readable. Consider

Route("/task/refresh-user/<username>", RefreshUserHandler)

instead of

(r'/task/refresh-user/(.*)', RefreshUserHandler)

for example. (Of course, I don't know what kwargs RefreshUser actually wants, but you can change the angle-bracketed part to the appropriate name.)


Function that takes a positive integer number and returns the next bigger number formed by the same digits I am taking on this challenge on CodeWars. You have to create a function that takes a positive integer number and returns the next bigger number formed by the same digits:

next_bigger(12)==21
next_bigger(513)==531
next_bigger(2017)==2071

This is my solution:

 def next_bigger(s)
      s.to_s.split("").permutation.to_a.map {|a| a.join("").to_i }.reject { |n| n <= s }.inject do |memo, p|
      (memo-s) < (p-s) ? memo : p
  end
 end

That passes these tests:

Test.assert_equals(next_bigger(12),21)
Test.assert_equals(next_bigger(513),531)
Test.assert_equals(next_bigger(2017),2071)
Test.assert_equals(next_bigger(414),441)
Test.assert_equals(next_bigger(144),414)

But I am being told my code is too inefficient. I am doing a few of these challenges each day to improve my ruby (I am a beginner). Could someone recommend how this could be made more efficient?

The question of finding the next permutation in lexicographic order is a classical one (going back 7 centuries) and well documented in the literature. E.g. Wikipedia gives the following algorithm:

  Find the largest index k such that a[k] < a[k + 1]. If no such index exists, the permutation is the last permutation.
  Find the largest index l greater than k such that a[k] < a[l].
  Swap the value of a[k] with that of a[l].
  Reverse the sequence from a[k + 1] up to and including the final element a[n].


Count check before Linq Single() call Should I check Count before Linq Single() call if I expect only one element?

if (objects.Count != 1)
{
    throw new
        InvalidOperationException(
            "%Collection% should contain one %element%, but now it's: "
                 + objects.Count);
        }

return objects.Single();

Single implements the same check and throwing. But its message is less specific.

P.S. in my case wrong count is handled just writing log message like

_log.Error("Error during %operation%", ex);
task.Complete(TaskResult.Fail);

Yep, go for it. Adding more detail to error messages is helpful.

Also, depending on which style you prefer you could avoid explicitly checking the count by catching the exception thrown by Single().

try
{
    return objects.Single();
}
catch (InvalidOperationException ex)
{
    throw new InvalidOperationException(
        "%Collection% should contain one %element%, but now it's: " + objects.Count,
        ex
    );
}


Sorting lists by various measures of goodness def sort[A, B: Ordering](data: List[A], desc: Boolean)(measure: A => B): List[A] = {
        val baseOrdering = Ordering.by(measure)
        val ordering = if (desc) baseOrdering.reverse else baseOrdering
        data.sorted(ordering)
    }

  private def sortOne(stats: List[One], by: String, desc: Boolean): List[One] =
    by match {
      case "good" => sort(stats, desc)(_.data.good)
      case "bad" => sort(stats, desc)(_.data.bad)
      case "gross" => sort(stats, desc)(_.data.gross)
      case "warn" => sort(stats, desc)(_.data.warn)
      case "exuberant" => sort(stats, desc)(_.data.exuberant)
      case other => stats
    }
  private def sortTwo(stats: List[Two], by: String, desc: Boolean): List[Two] =
    by match {
      case "good" => sort(stats, desc)(_.data.good)
      case "bad" => sort(stats, desc)(_.data.bad)
      case "gross" => sort(stats, desc)(_.data.gross)
      case "warn" => sort(stats, desc)(_.data.warn)
      case "exuberant" => sort(stats, desc)(_.data.exuberant)
      case other => stats
    }

  private def sortThird(stats: List[Third], by: String, desc: Boolean): List[Third] =
    by match {
      case "good" => sort(stats, desc)(_.data.good)
      case "bad" => sort(stats, desc)(_.data.bad)
      case "gross" => sort(stats, desc)(_.data.gross)
      case "warn" => sort(stats, desc)(_.data.warn)
      case "exuberant" => sort(stats, desc)(_.data.exuberant)
      case other => stats
    }

Actually there are more classes. They all have same fields like good, gross, bad, warn.

These classes that are in methods have all similar fields good, gross, bad, warn. I can't rewrite classes.

It is working, but it looks bad. Maybe someone has some ideas?

You could try that:

def sort[A, B: Ordering](data: List[A], desc: Boolean)(measure: A => B): List[A] = {
    val baseOrdering = Ordering.by(measure)
    val ordering = if (desc) baseOrdering.reverse else baseOrdering
    data.sorted(ordering)
}

Then you do

sort(listOfFirstClass, desc)(_.good)


Fetching a set of event IDs from a database based on a set of company IDs The following code fetches a set of company IDs from a database if there are any events associated with a company that has a missing property. Because the set of company IDs is embedded into the SQL query using parameters, and the number of parameters may exceed the maximum for the RDBMS / database driver, the set is partitioned into maximal subsets.

Performance aside, in terms of maintainability and ability to debug, is the following "too Linq-y"?

HashSet<int> missingCompanyIds =
    companyEvents
        .Select(ev => ev.CompanyId).ToHashSet()
        .Partition(DataContext.MaxAvailableSqlParameters(usedParameters), true)
        .SelectMany(companyIdsPartition =>
                missingEventsQuery                                           // linq-to-sql
                    .Where(ev => companyIdsPartition.Contains(ev.CompanyId)) // linq-to-sql
                    .Select(ev => ev.CompanyId)                              // linq-to-sql
                    .Distinct())                                             // linq-to-sql
        .ToHashSet();

If any of the database side-effects of the query methods cause a crash, perhaps this is harder to debug, I don't know. I am able to set breakpoints inside lambdas, but my local environment might contain fewer of the objects I wish to inspect when debugging (e.g. the missingEventsQuery)? I'm unsure if these concerns would be relevant in practice.

Would there be any advantages in the case above if some of the .Select()s were written as for loops?

Not so a direct answer to your question but this may help you.

Most of methods you are using are really extension methods, If your only worry is debugging or logging, you can get code for these methods and create your own extension methods with same names but Namespace must be different.

As these are extension methods, you will not be able to override them.


Binning and calculating a huge range of data I have a code that I'm fairly certain does what I want. Bear with me as I explain what I'm doing:

Imagine that there is 10 houses, where there can be one to an infinite number of persons. Each of those persons sends a number of messages, containing their userid and the house number. This can be from 1 to infinite number of messages. I want to know the average number of messages that is sent by each person, for each house, to later plot which house got the largest number of average messages.

Now, that I've explained conceptually, the houses aren't houses, but latitudes, from f.ex -90 to -89 etc. And that a person can send messages from different houses.

This is some sample input:

lat = [-83.76, -44.88, -38.36, -35.50, -33.99, -31.91, -27.56, -22.95,
       -19.00, -12.32,  -6.14,  -1.11,   4.40,  10.23,  19.40,  31.18,
        40.72,  47.59,  54.42,  63.84,  76.77]

userid= [525, 701, 701, 520, 701, 309, 373, 255,
         372,  636, 529, 529, 529, 775,   345,  636,
          367, 366,  372,  251,  273]

The real input is some houndred million values in the range of -90 to 90 for latitudes, and different numbers in no set range for the userid. There are about 100 000 unique userids. These are stored in an SQLite database.

"My code" takes about 13s with 1/300th of my data, so I guess there is a lot of possibilities for optimization. I got the code with great help from J-Richard-Snape in this question. Note that it is my code down until min_lat so he is not held accountable for the code over that.  Any suggestions?

import sqlite3 as lite
import numpy as np
import matplotlib.pyplot as plt
from itertools import groupby

def getQuantity(databasepath):
    latitudes = []
    userids = []
    info = []
    con = lite.connect(databasepath)
    with con:
        cur = con.cursor()
        cur.execute('SELECT latitude, userid FROM message')
        con.commit()
        while True:
            tmp = cur.fetchone()
            if tmp != None:
                info.append([float(tmp[0]),int(tmp[1])])
            else:
                break
        info = sorted(info, key=itemgetter(0))
        for x in info:
            latitudes.append(x[0])
            userids.append(x[1])
        info = []
        tmp = 0
        min_lat = -90
        max_lat = 90
        binwidth = 1

        bin_range = np.arange(min_lat,max_lat,binwidth)

        all_rows = zip(latitudes,userids)
        binned_latitudes = np.digitize(latitudes,bin_range)
        all_in_bins = zip(binned_latitudes,userids)
        unique_in_bins = list(set(all_in_bins))
        all_in_bins.sort()
        unique_in_bins.sort()

        bin_count_all = []
        for bin, group in groupby(all_in_bins, lambda x: x[0]):
            bin_count_all += [(bin, len([k for k in group]))]

        bin_count_unique = []
        for bin, group in groupby(unique_in_bins, lambda x: x[0]):
            bin_count_unique += [(bin, len([ k for k in group]))]

        bin_density = [(bin_range[b-1],a*1.0/u) for ((b,a),(_,u)) in         zip(bin_count_all, bin_count_unique)]

        bin_density =  np.array(bin_density).transpose()

        # all_in_bins and unique_in_bins now contain the data
        # corresponding to the SQL / pseudocode in your question

        # plot as standard bar - note you can put uneven widths in as an         array-like here if necessary
        plt.bar(*bin_density, width=binwidth)

Especially info.append([float(tmp[0]),int(tmp[1])]) causes a memory error.

You call con.commit() unnecessarily — there is no change to be committed.  You defined all_rows, but never use it.  But those are just minor inefficiencies.

The point of using a database is that you can ask it to tell you interesting things about your data.  If you are just going to dump out the entire contents of a table for analysis, you might as well use a CSV file.

A query like this would be ideal:

SELECT FLOOR(latitude) AS latitude
     , COUNT(userid) AS message_count
     , COUNT(DISTINCT userid) AS distinct_count
    FROM message
    GROUP BY 1
    ORDER BY 1;

Unfortunately, SQLite lacks a FLOOR() function, so a circumlocution is necessary:

SELECT CASE WHEN latitude = CAST(latitude AS INTEGER) OR latitude >= 0 THEN CAST(latitude AS INTEGER)
            ELSE CAST(latitude - 1 AS INTEGER)
       END AS latitude
     , COUNT(userid) AS message_count
     , COUNT(DISTINCT userid) AS distinct_count
    FROM message
    GROUP BY 1
    ORDER BY 1;

Note that a simple CAST (latitude AS INTEGER) would be inappropriate for histogram binning, as casting truncates towards zero, such that -0.99 < latitude < +0.99 would all map to 0.

It should be trivial to read the results from that query into bin_count_all and bin_count_unique.

Note that to make that query run efficiently, it would be wise to

CREATE INDEX message_latitude_userid ON message (latitude, userid);


Minimal CORS mail PHP I have an PHP endpoint used to mail me feedback or error logs from my sites here: https://github.com/kaihendry/vanilla-php-feedback-form/blob/master/feedback/feedback.php

<?php
switch ($_SERVER['HTTP_ORIGIN']) {
    case "https://example.com":
        header("Access-Control-Allow-Origin: " . $_SERVER['HTTP_ORIGIN']);
        break;
    default:
        die("Bad origin: " . $_SERVER['HTTP_ORIGIN']);
}
// Access-Control headers are received during OPTIONS requests
if ($_SERVER['REQUEST_METHOD'] == 'OPTIONS') {
    if (isset($_SERVER['HTTP_ACCESS_CONTROL_REQUEST_METHOD']))
        header("Access-Control-Allow-Methods: GET, POST, OPTIONS");
    if (isset($_SERVER['HTTP_ACCESS_CONTROL_REQUEST_HEADERS']))
        header("Access-Control-Allow-Headers: {$_SERVER['HTTP_ACCESS_CONTROL_REQUEST_HEADERS']}");
    exit;
}
$subject = 'Feedback from ' . $_SERVER['REMOTE_ADDR'] . ' ' . $_SERVER['HTTP_REFERER'];
$input = json_decode(file_get_contents('php://input'), true);
if(empty($input["msg"])) { http_response_code(400); die(); }
$message = json_encode($input, JSON_PRETTY_PRINT) . "\n\n--\n" . json_encode($_SERVER, JSON_PRETTY_PRINT);
if(empty($input["from"]) || !filter_var($input["from"], FILTER_VALIDATE_EMAIL)) {
    mail('me@example.com', $subject, $message);
} else {
    $headers = 'From: ' . $input["from"];
    mail('me@example.com', $subject, $message, $headers);
}
?>

There are a couple of problems:

It's slow. I suspect it's mail(), which is used with ssmtp & AWS SES. It's fast when I comment the mail line out. Can it be backgrounded somehow?
Should I be returning JSON like status OK or not OK if mail() fails? What is best practice?
Is there an easy way of compiling them into a digest or maybe better alternatives to email I may have missed? I'm a bit worried someone can abuse this script using cURL. How can I prevent that sanely?

1

From looking on the script, seems there is nothing that can be slow except of the mail(). The problem is, it needs to connect with the target mail server and thats often very slow.

mail() got many problems so it is best avoid using it. You can read about it here and here

The best alternative is probably Swiftmailer http://swiftmailer.org/. With that  you can solve also your "in background sending" using spool. Your mails will be send after the script finished running.

For the next two questions I would advise to handle your script like an HTTP API.

2

With Spool

you cannot know in real time if sending of your email was an success, as this will happen after your script will terminate.

Without Spool

it could still be faster (or not), but you will know if it was ok right away

What to return depend now on the above

You can use standard response codes, almost for all your scenarios. http_status_codes
You don't need to return anything in the body most of the time.

Fail:

400 - Bad Request - when the request is missing $msg, or body json is malformed
401 - Unauthorized - when missing http_origin or not in the list of allowed
405 - Method not allowed - if it is not POST or OPTIONS
409 - Conflict - email sending failed (if not usign spool)

Success:

200 - OK - email send successfully (if not using spool)
202 - Accepted - when everything is ok, but using spool so you don't know if the sending of email was successfull

In an API approach you would be able to call some url to check if your Accepted request was done processing or not, but this is another topic.

3

As I said before, this is basically an API, so all methods how to secure an API apply here. You can study those.
If you want to go simple and quick the easier ones are:

SSL + Http Basic Auth - simple basic auth when going over https is quite secure
SSL + ApiKey in header + Token Auth - basic thing for all API

I would not recommend going without SSL as you can always catch the request and read the keys.


Generic Cache struct I've came up with the following implementation of a very simple cache:

use std::cmp::Eq;
use std::collections::HashMap;
use std::hash::Hash;

pub struct Cache<I, R, F>
    where I: Copy + Eq + Hash, F: Fn(I) -> R
{
    calculation: F,
    values: HashMap<I, R>,
}

impl<I, R, F> Cache<I, R, F> where I: Copy + Eq + Hash, F: Fn(I) -> R {
    pub fn new(calculation: F) -> Cache<I, R, F> {
        Cache { calculation, values: HashMap::new() }
    }
    pub fn value(&mut self, arg: I) -> &R {
        // Pop or calculate new value
        let result = match self.values.remove(&arg) {
            Some(v) => v,
            None => (self.calculation)(arg)
        };
        // Store value
        self.values.insert(arg, result);
        // Return a reference to it
        self.values.get(&arg).unwrap()
    }
}

A simple usage example would be:

fn to_lowercase(value: &str) -> String {
    println!("Calculating \"{}\".to_lowercase()", value);
    value.to_lowercase()
}

fn main() {
    let mut cache = Cache::new(to_lowercase);
    println!("Calculated: {}", cache.value("ABC"));
    println!("Calculated: {}", cache.value("ABC"));
}

Which prints:

Calculating "ABC".to_lowercase()
Calculated: abc
Calculated: abc

However, I feel like my code is much more complicated than it needs to be (specially around the Cache.value implementation), what can be improved here?

The biggest change you can do is to the value function. Because most of what you are building is actually provided via HashMap'sentry API. This could reduce the body of your value function to simply this:

match self.values.entry(arg) {
    Entry::Occupied(val) => val.into_mut(),
    Entry::Vacant(slot) => slot.insert((self.calculation)(arg)),
}

You could also mess with Entry's or_insert_with function, but I personally prefer the approach above.

You may also consider using Clone instead of Copy. The advantage of Clone are that it is more explicit and that it should support more types. The disadvantage obviously is that clone can be arbitrarily expensive. In general I'd say you want Clone for a generic interface such as this.

Finally, I understand it's an exercise, but the names are a bit plain:

Cache is very generic: what do you actually cache? Call it something like FunctionResultCache or Memoizer
value is also a bit weird. What you're doing in essence is wrapping a function, so I'd name this call or invoke or something similar.


Simple: Need help refactoring this awkward looking method I need help refactoring this method. It is way too long.

  def report_total(feed_event, advisor)
    count = 0
    advisor.activity_feed_events.each do |lead|
      if lead == SignupFeedEvent
        count += 1
      else
        if lead.is_a?(feed_event)
          if lead.event_date > (Time.now - 7.days)
            count += 1
          end
        end
      end
    end
    count
  end

You could use scopes to do this at the database level, which would (potentially) be much faster than returning then iterating over all ActivityFeedEvents.

# ActivityFeedEvent
scope :by_type, ->(type){ where(type: type.to_s) }
scope :recent, ->{ where("event_date > ?", Time.now - 7.days) }

# wherever report_total is
def report_total(feed_event_type, advisor)
  advisor.activity_feed_events.by_type(SignupFeedEvent).count +
    advisor.activity_feed_events.recent.by_type(feed_event_type).count
end

Note that you need a lambda for the recent scope despite it not taking args, because otherwise the Time.now will only be evaluated once (when the server starts).

Or, perhaps even more neatly, you could do it the other way round:

# ActivityFeedEvent
scope :for, ->(advisor){ where(advisor_id: advisor.id) }
scope :recent, ->{ where("event_date > ?", Time.now - 7.days) }

# wherever report_total is
def report_total(feed_event_type, advisor)
  SignupFeedEvent.for(advisor).count + feed_event_type.for(advisor).count
end

Depending on your models you may need a more descriptive name than for for this one, belonging_to or for_advisor are candidates.

Also note that some people prefer to write scopes as class methods instead of using lambdas

# ActivityFeedEvent
def self.for(advisor)
  where(advisor_id: advisor.id)
end

def self.recent
  where("event_date > ?", Time.now - 7.days)
end


Terser way of generating deeply nested hash I need to generate a Ruby hash that looks like this:

{ "children" => [
  { "children" => [
    { "children" => [
      { "children" => [] }
    ]}
  ]}
]}

... for an arbitrary level of nesting.  So far the best I've come up with is:

def nested_hash(levels)
  return {} if levels < 1
  root = { 'children' => [] }
  children = root['children']
  (levels - 1).times { children = (children << { 'children' => [] }).first['children'] }
  root
end

This doesn't seem particularly terse or elegant.  Can anyone offer suggestions on making this more terse or elegant?

Recursion perhaps?

def nested_hash(levels)
  return if levels <= 0
  { "children" => [ nested_hash(levels - 1) ].compact }
end

Or it could be

def nested_hash(levels)
  return nil if levels <= 0
  array = nested_hash(levels - 1)
  { "children" => array.nil? ? [] : [array] }
end

if you prefer handling the nil upfront, instead of removing it with compact

In either case, nested_hash(3) will get you

{"children"=>[
  {"children"=>[
    {"children"=>[]}
  ]}
]}

Note that unlike yours, these ones will return nil when levels is zero or less. So you'll want to do the {} fallback elsewhere, e.g. hsh = nested_hash(x) || {}.


Shuffle array without duplicates I made a function about random backgrounds

And those images will unique as possible. (Shuffle again after all gone)

My steps is :

Setup the array
Shuffle it
Do function (check if isset() so run on)

PHP :

$rand_image_ids = [1048,1049,1061,1064,983,972,964];
shuffle($rand_image_ids);
$rand_images_i = 0;

function rand_image($width,$height){
    global $rand_image_ids,$rand_images_i;
    if( !isset($rand_image_ids[$rand_images_i]) ) {
        shuffle($rand_image_ids);
        $rand_images_i = 0;
    }
    $id = $rand_image_ids[$rand_images_i];
    $rand_images_i++;
    return 'https://unsplash.it/'.$width.'/'.$height.'?image='.$id;
}

So I am looking for :

Bugs ?
Any better way to do ?
Shorter codes if possible

Possible with PHP 5.6

There is one bug, in that the function doesn't quite do what you want it to. It does not loop though the randomized array, persisting the order until you've reached the end. As you seem to want from your description. What it does do is to randomize the array once, then once more if there is no array index 0 (which is only true if the array is empty). Then it retrieves the first image ID from the array, increases the counter, generates the URL. Before it finally promptly forgets everything done inside the function, except the finalized URL.

If you want true persistence on the order of images, then you need to store it somewhere. Either in the session, or in a database.

As for the last two questions, I've refactored your code to be more in line with recommended practices:

function randomImageLink ($width, $height, $images) {
    // Using mt_rand () instead of array_rand () due to randomization issues.
    $loc = mt_rand (0, count ($images) - 1);

    // Get a random image ID from the array.
    $id = $images[$loc];

    return 'https://unsplash.it/' . $width . '/' . $height . '?image=' . $id;
}

$imageList = [1048, 1049, 1061, 1064, 983, 972, 964];
$imageLink = randomImageLink (800, 600, $imageList);

Note that this is based upon the function you wrote, not the function you wanted. It should, however, work as an example on how you can restructure your code in general.

PS: I've assumed that the list of image IDs come from a external source, such as a "glob()" on a folder or a DB query.


Extending basic differentiator to handle more kinds of expressions Exercise 2.56.

  Show how to extend the
  basic differentiator to handle more
  kinds of expressions. For instance,
  implement the differentiation rule

  $$ \frac{d(u^n)}{dr} = nu^{n-1}(\frac{du}{dr})$$

  by adding a new clause to the deriv
  program and defining appropriate
  procedures exponentiation?, base,
  exponent, and make-exponentiation.
  (You may use the symbol ** to denote
  exponentiation.) Build in the rules
  that anything raised to the power 0 is
  1 and anything raised to the power 1
  is the thing itself.

From the book:

(define (deriv exp var)
  (cond ((number? exp) 0)
        ((variable? exp)
         (if (same-variable? exp var) 1 0))
        ((sum? exp)
         (make-sum (deriv (addend exp) var)
                   (deriv (augend exp) var)))
        ((product? exp)
         (make-sum
           (make-product (multiplier exp)
                         (deriv (multiplicand exp) var))
           (make-product (deriv (multiplier exp) var)
                         (multiplicand exp))))

I added:

        ((exponentiation? exp)
         (make-product (exponent exp)
                       (make-exponentiation (base exp) (- (exponent exp) 1))))

Book:

        (else
         (error "unknown expression type -- DERIV" exp))))

(define (variable? x) (symbol? x))

(define (same-variable? v1 v2)
  (and (variable? v1) (variable? v2) (eq? v1 v2)))

(define (make-sum a1 a2)
  (cond ((=number? a1 0) a2)
        ((=number? a2 0) a1)
        ((and (number? a1) (number? a2)) (+ a1 a2))
        (else (list '+ a1 a2))))

(define (=number? exp num)
  (and (number? exp) (= exp num)))

(define (make-product m1 m2)
  (cond ((or (=number? m1 0) (=number? m2 0)) 0)
        ((=number? m1 1) m2)
        ((=number? m2 1) m1)
        ((and (number? m1) (number? m2)) (* m1 m2))
        (else (list '* m1 m2))))

(define (sum? x)
  (and (pair? x) (eq? (car x) '+)))

(define (addend s) (cadr s))

(define (augend s) (caddr s))

(define (product? x)
  (and (pair? x) (eq? (car x) '*)))

(define (multiplier p) (cadr p))

(define (multiplicand p) (caddr p))

I added:

(define (exponentiation? x)
  (and (pair? x) (eq? (car x) '**)))
(define (base s) (cadr s))
(define (exponent s) (caddr s))
(define (make-exponentiation m1 m2)
  (cond ((=number? m2 0) 1)
        ((= m2 1) m1)
        (else (list '** m1 m2))))

Can this be improved?

Your definitions are correct except for one minor detail.  The cond clause:

((= m2 1) m1)

should be:

((=number? m2 1) m1)

since one needs to test first whether m2 is a number.

Stylistically, I would prefer to define base and exponent as:

(define base cadr)
(define exponent caddr)

although there is certainly nothing wrong with your definitions.


Given preorder array construct a BST Ok, code reviewers, I want you to pick my code apart and give me some feedback on how I could make it better or more simple. Also its really hard to make this code work if preorder array contains duplicate. Any code/psuedocode is useful.

public class PreOrderTraversalBST {

    private TreeNode root;

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

        TreeNode (TreeNode left, int item, TreeNode right) {
            this.left = left;
            this.item = item;
            this.right = right;
        }
    }

    /**
     * QQ: is there any alternative for this ?
     */
    private static class Counter {
        int counter;

        Counter(int counter) {
            this.counter = counter;
        }

    }

    public void preOrderRecursive (int[] a) {
        root = dpPreOrderRecursive (a, new Counter(0), Integer.MIN_VALUE, Integer.MAX_VALUE );
    }

    private TreeNode dpPreOrderRecursive (int[] arr, Counter counter, int min, int max) {

        if (counter.counter < arr.length && arr[counter.counter] > min && arr[counter.counter] < max) {

            int item = arr[counter.counter];

            TreeNode treeNode = new TreeNode(null, item, null);

            counter.counter++;
            treeNode.left = dpPreOrderRecursive(arr, counter, min, item);
            treeNode.right = dpPreOrderRecursive(arr, counter, item, max);

            return treeNode;
        }
        return null;
    }

    public void preOrderWithStack (int[] arr) {

       final Stack<TreeNode> stack = new Stack<TreeNode>();
       root = new TreeNode(null, arr[0], null);
       stack.push(root);
       int i = 1;

       while (arr[i] < root.item) {
           TreeNode node = new TreeNode(null, arr[i], null);
           if (arr[i] < stack.peek().item) {
               stack.peek().left = node;
           } else {
               TreeNode temp = null;
               while (!stack.isEmpty() && arr[i] >= stack.peek().item) {
                   temp = stack.pop();
               }
               temp.right = node;
               stack.push(temp);
           }
           stack.push(node);
           i++;
       }

       TreeNode rightNode = new TreeNode(null, arr[i], null);
       root.right = rightNode;
       stack.push(rightNode);
       i++;

       while (i < arr.length) {
           TreeNode node = new TreeNode(null, arr[i], null);
           if (arr[i] >= stack.peek().item) {
               stack.peek().right = node;
               stack.push(node);
           } else {
               TreeNode temp = null;
               while (!stack.isEmpty() && arr[i] < stack.peek().item) {
                   temp = stack.pop();
               }
               temp.left = node;
               stack.push(temp);
           }
           stack.push(node);
           i++;
          }
        }
}

Complexity check. Does stack based solution account to O(nlogn) time comlexity or O(n2) ?

Your binary tree setup does not appear to be well balanced.... and no, the counter is unnecessary. I think you have perhaps over-thought the process...

If the data is pre-sorted, you can simply bifurcate your data in a recursive process.

Your method is very close to what I would do, so copy/paste:

private TreeNode dpPreOrderRecursive (int[] arr, /*Counter counter,*/ int min, int max) {

    int mid = (min + max) >> 1;
    TreeNode treeNode = new TreeNode(null, arr[mid], null);
    treeNode.left = mid > min ? dpPreOrderRecursive(arr, min, mid - 1) : null;
    treeNode.right = mid + 1 < max ? dpPreOrderRecursive(arr, mid + 1, max) : null;
}

Then, you can get your tree with:

TreeNode root = dpPreOrderRecursive(arr, 0, arr.length);

(none of the Integer.MIN/MAX).


Open source Pastebin tool made in JSP and servlet I'm a student and still studying. I just made my own Pastebin tool to generate source code. I've also contributed this project as an open source. You could see this project from here.

If you wish to see the demo, you could visit here.

Now after this, I've made this project using JSP and servlet. Actually, when the user presses "Generate source codes", the URL looks something like this:

  http..//viewCode.jsp?id=10428

Please contribute your opinions here. Here is my source code to generate the URL like this:

<%@ page language="java" contentType="text/html; charset=ISO-8859-1"
    pageEncoding="ISO-8859-1"%>

<%@ page language="java" import="java.sql.*" %>
<%@ page language="java" import="java.util.*" %>
<%@ page language="java" import="org.apache.commons.lang3.StringEscapeUtils" %>
<%@ page language="java" import="org.apache.*" %>

<%

    String submit_code1=request.getParameter("paste_code");
    String title_code=request.getParameter("title_code");

    Random random = new Random(System.currentTimeMillis());
    int gen_key=10000 + random.nextInt(20000);

    String submit_code2 = StringEscapeUtils.escapeHtml4(submit_code1);

    try{
    Class.forName("com.mysql.jdbc.Driver");
    Connection con=DriverManager.getConnection("jdbc:mysql://localhost:3306/db_name", "root", "1234");
    PreparedStatement pstmt=con.prepareStatement("insert into paste_tool(gen_key, title, source_codes) VALUES(?, ?, ?)");

    pstmt.setInt(1, gen_key);
    pstmt.setString(2, title_code);
    pstmt.setString(3, submit_code2);

    pstmt.executeUpdate();

    pstmt.close();

    response.sendRedirect("viewCode.jsp?id="+gen_key);

}
catch(Exception e){
    e.printStackTrace();
}
%>

On the upside of your code, you kept it as simple as possible.
But there are a few flaws - and the upside doesn't come without consequences.

What's also good is that you already use PreparedStatement.

Security Risk

Hard-coding the username and password is not a good idea for another reason.
A misconfigured webserver could easily expose these critical credentials by accident.

Layered Architecture

We say that often, an architecture consists of 3+1 layers: UI, Business Logic / App, Persistence, plus Main.

In your case this is all mixed up.
The upside is that in your case it keeps things seemingly simple.
But this is a trap.

Someone needs to change the source code of the application in order to connect with a different password, different username, or to use a different SQL RDBMS than MySQL, or to use a different type of DBMS altogether.

ID Collisions likely

The way how you get your ID means that ID collisions are quite likely.
There are better ways to generate a unique ID.
In case of MySQL, and most other RDBMS for that matter, you can actually let the DBMS generate an ID for you.
In MySQL, the id column would need to be defined like this: id INT PRIMARY KEY AUTO_INCREMENT.

Usually, we say that using auto-incremental IDs is a security issue.
However, if pastes are public anyway, that's a different topic.
The pastes are not access-protected anyway.
So incremental IDs are fine in this case.

Otherwise, universally unique ids or hashes with unlikely collisions are far better user ids. One might still want to go for id INT PRIMARY KEY AUTO_INCREMENT hidden in the DBMS for relations and use a uuid VARCHAR(...) UNIQUE for the id visible outside the DBMS.

Error handling

You want to close your pstmt even in case of error, and you want to close your con in any case.

You can use Java 7 try-with-resources for that:

try (final Connection con = DriverManager.getConnection(dbUrl, dbUsername, dbPassword)) {
    try (final PreparedStatement pstmt = con.prepareStatement("INSERT INTO paste_tool(gen_key, title, source_codes) VALUES(?, ?, ?)")) {
        // ...
    }
}

This will close your PreparedStatement and your Connection automatically, regardless of whether they were successful or threw exceptions.

Code Conventions

In Java, we only use _ in names in these rare cases:

The name implements an API defined elsewhere and there the name already contains _.
The name is a compile time constant, as in public static final short MAX = 32767;
The _ replaces a comma in natural language, as in givenConnectedChatClient_whenSendingMessage_thenReceivesMessage().

In your case, you should rename things:

gen_key -> genKey
submit_code1 -> submitCode1
title_code -> titleCode

Why is it named submit_code1 anyway? Is there also non-submitted code? Is there also a submit_code0? A submit_code2? And why is it named submit_code1, when in the form it is named paste_code?

I think it that submitCode1 should actually be named pasteCode.
And submitCode2 should be renamed to htmlEscapedPasteCode - or removed, see next point.

When to replace the special characters

You replace the special characters in the paste code before storing the paste in the DBMS.
While this gives you a little bit of performance - you have to replace only once, upon storage, not many times upon each retrieval, I think it's not the best way how to handle it.

Consider you want to extend this with different interfaces.
Maybe you want to implement raw retrieval, or PDF export, or the facility that a user can fetch all his pastes as one big zip file?
Then you'd have to reverse the replacement of the special characters.

From an architecture perspective, the special characters are a presentation topic - it's because you use HTML as presentation.
Therefore it should be in the presentation layer, not in the persistence layer.
If you then add more presentation modules, they will not have to reverse presentations of other modules that are otherwise unrelated to them.
It would look awkward in a raw module or PDF module to first have to reverse the HTML special characters from what they get from the database.

So, I'd put the StringEscapeUtils.escapeHtml4() call in viewCode.jsp instead.

Error Handling 2

What happens in case of an ID collision (gen_key already used)?

Error Handling 3

What happens in case the form is submitted empty?

ISO-8859-1 vs. UTF-8

I'd really use UTF-8. Always. Everywhere. Unless there's a really good reason not to. And I don't think that here there is.

ASP-style JSPs vs. XML-style JSPs

I'd rather go for XML-style JSPs than ASP-style JSPs.
For many it may just be a matter of taste.
But to me there's the advantage that I can XSLT process the source of the JSP.


OpenVPN Authentication Script I wrote a shell script to authenticate users connecting to an OpenVPN server. When a user attempts to connect, OpenVPN executes the script with the username and password variables sends.
The script then sends an HTTP request to a web server that either returns a status code of 202 upon success and 401 on failure.

I would like to know if the following script is secure when it comes to remote code execution and url-encoded arguments injection.

#!/bin/bash

# Change the domain to your SUPINBOT installation.
URL=http://localhost:8080/vpn/api/checkAccount
# Change to a secure random string.
API_KEY=PRIVATE_KEY

rawurlencode() {
    local string="${1}"
    local strlen=${#string}
    local encoded=""
    local pos c o

    for (( pos=0 ; pos<strlen ; pos++ )); do
        c=${string:$pos:1}
        case "$c" in
            [-_.~a-zA-Z0-9] ) o="${c}" ;;
            * )               printf -v o '%%%02x' "'$c"
        esac
        encoded+="${o}"
    done
    ENCODED=$encoded
}

rawurlencode $API_KEY;
encodedApiKey=$ENCODED;

rawurlencode $username;
encodedUsername=$ENCODED;

rawurlencode $password;
encodedPassword=$ENCODED;

responseCode=$(curl --write-out %{http_code} --silent --output /dev/null -d "apiKey=$encodedApiKey&username=$encodedUsername&password=$encodedPassword" $URL)

if [ "$responseCode" -eq "202" ]; then
    exit 0;
else
    exit 1;
fi;

The URL encoding looks correct. However, that doesn't mean that users cannot enter something malicious that will trigger some bug in the server. But that's up to the server implementation to be robust to malicious inputs. Also note that anybody with access to this script can read your API key and make arbitrary API calls.

Many lines end with ;. This is pointless, you should remove all those ;.

The exit code of a Bash script is the exit code of the last command.
So you could replace the last if-else condition with this simple line, and get the exact same effect:

[ "$responseCode" -eq "202" ]


Longest substring with unique characters Find the length of the longest substring without repeating characters

Any comments on my code?

public class LongestSubstring {
    /**
     * First version. O(n^2) runtime
     * For each character, find the longest substring starting
     * with this character and update max
     * @param s
     * @return
     */
    public int lengthOfLongestSubstringV1(String s) {
        char[] c = s.toCharArray();
        int n = c.length;
        int max = 0;

        for (int i = 0; i < n; i++){
            // localMax is the size of the longest substring starting with character i
            // and that has no repeated character
            int localMax = findLocalMax(c,i);
            // update max if localMax > max
            max = (max > localMax) ? max : localMax;
        }
        return max;
    }

    /**
     * find the largest substring that has no repeated character
     * starting at character i in array c
     * @param c
     * @param i
     * @return
     */
    public int findLocalMax(char[] c, int i){
        int n = c.length;
        // seen: characters already seen
        HashSet<Character> seen = new HashSet<Character>();
        int localMax = 0;

        for (int j = i; j < n; j++){
            // c[j] was seen already
            if (seen.contains(c[j])){
                return localMax;
            }
            else{
                seen.add(c[j]);
                localMax++;
            }
        }
        return localMax;
    }

    /**
     * Second version. O(n^2) runtime
     * if a character is seen again,
     * @param s
     * @return
     */
    public int lengthOfLongestSubstringV2(String s) {
        char[] c = s.toCharArray();
        int n = c.length;
        // currentLetters: HashSet of the letters contained in the current substring
        HashSet<Character> currentLetters = new HashSet<Character>();
        // pointer1 points to the beginning of the substring
        int pointer1 = 0;
        int max = 0;
        int currentCount = 0;

        // pointer2 points to the end of the substring
        for (int pointer2 = 0; pointer2 < n; pointer2++){
            if (currentLetters.contains(c[pointer2])){
                // if the letter c[pointer2] has already been seen
                // remove all the letters before the first occurrence
                // of this letter to consider the substring beginning after
                // this first occurrence
                while (c[pointer1] != c[pointer2]){
                    currentCount--;
                    currentLetters.remove(c[pointer1]);
                    pointer1++;
                }
            }
            else{
                // otherwise, add the letter to the substring
                currentCount++;
                currentLetters.add(c[pointer2]);
            }
            max = (max > currentCount) ? max : currentCount;
        }

        return max;
    }
}

public class TestLongestSubstring {
    LongestSubstring lgs = new LongestSubstring();
    @Test
    public void testEmptyString(){
        String s = "";
        assertEquals(0,lgs.lengthOfLongestSubstringV1(s));
        assertEquals(0,lgs.lengthOfLongestSubstringV2(s));
    }

    @Test
    public void testSameCharacterExpects1(){
        String s = "bbbb";
        assertEquals(1,lgs.lengthOfLongestSubstringV1(s));
        assertEquals(1,lgs.lengthOfLongestSubstringV2(s));
    }

    @Test
    public void testabcabcbbExpects3(){
        String s = "abcabcbb";
        assertEquals(3,lgs.lengthOfLongestSubstringV1(s));
        assertEquals(3,lgs.lengthOfLongestSubstringV2(s));
    }
}

public class TestLongestSubstringRunner {
    public static void main(String args[]){
        Result result = JUnitCore.runClasses(TestLongestSubstring.class);
        for (Failure failure : result.getFailures()){
            System.out.println(failure.toString());
        }
        System.out.println(result.wasSuccessful());
    }
}

Time complexity

The time complexity is not \$O(N^2)\$ as you estimated, but actually \$O(N M)\$, where n is the length of the input string and m is the number of unique characters in the input string. This makes a big difference, because in practice m is typically bounded by a constant, the size of the alphabet, which in the case of ascii is 256. If m is a constant then the asymptotic complexity becomes simply \$O(N)\$, which is great news for you.

Optimizations

A small tweak can make the implementation significantly faster: change the loop condition in the outer function to go until n - max instead of n.

If you know the size of the alphabet in advance, then you can use a boolean[] instead of a set, with the character codes as indexes. It's simpler, and by reducing the autoboxing, faster too.

Use interface types instead of implementations

You could declare the HashSet as a Set instead.


JavaScript News Ticker The data (news items) comes from a
. I'm using setInterval and setTimeout for the timing aspects, and jQuery animate for the movement to the right. I know that setInterval and setTimeout start acting weirdly when the tab is not active, but that's not a problem for my specific use-case.

I'm looking for any comments, specifically the entire displayItem() function -- is there a better way to do this?

var items = [];
$('.news li').each(function() { //get all items from
 and add to array
  items.push($(this).html());
});

//Ticker
var duration = 5000; //take 5 seconds to show one item
$('.news ul').remove();
$('.news').append('Headlines');

function displayItem(html, i) {
  setTimeout(function() {
    $('.news .item').html(items[i]).animate({
      "left": "+=400px"
    }, {
      duration: duration,
      easing: "linear",
      complete: function() {
        $('.news .item').css('left', '0');
      }
    });
  }, (duration + 75) * i);
}

function loopAndDisplayItems() {
  for (var i = 0; i < items.length; i++) {
    var time = (i + 1) * 1000;
    displayItem(items[i], i);
  }
}

setInterval(loopAndDisplayItems, items.length * duration);
loopAndDisplayItems();
.news {
  border-radius: 7px;
  background-color: lightgray;
  padding: 8px;
  font-size: 12px;
}
.news .title {
  color: red;
}
.news .item {
  margin-left: 5px;
  position: relative;
}

Headline 1

Headline 2

Reminds me of the days where <marquee> was the easiest way to add this kind of animation.

In terms of markup, your markup makes sense. However, I suggest you also move the title inside the markup instead of being in the JS.

With regards to your problem with timers, browsers usually slow down timers when the tab is inactive. This is to conserve battery life. As far as I know, setInterval, setTimeout and requestAnimationFrame are all affected.

Another problem I see is you animate in intervals. You don't let the animation finish before animating the next item. What you should do here is put your animation in a function, and when it finishes, it calls itself recursively from the complete callback. That way, you know it finishes before pulling in the next item.

Now if you don't mind CSS3, you can simply use transition or the more advanced animation property coupled with @keyframes block.

Now here's my take on your problem. The main difference here is I animate the entire list. That way, I only need to deal with 1 item when animating. There is a padding on each item to show a gap between items.

var list = $('.news ul');
var totalWidth = 0;
var items = $('.news li').each(function(){
  totalWidth += $(this).outerWidth();
});

function scrub() {
  // Note that we added a wrapper. That's so the entire list scrubs in,
  // and you don't have to deal with it individually.
  list.animate({
    // We animate the left all the way past negative so text travels from
    // right to left and we read left to right.
    left: -totalWidth
  }, {
    easing: 'linear',
    // We adjust speed here. Here it is 20ms per pixel.
    duration: totalWidth * 20,
    complete: function(){
      // When the animation is done, we just move the element back to its
      // starting position, which is off-screen to the right.
      $(this).css({ left: '100%' });
      // Run scrub again.
      scrub();
    }
  });
}
// Start the scrub
scrub();
.news {
  border-radius: 7px;
  background-color: lightgray;
  padding: 8px;
  font-size: 12px;
  overflow: hidden;
}
.news header {
  float: left;
  color: red;
}
.news .wrapper {
  overflow: hidden;
}
.news ul {
  left: 100%;
  position: relative;
  margin: 0;
  padding: 0;
  list-style: none;
  white-space: nowrap;
}
.news li {
  padding-right: 50%;
  min-width: 100%;
  display: inline-block;
}

  Headlines

Foo!Lorem ipsum dolor sit amet, consectetur adipiscing elit. Vivamus ac ligula nec tortor facilisis pharetra a ut arcu. Ut lacus libero, finibus sit amet risus id, rutrum malesuada odio.

Bar!Aliquam luctus dapibus eros id eleifend. Lorem ipsum dolor sit amet, consectetur adipiscing elit. In ac tellus sem. Integer laoreet sem in ex tincidunt fringilla.


Count the ways to partition an array into two equal sets I need to count the number of ways to partition an array into two contiguous arrays such that those arrays contain exactly same set of items.

Suppose A = [4, 1, 1, 2, 4, 2, 4, 1]. If you pass this array to the function, it needs to return 2. (Those subarrays are  [4, 1, 1, 2] & [4, 2, 4, 1] and [4, 1, 1, 2, 4] & [2, 4, 1].)

Suppose A = [1, 5, 1]. If you pass this array to the function, it needs to return 0.

My code given below. Is there any better way to do this?

def solve(A):
    count = 0
    for i in range(1, len(A)-1/2):

        left = A[:i]
        right = A[i:]

        left[:i].sort()
        right[i:].sort()

        if set(left) == set(right):
            count += 1
    return count

Your current approach is going to be very slow because you're slicing the each time and then that is followed by an unnecessary sort calls and ultimately you call set() on both of the lists.

Slicing a list is very slow, because it requires creating an array of new references(pointers) to the items of the original array. To fix that we can use collections.deque here, ie. start with two deques first. First one is going be a deque(say full_deque) of the list passed to the array and second one will be empty(say current_deque). And now during the loop we simply pop the leftmost item from the full_deque and append it to the current_deque, these are O(1) operations for a deque so we will save a lot of time. And then simply call set() on both deques and compare those sets:

from collections import deque

def solve_deque(seq):
    count = 0
    full_deque = deque(seq)
    current_deque = deque()
    for _ in xrange(len(seq)):
        current_deque.append(full_deque.popleft())
        if set(current_deque) == set(full_deque):
            count += 1
    return count

The above operation is around 7 times faster than your version:

>>> A = [random.randint(0, 100) for _ in xrange(1000)]
>>> %timeit solve(A)
10 loops, best of 3: 74.4 ms per loop
>>> %timeit solve_deque(A)
100 loops, best of 3: 11.3 ms per loop

But for bigger list it is still not good enough as the set() calls in each loop are expensive.

>>> A = [random.randint(0, 100) for _ in xrange(10**5)]
>>> %timeit solve_deque(A)
1 loops, best of 3: 1min 26s per loop

To improve this even further we need to approach this differently, and one such approach is to use a Counter. Similar to deque we maintain two Counters here: 1. full_counter: Contains count of the initial list items 2. current_counter: Contains counts of items seen so far. Apart from these we are also going to maintain two sets here: 1. full_keys: Initial unique items from the list 2. current_keys: Unique items seen so far.

Here during the loop we are going to increment the count of current item in current_counter and decrease its count from the full_counter. But we can't compare these counters, so that's where the 2 sets come in. Firstly add current item to the current_keys set and then check if the count of current item is equal to 0 in full_counter and if it is then we can remove this item from the full_keys set and then simply compare these two sets:

from collections import Counter

def solve_counter(seq):
    count = 0
    full_counter = Counter(seq)
    current_counter = Counter()
    full_keys = set(seq)
    current_keys = set()

    for item in seq:
        current_counter[item] += 1
        full_counter[item] -= 1
        current_keys.add(item)
        if full_counter[item] == 0:
            full_keys.remove(item)
        count += full_keys == current_keys
    return count

Timings:

>>> A = [random.randint(0, 100) for _ in xrange(1000)]
>>> %timeit solve_counter(A)
1000 loops, best of 3: 764 µs per loop
>>> A = [random.randint(0, 100) for _ in xrange(10**5)]
>>> %timeit solve_counter(A)
10 loops, best of 3: 109 ms per loop

Few other points:

The recommended way to get both item and the index is to use enumerate() rather than using range() with indexing. Also in Python 2 range() returns a list, so unless you actually need a list always use xrange().
Use upper case variable names(A) only for module level constants, that applies to the function arguments as well, we can use num_list or seq instead of A here.
left[:i].sort() and right[i:].sort() are useless, slice on Python lists returns a new shallow copy. So, here you created two new lists first, sorted them and then threw them away. To sort left and right, simply do left.sort() and rigth.sort().


Calculate mean, median, and mode in Rust I'm learning Rust using The Rust Programming Language. I'm trying the assignment at the end of chapter 8 — Hash Maps.

The task is:

  Given a list of integers, use a vector and return the mean (average), median (when sorted, the value in the middle position), and mode (the value that occurs most often; a hash map will be helpful here) of the list.

The average value and the median were very easy for me, but I struggle solving the last part of the task. I do have a working solution (see below), but I'm sure there must be some more elegant and readable way (using the standard library?) for this.

use std::collections::HashMap;
fn main() {
  let  mut numbers = vec![42, 1, 36, 34, 76, 378, 43, 1, 43, 54, 2, 3, 43];

  let avg: f32;
  let median: i32;
  let mode: i32;

  { // calculate average
      let mut sum: i32 = 0;
      for x in &numbers {
          sum = sum + x;
      }

      avg = sum as f32 / numbers.len() as f32;
  }

  { // calculate median
      numbers.sort();
      let mid = numbers.len() / 2;

      median = numbers[mid];
  }

  { // calculate mode
      // new HashMap
      let mut times = HashMap::new();

      // count
      for x in &numbers {
          let cnt = times.entry(*x as usize).or_insert(0);
          *cnt += 1;
      }

      let mut best: (i32, i32) = (*times.iter().nth(0).expect("Fatal.").0 as i32, *times.iter().nth(0).expect("Fatal.").1 as i32);

      for x in times.iter() {
          if *x.1 > best.1 {
              best = (*x.0 as i32, *x.1);
          }
      }
      mode = best.0;
  }

  println!("AVERAGE: {}", avg);
  println!("MEDIAN: {}", median);
  println!("MODE: {}", mode)

}

Learn to love rustfmt. For example, the Rust standard is 4-space indents.
Learn to love Clippy, which can show you the more idiomatic way to iterate over a collection:

warning: it is more idiomatic to loop over references to containers instead of using explicit iteration methods
  --> src/main.rs:43:18
   |
43 |         for x in times.iter() {
   |                  ^^^^^^^^^^^^ help: to write this more concisely, try: `&times`
   |
   = note: #[warn(explicit_iter_loop)] on by default
   = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#explicit_iter_loop

There's no reason to declare your variables early in the function. That's the kind of thing you would do in very old C or JavaScript and isn't required in modern languages. Declare them as late as possible.
Extract methods for each of the operations you'd like to perform. This allows you to remove the artificial blocks inside the main function and makes the comments into code identifiers. This removes the need for the variables in the main function at all.
There's no need to use a Vec as you never add or remove items from the list. A plain array will suffice.
As mentioned in the comments, Iterator::sum already exists. If it didn't, you could use something like Iterator::fold.
Your implementation of median is incorrect. When there are an even number of values, the median is defined as the average of the two middle values. I've not changed your code for this because doing so would require changing the type of median and I'm not sure how you'd prefer to do so.
There's no need to cast the value to usize when counting the number of occurrences; storing the i32 in the HashMap makes more sense.
You can dereference the number using &value in the for loop binding.
Once you have built your mapping of occurrences, you can iterate over it, taking the maximum value with Iterator::max_by_key, throw away the count using Iterator::map, then have a single expect call.
Use a better error message than "Fatal" - describe what the problem was.
Don't use nth(0) - that's just .next()

use std::collections::HashMap;

fn average(numbers: &[i32]) -> f32 {
    numbers.iter().sum::<i32>() as f32 / numbers.len() as f32
}

fn median(numbers: &mut [i32]) -> i32 {
    numbers.sort();
    let mid = numbers.len() / 2;
    numbers[mid]
}

fn mode(numbers: &[i32]) -> i32 {
    let mut occurrences = HashMap::new();

    for &value in numbers {
        *occurrences.entry(value).or_insert(0) += 1;
    }

    occurrences
        .into_iter()
        .max_by_key(|&(_, count)| count)
        .map(|(val, _)| val)
        .expect("Cannot compute the mode of zero numbers")
}

fn main() {
    let mut numbers = [42, 1, 36, 34, 76, 378, 43, 1, 43, 54, 2, 3, 43];

    println!("AVERAGE: {}", average(&numbers));
    println!("MEDIAN: {}", median(&mut numbers));
    println!("MODE: {}", mode(&numbers));
}

A clever one-pass solution for mode is also possible:

fn mode(numbers: &[i32]) -> Option<i32> {
    let mut counts = HashMap::new();

    numbers.iter().copied().max_by_key(|&n| {
        let count = counts.entry(n).or_insert(0);
        *count += 1;
        *count
    })
}


Groovy script to get average price per group from different data arrays I've been trying to learn Groovy lately and I tried to solve a problem which involves designing a routine that will calculate the average Product price per Group.

I have the following data:

// contains information about [Product, Group, Cost]
products = [
    ["A", "G1", 20.1],
    ["B", "G2", 98.4],
    ["C", "G1", 49.7],
    ["D", "G3", 35.8],
    ["E", "G3", 105.5],
    ["F", "G1", 55.2],
    ["G", "G1", 12.7],
    ["H", "G3", 88.6],
    ["I", "G1", 5.2],
    ["J", "G2", 72.4]
]

// contains information about Category classification based on product Cost
// [Category, Cost range from (inclusive), Cost range to (exclusive)]
// i.e. if a Product has Cost between 0 and 25, it belongs to category C1
category = [
    ["C3", 50, 75],
    ["C4", 75, 100],
    ["C2", 25, 50],
    ["C5", 100, null],
    ["C1", 0, 25]
]

// contains information about margins for each product Category
// [Category, Margin (either percentage or absolute value)]
margins = [
    "C1" : "20%",
    "C2" : "30%",
    "C3" : "0.4",
    "C4" : "50%",
    "C5" : "0.6"]

What I tried so far is the following (I mixed some Java here):

def calculateResult() {
    def group1 = []
    def group2 = []
    def group3 = []
    def pricePerGroupMap = [:]

category.each { cat ->
    String catDesc = cat[0]
    BigDecimal min = cat[1]
    BigDecimal max = cat[2]

    if (max == null) {
        max = 9999.9;
    }

    products.each { product ->
        BigDecimal currProductPrice = BigDecimal.valueOf(product[2])
        if (currProductPrice.compareTo(min) >= 0 && currProductPrice.compareTo(max) <= 0) {
            String selectedMarginAsString = margins.getAt(catDesc);
            BigDecimal selMargin = 0.0;

            if (selectedMarginAsString.endsWith("%")) {
                selMargin = new BigDecimal(selectedMarginAsString.trim().replace("%", "")).divide(BigDecimal.valueOf(100));
            } else {
                selMargin = new BigDecimal(selectedMarginAsString);
            }

            if (product[1].equals("G1")) {
                group1.add(product[2] * (1 + selMargin))
            } else if (product[1].equals("G2")) {
                group2.add(product[2] * (1 + selMargin))
            } else if (product[1].equals("G3")) {
                group3.add(product[2] * (1 + selMargin))
            }
        }

    }
}
pricePerGroupMap["G1"] = group1.sum() / group1.size()
pricePerGroupMap["G2"] = group2.sum()/ group2.size()
pricePerGroupMap["G3"] = group3.sum()/ group3.size()
print pricePerGroupMap
}

I am sure there are better ways to optimize this algorithm taking advantage of closures.

After being investigating a little bit more about usage of closures and some other facilities that Groovy provides, I managed to reduce a little bit the algorithm, after looking for benchmarking posts, I believe this is probably not the most optimal solution, but it looks a lot better than the first proposal (the one in my question).

One of the things I considered to improve the solution is to use Groovy "maps" instead of multiple arrays to hold the data.

I also considered using one additional variable to hold the value of a group being evaluated.

And finally, I decided to avoid using more conditionals to add the results to the arrays and use a more simple instruction taking advantage of the map definition.

MAX_DEFAULT_VALUE_WHEN_NULL = 99999.9

def calculateResult() {
    def pricesPerGroupMap = [G1:[], G2:[], G3:[]]
    def avgPricePerGroupMap = [:]

    category.each { cat ->
        String categoryId = cat[0]
        BigDecimal min = cat[1]
        BigDecimal max = cat[2]

        if (max == null) {
            max = MAX_DEFAULT_VALUE_WHEN_NULL
        }

        products.each { product ->
            BigDecimal currProductPrice = BigDecimal.valueOf(product[2])
            String group = product[1]
            if (currProductPrice.compareTo(min) >= 0 && currProductPrice.compareTo(max) == -1) {
                String selectedMarginAsString = margins.getAt(categoryId);
                BigDecimal selMargin = 0.0;

                if (selectedMarginAsString.endsWith("%")) {
                    selMargin = new BigDecimal(selectedMarginAsString.trim().replace("%", "")).divide(BigDecimal.valueOf(100))
                } else {
                    selMargin = new BigDecimal(selectedMarginAsString)
                }
                pricesPerGroupMap[group].add(currProductPrice * (1 + selMargin))
            }
        }
    }
    pricesPerGroupMap.each { k, v ->
        avgPricePerGroupMap.put(k, new BigDecimal(v.sum()).divide(v.size(), 1, RoundingMode.UP))
    }
    return avgPricePerGroupMap
}


CRUD implementation using Laravel 4 I just started using Laravel 4. I read some tutorials on how to implement CRUD in Laravel, and I was able to develop a portion of it. What I am going to show you is more on Controller and Eloquent ORM.

I have here 2 models "Group" which can only have one Country, but Country can be assigned to different Group ("one to many").

Group Model

class Group extends Eloquent
{
    protected $table = 'group';
    protected $guarded = array('group_id');
    protected $primaryKey = 'group_id';

    public function country()
    {
        return $this->belongsTo('country', 'country_id');
    }
}

Country Model

class Country extends Eloquent
{
    protected $table = 'country';
    protected $primaryKey = 'country_id';

    public function group() {
        return $this->hasMany('group', 'country_id');
    }
}

In my controller, I have index() function which accepts user search filter and pagination request.

Group Controller

public function index()
{
    // search variables here
    $s_name = Input::get("s_name");
    $s_status = Input::get("s_status");

    // order variables here
    $sortBy = Input::get("sort_by");
    $orderBy = Input::get("order_by");

    // get all the groups
    $groups = Group::with('country')
        ->join('country', 'country.country_id', '=', 'group.country_id');

    // search conditions
    $groups = $groups->whereIn('group_status', array(ACTIVE, INACTIVE));
    if (!empty($s_name)) {
        $groups = $groups->where('group_name', 'LIKE', "%$s_name%");
    }

    if (!empty($s_status)) {
        $groups = $groups->where('group_status', 'LIKE', "%$s_status%");
    }

    // order
    if (!empty($sortBy)) {
        $orderBy = (empty($orderBy)) ? ASCENDING : $orderBy;
        if ($sortBy == "o_name") {
            $groups = $groups->orderBy('group_name', $orderBy);

        } else if ($sortBy == "o_country") {
            $groups = $groups->orderBy('country_short_name', $orderBy);

        } else if ($sortBy == "o_status") {
            $groups = $groups->orderBy('group_status', $orderBy);

        }
    } else {
            $groups = $groups->orderBy('group.created_at', DESCENDING);

    }

    $groups = $groups->paginate(PAGINATION_LIMIT);
}

Typically the primary key on the Models is the id in the database table. You have these set as group_id and country_id. This is possible, but confusing. You usually only reference these when making relationships and Laravel will do this automatically if your keys follow a convention. Here is an example:

Group Model

class Group extends Eloquent {
    protected $table = 'group'; // this is assumed by the model name
    protected $guarded = array('id'); // this is usually id
    protected $primaryKey = 'id'; // this usually isn't necessary

    public function country()
    {
        return $this->belongsTo('country'); // this is referencing `country_id` on the groups table and associating it with the Country model.
    }

    // this pertains to part of the answer about the controller
    public function scopeStatus($query, $status)
    {
        return $query->where('group_status', 'LIKE', $status);
    }

    public function scopeName($query, $name)
    {
        return $query->where('group_name', 'LIKE', $name);
    }
}

Country Model

class Country extends Eloquent
{
    protected $table = 'country';
    protected $primaryKey = 'country_id'; // not necessary, should be `id`

    public function groups() {
        return $this->hasMany('group'); // you should have a `group_id` column on the `country` table to associate with, this is automatically inferred by Laravel
    }
}

Group Controller

There are many pieces of your Controller which should be part of your Model. These custom queries can be reused and make your application more flexible. You can use query scopes to add to your models.

I will do my best to show you an example. I will be updating your model as well so you can see what I am moving into the model.

public function index()
{
    // search variables here
    $s_name   = Input::get("s_name");
    $s_status = Input::get("s_status");

    // order variables here
    $sortBy  = Input::get("sort_by");
    $orderBy = Input::get("order_by");

    // if you are going to filter all groups anyways
    // there is not a reason to get all of them, just
    // query the ones you need! :)
    if (!empty($s_name)) {
        $groups = Group::name($s_name); // this is a query scope defined in your model
    }

    if (!empty($s_status)) {
        $groups = Group::status($s_status); // this is a query scope defined in your model
    }

    //.. other logic here
}

I tried to understand your other sorting logic as well but couldn't comprehend all of it without seeing the data. This sorting could be added as a filter in your model or even to your query scopes. Then all of this logic would be removed from your Controller and could be re-used throughout your application.

Hopefully this helps!

Here are some other resources on query scopes. I find many people miss the power of these:

http://www.develophp.org/2013/05/laravel-4-model-query-scopes/
http://ifonlyiknewthat.com/laravel/laravel-4-orm-query-scope/


Setting enum flags by string (in any order) I want to set flags which are given in string representation in any order. And it is intimidating, because you have to check like 2^3 possible options in my occassion. I was just wondering is there any better way to do that than just simple brute-force. Any optimization and readability improvment would be great. This is my code:

[Flags]
public enum salaryFeatures
{
Children = 1,
Graduate = 2,
Disability = 4
}

class Program
{

public static void Main()
{
    string Employee = "10023 Mark Male 6.7 70 30 20 Children Graduate";
    salaryFeatures f;
    f = GetFeaturesByString(Employee);
}

public static salaryFeatures GetFeaturesByString(string fields)
{
    bool children = false;
    bool graduate = false;
    bool disability = false;
    salaryFeatures features = new salaryFeatures();

    if (fields.ToLower().Contains("children")) children = true;
    if (fields.ToLower().Contains("graduate")) graduate = true;
    if (fields.ToLower().Contains("disability")) disability = true;

    if (children && graduate && disability)
    {
        return features = salaryFeatures.Children | salaryFeatures.Disability | salaryFeatures.Graduate;
    }
    if (children && graduate)
    {
        return features = salaryFeatures.Children | salaryFeatures.Graduate;
    }
    if (children && disability)
    {
        return features = salaryFeatures.Children | salaryFeatures.Disability;
    }
    if (children)
    {
        return features = salaryFeatures.Children;
    }
    if (graduate && disability)
    {
        return features = salaryFeatures.Graduate | salaryFeatures.Disability;
    }
    if (graduate)
    {
        return features = salaryFeatures.Graduate;
    }
    if (disability)
    {
        return features = salaryFeatures.Disability;
    }
    return features;
}
}

Here is my take on your code

[Flags]
public enum SalaryFeatures
{
  None= 0,
  Children = 1,
  Graduate = 2,
  Disability = 4
}

class Program
{

  public static void Main()
  {
    string Employee = "10023 Mark Male 6.7 70 30 20 Children Graduate";
    SalaryFeatures f;
    f = GetFeaturesByString(Employee);
    // f.Dump(); //LinqPad only
  }

  public static SalaryFeatures GetFeaturesByString(string fields)
  {
    SalaryFeatures features = SalaryFeatures.None;

    if (fields.IndexOf("children", StringComparison.OrdinalIgnoreCase) >= 0) features |= SalaryFeatures.Children;
    if (fields.IndexOf("graduate", StringComparison.OrdinalIgnoreCase) >= 0) features |= SalaryFeatures.Graduate;
    if (fields.IndexOf("disability", StringComparison.OrdinalIgnoreCase) >= 0) features |= SalaryFeatures.Disability;

    return features;
  }
}

First of the definition of your Enumeration is not complete. You need to define a None value for it

  Use None as the name of the flag enumerated constant whose value is
  zero. You cannot use the None enumerated constant in a bitwise AND
  operation to test for a flag because the result is always zero.
  However, you can perform a logical, not a bitwise, comparison between
  the numeric value and the None enumerated constant to determine
  whether any bits in the numeric value are set.

See also the Guidelines when defining a Flags enum. I also change the naming of the enum to PascalCase (SalaryFeatures).

After that I have improved your method GetFeaturesByString to use bitwise OR to combine the values depending on the values that are found in your string. I also use the method IndexOf(string, StringComparison) to check if the search string exists in the source string, instead of using ToLower() and Contains().


Optimize setting parameters from query string I am currently setting over a hundred global variables by performing individual checks against the URL string.

var Query_MenuTiles = getParameterByName("menutiles")
var Query_Link = getParameterByName("link")
var Query_Speed = getParameterByName("speed")

Unless it would slow things down, I assume that setting keys and properties of an object would be a cleaner way of doing this.

The getParameterByName function is as follows:

function getParameterByName(name, source ) {
    if (typeof source == 'undefined') {
        source = window.location.search
    }
    if ( user_fastquery == '1' ) {
        return search[name] || ''
    }
    else {
      name = name.toLowerCase()
      name = name.replace(/[\[]/, "\\\[").replace(/[\]]/, "\\\]");
      var regexS = "[\\?&]" + name + "=([^&#]*)"; //"=([^&#]*)";
      var regex = new RegExp(regexS);
      var query_x = 'back'
      var query_y = 'frame'
      if ( name != query_x ) {
        if ( source.indexOf("&" + query_x + "=") >= 0 ) {
            var array_source = source.split('&' + query_x + '=')
            source = array_source[0]
            var source_tail = array_source[1]
        }
        else if ( source.indexOf("?" + query_x + "=") >= 0 ) {
            var array_source = source.split("?" + query_x + "=" )
            source = array_source[0]
        }
      }
      var results = regex.exec( source );
      if(results == null)
        return "";
      else
        return decodeURIComponent(results[1].replace(/\+/g, " "));
    }
}

You are correct that it would be much faster to parse all the parameters once into an object and then just access them from there rather than refind every single parameter from scratch each time:

function getParameters(sourceURL) {
    var pairs, parts, i, output = {};
    sourceURL = sourceURL || window.location.search;
    // strip off leading ?
    if (sourceURL.charAt(0) === "?") {
        sourceURL = sourceURL.slice(1);
    }
    // split into individual pairs
    pairs = sourceURL.split("&");
    for (i = 0; i < pairs.length; i++) {
        // split each pair
        parts = pairs[i].split("=");
        if (parts.length < 2) {
            parts.push("");
        }
        // make key always be lowercase and store key/value pair
        output[decodeURIComponent(parts[0]).toLowerCase()] = decodeURIComponent(parts[1]);
    }
    return output;
}

var params = getParameters();

Then, you can fetch an individual parameter anytime from the params object.  In fact, you won't even need to store them all in globals since there already stored in this one variable.

var Query_MenuTiles = params["menutiles"];

Working demo: http://jsfiddle.net/jfriend00/pj5ocazd/


World resource system for Adventure Game/RPG I'm beginning the long process of developing an Adventure Game/RPG and I was hoping to get some review on a resource system I'm coming up with.  How it works is, a resource is generic to every resource in the world.  Each resource will have a name, a random yield, and a random quality. So I've created an interface for the generic properties and function each resource will have.

Interface

public interface IResource{
    string Name {get;set;}
    string Quality {get;set;}
    int Yield{get;set;}
    string ConvertQualityToString();
    int GetResourceYield();
}

Then I implement the interface in an actual resource base class.

Implementation

public class Resource : MonoBehaviour, IResource{
    public string Name { get; set; }
    public string Quality { get; set; }
    public int Yield { get; set; }

    public string ConvertQualityToString(){
        var val = Random.Range(0, 3);
        string quality;
        switch (val){
            case 1:
                quality = "Good";
                break;
            case 2:
                quality = "Great";
                break;
            default:
                quality = "Poor";
                break;
        }
        return quality;
    }

    public int GetResourceYield(){
        var val = Random.Range(1, 25);
        return val;
    }
}

And finally, I create the specific resource itself and attach it to my game object in the inspector.

Resource Class

public class Granite : Resource{
    void Start(){
        Name = "Granite";
        Quality = ConvertQualityToString();
        Yield = GetResourceYield();
    }
}

Is this the proper way to work this system?  Obviously, the system actually does work as when I log the values on startup, Yield and Quality are randomized and the Name for each resource object I create is correct.  Is there a better way to do this?  Are there things I should change?  Thanks in advance.

A few notes:

All IResource properties have public setters. Are you absolutely certain that other code should be able to modify those? I'd recommend making these read-only unless you have specific reasons not to.
ConvertQualityToString and GetResourceYield look like internal helper methods. Making them part of the IResource interface creates a very confusing API: what's the difference between resource.Yield and resource.GetResourceYield()? With the way the code is structured, I'd expect these methods to be protected.
Are you sure that string is a good type for Quality? If you've got a fixed number of qualities an enum is a more strongly-typed and descriptive choice. And if different qualities have different characteristics then perhaps a Quality class makes more sense. Try to avoid writing 'stringly-typed' code.
ConvertQualityToString gives a 33% chance for each quality. Likewise, GetResourceYield always produces yields between 1 and 25. Are you sure that's applicable to all resources and in all circumstances? Should quality and yield even be determined by a Resource instance itself? What if, for example, some enemies should have a higher chance of dropping high-quality resources? Or what if you want to provide player upgrades that increase the average resource yield?
What's the point of that Granite class? There's no custom logic in there, and the resource name itself is sufficient to distinguish between different resources.
In ConvertQualityToString, there's no need for that local quality variable: you can let each case return a value directly (which also allows you to remove those break statements).
ConvertQualityToString is a strange name: it doesn't really convert anything. Inaccurate and confusing names tend to make code more difficult to work with.