Product of dictionary values Elements that smell funny: argument unpacking to itertools.product. I'm needing sorted keys (even though I don't care about key order in the final result).

"""
>>> d = {'A': [0, 1, 2],
...      'B': [3, 4],
...      'C': [5, 6, 7]}
>>> for element in dict_product(d):
...     print(element)
{'A': 0, 'B': 3, 'C': 5}
{'A': 0, 'B': 3, 'C': 6}
{'A': 0, 'B': 3, 'C': 7}
{'A': 0, 'B': 4, 'C': 5}
{'A': 0, 'B': 4, 'C': 6}
{'A': 0, 'B': 4, 'C': 7}
{'A': 1, 'B': 3, 'C': 5}
{'A': 1, 'B': 3, 'C': 6}
{'A': 1, 'B': 3, 'C': 7}
{'A': 1, 'B': 4, 'C': 5}
{'A': 1, 'B': 4, 'C': 6}
{'A': 1, 'B': 4, 'C': 7}
{'A': 2, 'B': 3, 'C': 5}
{'A': 2, 'B': 3, 'C': 6}
{'A': 2, 'B': 3, 'C': 7}
{'A': 2, 'B': 4, 'C': 5}
{'A': 2, 'B': 4, 'C': 6}
{'A': 2, 'B': 4, 'C': 7}

"""

from itertools import product

def dict_product(d):
    sorted_keys = sorted(d.keys())
    for element in product(*list(d[k] for k in sorted_keys)):
        yield dict(zip(sorted_keys, element))

if __name__ == "__main__":
    import doctest
    doctest.testmod(verbose=True)

You can use the fact that d.keys() and d.values() are always aligned. This gets rid of both the awkward way of constructing the values as well as the sorting of the keys.

def dict_product(d):
    keys = d.keys()
    for element in product(*d.values()):
        yield dict(zip(keys, element))

The assignment of keys is not strictly necessary, but it avoids the look-up every loop iteration.

Note that list(d[k] for k in sorted_keys) should have been written as a list comprehension directly, instead of calling list on a generator comprehension. So [d[k] for k in sorted_keys].

Your doctest might fail in Python 2 or in Python implementations other than cPython. This is because only recently have dictionary keys become ordered (by insertion time) in (c)Python 3. This is still an implementation detail and not something you should rely upon. Since doctest performs a string comparison, it does not notice that the objects are actually the same.

The only way around this, at least the only one I can see, is to use a more sophisticated testing framework, like unittest. This comes with more code overhead, though.


Decorator that supplies arguments to functions I have assignment to generate argument based decorator I wrote like this.

def my_decorator(*deco_args):
    def wrap(f):
        def wrapped_f(*func_args):
            if len(deco_args) == 1 and len(func_args) ==1:
                return f(func_args[0], deco_args[0])
            elif len(deco_args) == 2 and len(func_args) == 2:
                return f(*(deco_args+func_args))
        return wrapped_f
    return wrap

@my_decorator(2)
def pow(x, y):
    return x ** y

print pow(4) #=> 16

@my_decorator("/home", "user")
def path(*args):
   return "/".join(args)

print path("videos", "cats.png") #=> "/home/user/videos/cats.png"

Is there any better way to do this ?

I don't think my_decorator is a very descriptive name, but maybe that's just part of the assignment.  Also, f isn't a very descriptive name for the function you are decorating.

I did my own test:

>>> @my_decorator(2)
... def here(*args):
...     print args
...
>>> here(4, 5)
>>>

Do you know why it doesn't print anything?  It's because you have if and elif in wrapped_f, but one argument to my_decorator and two arguments to here doesn't match either of your conditions, so wrapped_f doesn't know what to do.  To be safe, it does not call the function and just returns None.  That always happens when you don't tell a function what to return: it returns None.  My question is, why do you have those conditions?  I would change that function to:

def wrapped_f(*func_args):
    return f(*(deco_args+func_args))

Your code really has only four lines of reviewable code, so I will just suggest a new (not necessarily better) way:

from functools import partial

class MyDecorator:
    def __init__(self, *args):
        self.args = args
    def __call__(self, func):
        return partial(func, *self.args)

functools.partial takes a function and any other arguments and returns a callable object that, when called, calls the function with the original arguments and whatever other arguments are given.  Our decorator is called with 2 for my above example, so it sets self.args to (2,).  When our MyDecorator object is called (with here as an argument), the __call__ method is executed and we return a partial object with that function and the arguments we got earlier.  Therefore, when the result is called with more arguments, it calls the function with the arguments originally given to the decorator as well as the arguments being given currently.  You can see that partial is very similar to your decorator.  We just needed to make it decorator-compatible.

Edit: As mentioned by @JoeWallis in the comments, you could still use partial() in nested functions similar to yours:

def my_decorator(*deco_args):
    def wrap(f):
        return partial(f, *deco_args)
    return wrap


Filtered WPF textbox I am trying to make a textbox which filters user input to match specified type, so I can discard some of my validation logic.

For example, if I specify ushort I want my textbox to only accept text changes which result in a valid ushort value and nothing else.

This is what I've ended up with:

public abstract class CustomTextBox<T> : TextBox
{
    public static readonly DependencyProperty ValueProperty = DependencyProperty.Register("Value", typeof(T), typeof(CustomTextBox<T>), new FrameworkPropertyMetadata(default(T), FrameworkPropertyMetadataOptions.BindsTwoWayByDefault | FrameworkPropertyMetadataOptions.Inherits, OnValueChanged));
    public T Value
    {
        get { return (T)GetValue(ValueProperty); }
        set { SetValue(ValueProperty, value); }
    }

    protected override void OnPreviewTextInput(System.Windows.Input.TextCompositionEventArgs e)
    {
        if (_regex != null && !_regex.IsMatch(e.Text))
        {
            e.Handled = true;
        }
        else
        {
            base.OnPreviewTextInput(e);
        }
    }

    protected override void OnTextChanged(TextChangedEventArgs e)
    {
        if (String.IsNullOrEmpty(Text))
        {
            return;
        }

        T val;
        if (!TryParse(out val))
        {
            var index = CaretIndex;
            Text = _validText;
            CaretIndex = index > 0 ? index - 1 : 0;
            e.Handled = true;
        }
        else
        {
            Value = val;
            _validText = Text;

        }
    }

    protected CustomTextBox(string regexPattern = null)
    {
        if (!String.IsNullOrEmpty(regexPattern))
        {
            _regex = new Regex(regexPattern);
        }
        _validText = ToString(Value);

        Loaded += OnTextboxLoaded;
    }

    protected override void OnLostFocus(RoutedEventArgs e)
    {
        ValidateText();

        base.OnLostFocus(e);
    }

    protected virtual string ToString(T value)
    {
        return value.ToString();
    }

    protected abstract bool TryParse(out T val);

    private readonly Regex _regex;
    private string _validText;

    private void ValidateText()
    {
        T val;
        if (!TryParse(out val))
        {
            Text = ToString(Value);
        }
    }

    private void OnTextboxLoaded(object sender, RoutedEventArgs e)
    {
        ValidateText();
    }

    private static void OnValueChanged(DependencyObject d, DependencyPropertyChangedEventArgs e)
    {
        var tb = (CustomTextBox<T>)d;
        if (!e.OldValue.Equals(e.NewValue))
        {
            var str = tb.ToString((T)e.NewValue);
            if (!str.Equals(tb.Text))
            {
                tb.Text = str;
            }
        }
    }
}

ushort implementation:

public sealed class UshortTextBox : CustomTextBox<ushort>
{
    public UshortTextBox()
        : base(@"^[0-9]$")
    {
    }

    protected override bool TryParse(out ushort val)
    {
        return UInt16.TryParse(Text, out val);
    }
}

I'm new to this whole custom-control-making, so if you see some rookie mistakes or ways to improve my code, please let me know.

I think you can avoid inheritance here ("Favor Composition Over Inheritance") and use Attached Behavior instead.

Also, did you take a look at MaskedTextBox from Extended WPF Toolkit (http://wpftoolkit.codeplex.com/wikipage?title=MaskedTextBox)?


Merge two lists in Java and sort them using Object property and another condition I have two lists List<Person> list1 = new ArrayList(), list2 = new ArrayList(); (Not the same size), of the class Person:

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

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

    public String getName() {
        return name;
    }

    public int getAge() {
        return age;
    }

    @Override
    public String toString() {
        return "Person{" + "name=" + name + ", age=" + age + '}';
    }

}

I want to create a new list using list1 and list2 sorted by age (descending), but I also another condition that is better explained with an example:

//L1 = from list1, L2 = from list2
//looping trough the new list
Person{name=Alec L1, age=75}
Person{name=Menard L1, age=50} //Bob should be here
Person{name=Bob L2, age=50}

He should, because his age is equal to Menard, Alec is from L1 and two Person from L1 can't be one after another is this kind of situation happens.

A complete list should look like this:

Person{name=Giant L2, age=100}
Person{name=Derp L1, age=50}
Person{name=John L2, age=50}
Person{name=Menard L1, age=44}
Person{name=Lili L2, age=44}
Person{name=Lili L1, age=44}
Person{name=Menard L2, age=44}
Person{name=Bob L1, age=22}
Person{name=Alec L2, age=21}
Person{name=Alec L1, age=21}
Person{name=Herp L2, age=21}
Person{name=Herp L1, age=21}
Person{name=Alice L1, age=12}
Person{name=Little L2, age=5}

Here is my complete code to achieve this result:

    List<Person> list1 = new ArrayList(), list2 = new ArrayList();

    list1.add(new Person("Derp L1", 50));
    list1.add(new Person("Alec L1", 21));
    list1.add(new Person("Herp L1", 21));
    list1.add(new Person("Menard L1", 44));
    list1.add(new Person("Lili L1", 44));
    list1.add(new Person("Alice L1", 12));
    list1.add(new Person("Bob L1", 22));

    Collections.sort(list1, new Comparator<Person>() {
        @Override
        public int compare(final Person object1, final Person object2) {
            return object2.getAge()-object1.getAge();
        }
    });

    list2.add(new Person("Little L2", 5));
    list2.add(new Person("Lili L2", 44));
    list2.add(new Person("Alec L2", 21));
    list2.add(new Person("Herp L2", 21));
    list2.add(new Person("Menard L2", 44));
    list2.add(new Person("Giant L2", 100));
    list2.add(new Person("John L2", 50));

    Collections.sort(list2, new Comparator<Person>() {
        @Override
        public int compare(final Person object1, final Person object2) {
            return object2.getAge()-object1.getAge();
        }
    });
    List<Person> allList = new ArrayList();
    int l1 = 0, l2 = 0;
    for (int i = 0; i < list1.size()+list2.size(); i++)
        if (l1 < list1.size() && l2 < list2.size())
            if (list1.get(l1).getAge() > list2.get(l2).getAge())
                allList.add(list1.get(l1++));
            else if (list1.get(l1).getAge() < list2.get(l2).getAge())
                allList.add(list2.get(l2++));
            else if (allList.size() > 0)
            {
                if (list1.get(l1).getName().contains("L1") == allList.get(allList.size()-1).getName().contains("L1"))
                    allList.add(list2.get(l2++));
                else
                    allList.add(list1.get(l1++));
            }
            else
                allList.add(list1.get(l1++));
        else if (list1.size() == l1)
            allList.add(list2.get(l2++));
        else
            allList.add(list1.get(l1++));

    for (Person person : allList)
        System.out.println(person.toString());

But, is there another way to do it? More elegant code or using some built in Java class?

Your problem statement is not very clear.  As I understand it, you want to have a combined sorted list but interleave elements from list1 and list2 whenever the age is the same.

Here's a simple implementation of that logic.  I used java 8 streams to sort lists and put them in ArrayDeques.

import static java.util.stream.Collectors.toCollection;
import static java.util.Comparator.comparingInt;

static<T> List<T> interleaveSort(List<T> list1, List<T> list2, Comparator<T> cmp) {
    // sort the lists and set them up as queues
    Deque<T> x = list1.stream().sorted(cmp).collect(toCollection(ArrayDeque::new));
    Deque<T> y = list2.stream().sorted(cmp).collect(toCollection(ArrayDeque::new));

    List<T> result = new ArrayList<>(x.size() + y.size());

    while (!x.isEmpty() && !y.isEmpty()) {
        if (cmp.compare(x.peek(), y.peek()) > 0) {
            result.add(y.poll());
        } else {
            // whenever we pick from X, swap queues so next time we will favor the other list
            result.add(x.poll());
            Deque<T> t = x;
            x = y;
            y = t;
        }
    }
    // since the while loop terminates when one of the queues is empty
    // we should add the left-over elements
    result.addAll(x);
    result.addAll(y);

    return result;
}

Then, in your main method:

List<Person> allList=interleaveSort(list1, list2, comparing(Person::getAge).reversed());


Pseudo-random generation able to regenerate backwards Using the accepted answer from this question, I made a random number generator which can regenerate previous numbers in order.

Ideone version

byte[] Encrypt(byte[] ClearData, byte[] Key)
{
   MemoryStream ms = new MemoryStream();

    Rijndael alg = Rijndael.Create();
    alg.KeySize = 128;
    alg.BlockSize = 128;
    alg.Mode = CipherMode.ECB;

    alg.Key = Key;

    CryptoStream cs = new CryptoStream(ms, alg.CreateEncryptor(), CryptoStreamMode.Write);

    cs.Write(ClearData, 0, ClearData.Length);

    cs.Close();

    byte[] encryptedData = ms.ToArray();

    return encryptedData;
}

int RandomValue(int Max)
{
    byte[] FirstHalf = BitConverter.GetBytes(Key);
    byte[] SecondHalf = BitConverter.GetBytes(Key);

    byte[] KeyBytes = new Byte[FirstHalf.Length + SecondHalf.Length];
    FirstHalf.CopyTo(KeyBytes, 0);
    SecondHalf.CopyTo(KeyBytes, FirstHalf.Length);

    byte[] RandValue = Encrypt(new byte[0], KeyBytes);

    int Rand = Math.Abs(BitConverter.ToInt32(RandValue, 0));

    return Rand % Max;
}

int NextValue(int Max)
{
    ++Key;
    return RandomValue(Max);

}

int PreviousValue(int Max)
{
    --Key;
    return RandomValue(Max);

}

static void Main()
{
    byte[] buffer = new byte[sizeof(Int64)];
    Rand.NextBytes(buffer);
    Key = BitConverter.ToInt64(buffer, 0);

    for(int i = 0 ; i < 4 ; ++i)
    {
        Console.WriteLine(NextValue(10));
    }

    Console.WriteLine();

    for(int i = 0 ; i < 4 ; ++i)
    {
        Console.WriteLine(PreviousValue(10));
    }
}

Sample output:

Value 0: 8
Value 1: 1
Value 2: 0
Value 3: 8

Value 2: 0
Value 1: 1
Value 0: 8
Value -1: 7

The desired output from the asker of the stackoverflow question could be obtained by incrementing Key after getting a random value in the NextValue function but for my use this output is more appropriate.

It won't generate backwards properly if the Max changes. It doesn't need to be able to do this especially since Max in unlikely to change between calls, but seeing as how the whole purpose of this is to generate forward and backwards accurately, it seems like something should change.

I'm using Encrypt blindly since I don't understand how it works or even what the ClearData is used for.

What can be changed to improve it and make it more understandable?

Dispose of IDisposable objects with the using statement. For example, objects of type Rijndael, MemoryStream, and CryptoStream. Follow C#'s capitalization conventions, in particular camelCase for parameters (and though not specified in the link, for local variables).


Asking and checking if the answer is correct So I'm very new if JavaScript and my question is this a right way to implement this functionality? How could I improve this code.
<!DOCTYPE html>

        <title>TEST</title>

TEST

How many times can you subtract 6 from 30?

                <button class="incorrect">ДВА РАЗA</button>
                <button class="incorrect">ЧЕТЫРЕ РАЗA</button>
                <button class="incorrect">ТРИ РАЗA</button>
                <button class="incorrect">ШЕСТЬ РАЗ</button>
                <button class="correct">ПЯТЬ РАЗ</button>

Сколько раз вы можете вычесть 6 из 30?

What number should be subtracted from each of 50, 61, 92, 117 so that the numbers, so obtained in this order, are in proportion ?

                <input type="text"></input>
                <button class="check">Check</button>

Какое число нужно вычесть из 50, 61, 92, 117, чтобы числа, полученные в таком порядке, были пропорциональны?

Congrats on getting something like this working! I obviously don't know anything about the background of the project, so keep in mind that while my feedback may be valuable, if it's just a small side project you might not want to overengineer it.
Regardless, here are some points for you to regard:

External scripts: While embedding scripts into the document directly is a slight performance gain, using an external script gives you the luxury of

Mixed languages: While mostly an oddity while reading, it's generally a bad user experience to have two languages mixed together. For concistency in terms of UX, I'd advise that you stick to either one.

Reducing duplicated code: When writing code, it's always nice that if you have to change something, you only have to change it in one place, instead of multiple. This makes your code less error prone to spontaneous changes made by you. Considering this, we could refactor the following:

let input = document.querySelector('input');
if (input.value === '17') {
    input.style.backgroundColor = 'green';
    document.querySelector('#A2').innerHTML = 'Правильно';
} else {
    input.style.backgroundColor = 'red';
    document.querySelector('#A2').innerHTML = 'Неправильно';
}

... into ...
const input = document.querySelector('input');
const answer = document.querySelector('#A2');

if (input.value === '17') {
    input.style.backgroundColor = 'green';
    answer.innerHTML = 'Правильно';
} else {
    input.style.backgroundColor = 'red';
    answer.innerHTML = 'Неправильно';
}

HTML cleanup: If you need to query the document to fetch elements, it's always good to be as explicit as possible. On one line you are querying for input, but what if the page contains multiple input elements later? You should try to get a consistent and explicit HTML structure going for the questions/answers (which also ties in with the point made later on below). An example could be this:

...

    <input type="text"></input>
    <button class="check">Check</button>

...

Each of the elements in this node can be uniquely identified simply by the fact that they are a child of the unique(!) question element with the ID "q1". Queries that looked for input before would now look for #q1 > input instead.
You could also try to remove redundant classes, unless they are used for styling. In the first question, there is one button with the "correct" class, with all others being "incorrect". But the fact that every button that is not explicitely "correct" should be incorrect should be natural, hence you could consider removing these excessive classes.

The points mentioned above will probably increase the overall readability and maintainability of the project, but the biggest concern currently is the extendibility.
Look at it this way: Currently, every time you want to add a new question to the quiz, you need to modify your source code, mostly adding duplicated statements to check correct/incorrect answers. If you give your questions a more concrete structure, you could write reusable code, that is valid for any question that you add!
The exact implementation for this is up to you, but it could help to generate an object from an HTML node, grabbing the question title, description, answering possibilities (and the correct one obviously!) and setting up the question/answer handling.
Always remember: It's almost always better to have something that is extendible and/or scalable without having to modify the source code, just by modifying the markup or configuration.


Multi-page form attempt I have just started playing around with L4 and would like to get some feedback on how I implemented a multi-page form

Here are my routes:

Route::model('appointment', 'Appointment');
Route::get('appointment/book_new/{appointment}', array('as' => 'booking-form', 'uses' => 'AppointmentBookingController@index') );
Route::post('appointment/book_new/verify/{appointment}', 'AppointmentBookingController@verify');
Route::post('appointment/book_new/success/{appointment}', 'AppointmentBookingController@success');

Here is my controller:

class AppointmentBookingController extends BaseController {

    public function index(Appointment $appt)
    {
        return View::make('appointment/bookings/book',  array('appt' => $appt));
    }

    public function verify(Appointment $appt){
        $fields = array('name', 'email', 'description');

        // Declare the rules for the form validation
        $rules = array(
            'name'        => 'required',
            'email'       => 'required|email',
            'description' => 'required',
        );

        // Create a new validator instance from our validation rules
        $validator = Validator::make(Input::all(), $rules);

        // If validation fails, we'll exit the operation now.
        if ($validator->fails())
        {
            foreach($fields as $v)
            {
                Session::flash($v, Input::get($v));
            }
            return View::make('appointment/bookings/book',  array('appt' => $appt, 'errors' => $validator->messages()));
        } else {
            Session::put('booking-form-index', Input::all());
            return View::make('/appointment/bookings/verify',  array('appt' => $appt));
        }
    }

    public function success(Appointment $appt){
        // validate, if valid save the booking.
            // don't do anything for now
            var_dump ( Session::get('booking-form-index') );
            exit();

            // display the success page
            // return View::make('/appointment/bookings/success',  array('appt' => $appt));
}

Here are my views ( book.blade.php )

{{ Form::open(array('url' => '/appointment/book_new/verify/'. $appt->id )) }}

{{ Form::token() }}

    {{ Form::text('name', Session::get('name'), array('placeholder' => 'Name')) }}
    {{ $errors->first('name') }}

    {{ Form::text('email', Session::get('email'), array('placeholder' => 'email@email.com')) }}
    {{ $errors->first('email') }}

    {{  Form::textarea ('description') }}
    {{ $errors->first('description') }}

{{ Form::submit('Click Me!') }}

{{ Form::close() }}

verify.blade.php

{{ Form::open(array('url' => 'appointment/book_new/success/' . $appt->id )) }}

{{  Form::text('validate') }}

{{ Form::submit('Click Me!') }}

{{ Form::close() }}

success.blade.php

Success

Looks pretty standard to me! The only minor tweak I would make just for readability is to eliminate the else in AppointmentBookingController::verify(); since you return in the if, the else block is not necessary, and using guard clauses like this will (arguably) make your code more readable. See Invert "if" statement to reduce nesting.


Repeated comparison of sorted subarrays This question is from a recently concluded competition on Codechef.

You are given an array A of size n, and there are q queries of the form a, b, c, d such that a <= b and c <= d.

For each query, we need to check if the sub arrays A[a:b] and A[c:d] are similar. The sub-arrays are of same length and are similar if their sorted representations contains only one mismatch in all of their positions. It is also possible that two sub arrays can overlapped.

For example

A = [36, 24, 12, 24, 60]
a = 0, b = 2, c = 2, d = 4
sorted A[a:b] = [12, 24, 36]
sorted A[c:d] = [12, 24, 60]

They differ at only one index, so they are similar.

I tried to solve this in brute-force method which could solve preliminary test cases, but getting TLE for large inputs. Following is my code. How can we efficiently solve this problem?

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

/**
 * Created by renaganti on 6/11/17.
 */
public class Cloning {
    public static void main(String []args) {
        Scanner reader = new Scanner(System.in);
        int t = reader.nextInt();
        while(t-- > 0) {
            int n = reader.nextInt();
            int q = reader.nextInt();
            int i;
            int []arr = new int[n];
            for(i = 0; i < n; i++) {
                arr[i] = reader.nextInt();
            }

            for(i = 0; i < q; i++) {
                int a = reader.nextInt();
                int b = reader.nextInt();
                int c = reader.nextInt();
                int d = reader.nextInt();
                if( similar(arr, a, b, c, d) )
                    System.out.println("YES");
                else
                    System.out.println("NO");

            }
        }
    }
    private static boolean similar(int []arr, int a, int b, int c, int d) {
        int [] arr1 = new int[b-a+1];
        int [] arr2 = new int[d-c+1];

        for(int j = a-1, k = c-1, l = 0; j < b; j++, k++) {
            arr1[l] = arr[j];
            arr2[l] = arr[k];
            l++;
        }

        Arrays.sort(arr1);
        Arrays.sort(arr2);

        int diff = 0;
        for(int j = 0; j < arr1.length; j++) {
            if( arr1[j] != arr2[j])
                diff++;
            if( diff > 1 )
                break;
        }
        if( diff < 2 )
            return true;
        else
            return false;
    }
}

Test for the easy cases

    private static boolean similar(int []arr, int a, int b, int c, int d) {

This misses several quick and easy to check possibilities.

          if (a == c && b == d) {
              // each segment is similar to itself
              return true;
          }

Check if it's the same segment.  If so, done.

          if (b - a != d - c) {
              // if different sizes, not similar
              return false;
          }

The segments have to be the same size to be similar.

          if (a < c) {
              if (b >= c) {
                  int temp = b + 1;
                  b = c - 1;
                  c = temp;
              }
          } else {
              if (d >= a) {
                  int temp = d + 1;
                  d = a - 1;
                  a = temp;
              }
          }

If either segment overlaps the other, then we can remove the overlap.  We don't have to check those elements, as we already know that they are the same.  This eliminates all the work of checking the overlap.  In particular, we don't have to copy or sort those elements.

The two cases are a < c < b < d and c < a < d < b.  We test for the order of the first three, and the final one must be in that order to keep the sizes equal.  I.e. b = a + size - 1, so d = c + size - 1.  Therefore if a < c, then b < d.

Consider the worst case for your code.  That's when a and c both equal 0, while b and d equal arr.length - 1.  Your code copies the entire array twice, then sorts the entire array twice, and finally iterates over both arrays to compare each element.  This code would simply return true in constant time.

There's a method for that.

        int [] arr1 = new int[b-a+1];
        int [] arr2 = new int[d-c+1];

        for(int j = a-1, k = c-1, l = 0; j < b; j++, k++) {
            arr1[l] = arr[j];
            arr2[l] = arr[k];
            l++;
        }

We could use Arrays.copyOfRange, as such:

        int[] arr1 = Arrays.copyOfRange(arr, a, b + 1);
        int[] arr2 = Arrays.copyOfRange(arr, c, d + 1);

Then we don't have to write out the logic.

Don't do extra checks

        for(int j = 0; j < arr1.length; j++) {
            if( arr1[j] != arr2[j])
                diff++;
            if( diff > 1 )
                break;
        }
        if( diff < 2 )
            return true;
        else
            return false;

You could rewrite this as

        for (int j = 0; j < arr1.length && diff <= 1; j++) {
            if ( arr1[j] != arr2[j]) {
                diff++;
            }
        }

        return diff <= 1;

That's shorter but takes no less time.

The simpler return is a common pattern.  In Java, you can simply return the result.  You don't need to process it with an if/else.

I prefer to compare to 1 rather than 2, as 1 is what appears in the problem statement.  I prefer to keep the code as close to the problem statement as possible to make it easier to read later.

Even better might be

        for (int j = 0; j < arr1.length; j++) {
            if ( arr1[j] != arr2[j]) {
                diff++;
                if ( diff > 1 ) {
                    return false;
                }
            }
        }

        return true;

We only need to check diff if we increment it.  The rest of the time, it stays the same as it was.

Binary search

I haven't tried to write up a solution, but you don't have to copy and sort both array segments.  If you copy and sort one, you can just iterate over the other and binary search the first.  If there is a match, you go to the next one.  If no match, you mark off the value as if matched and increase the difference count.

This won't give any asymptotic improvement, but it may make the program a little faster.

Dynamic programming

Again, I haven't tried to write up a solution, but realize that two segments are similar if their first elements are the same and the remainder of the segments are similar.  So it might be worthwhile saving the work that we've done in sorting and comparing previous segments.  Then we can reduce new queries to previous queries with some added entries.

This would likely be memory intensive.

New data structure

Consider the possibility of a data structure that kept track of the original order and the sorted order and could map between them.  At the simplest level, a sorted order that remembered the original placement.  But at a more complex level, possibly a trie-like structure.

Again, this would likely be memory intensive.


Is there a better or more elegant way to code this CSV cell-parser (using CSVReader)? I'm currently doing my first internship and had to create an application that will check through the first row of a CSV for valid or invalid input. Is there a more elegant way to code or to refactor my FileValidator class? This is my first time programming in a professional environment, so I'm still trying to learn how to "clean code". If you see any bad practices, please say so.

package util;

import java.io.File;
import java.io.FileNotFoundException;
import java.io.FileReader;
import java.io.IOException;
import java.util.List;

import org.apache.log4j.Logger;

import au.com.bytecode.opencsv.CSVReader;

public class FileValidator {
    private CSVReader reader;
    List<String[]> row;
    private File file;
    private String date;
    private String[] header;

    private final static Logger log = Logger.getLogger(FileValidator.class);
    private static final int HEADER_MAX_COLUMNS = 4;
    private static String[] BUSINESS = { "SAFC", "FLDB", "RGSP", "JKAA",
            "FAKJ", "SFDK" };

    public boolean readFile(File newFile, String date) // receive File object
                                                        // and date provided by
                                                        // user
    {
        try {
            this.file = newFile;
            reader = new CSVReader(new FileReader(file));
            row = reader.readAll();
            header = row.get(0); // assign first row of CSV to a List<String[]>
                                    // (list of Strings(cells))
            this.date = date;
        } catch (FileNotFoundException e) {
            e.printStackTrace();
        } catch (IOException e) {
            e.printStackTrace();
        }
        return true;
    }

    public boolean validateFile() {
        boolean[] checkHeader = { checkNumColumns(), checkIfEmpty(),
                checkDate(), checkFileName(), checkBusiness() };

        for (int i = 0; i < checkHeader.length; i++) {
            if (checkHeader[i] == false)
                return false;
        }
        return true;
    }

    public boolean checkNumColumns() // check if number of header columns are
                                        // correct
    {
        if (!(row.get(0).length <= HEADER_MAX_COLUMNS)) {
            return false;
        } else
            return true;
    }

    public boolean checkIfEmpty() // check if any cells are empty
    {
        for (int i = 0; i < HEADER_MAX_COLUMNS; i++) {
            if (!header[i].trim().equals(header[i]) || header[i] == null
                    || header[i] == "" || header[i].length() == 0)
                return false;
        }
        return true;
    }

    public boolean checkDate() // check if user-given date matches header:
                                // cell[0]
    {
        if (!header[0].equals(date)) {
            return false;
        } else
            return true;
    }

    public boolean checkFileName() // check if file name equals cell[1]
    {
        if (!header[1].equals(file.getName())) {
            return false;
        } else
            return true;
    }

    public boolean checkBusiness() // check if business name (cell[2]) matches
                                    // any from static string list
    {
        for (int i = 0; i < BUSINESS.length; i++) {
            if (header[3].equals(BUSINESS[i]))
                return true;
        }
        return false;
    }
}

Declare your BUSINESS constant as final
readFile will currently always return 'true', barring some unforeseen exception. Seems like you could make it a void method as well.
validateFile is a bit bad method name. Imagine it being used elsewhere: if( x.validateFile() ) { /* do something */ }. That looks like you're asking whether to validate a file or not. If you want to call the method that, make it return void and make it throw an exception if the validation fails. Otherwise call it something like isFileValid().
All the methods in your class are defined as public, which makes little sense. You could make them all but one private and enter the File and date in a constructor. Or you could just make one static method there which would call the other methods. As far as I can see there's no need for a FileValidator object to actually have a state.
validateFile() makes a fancy but redundant boolean array of all the validation results. In reality you do not need to call all the validators if the first one of them fails. Try this instead:

private boolean validateFile() {
    return checkNumColumns() && checkIfEmpty() && checkDate() && checkFileName() && checkBusiness();
}

Your checkIfEmpty() method has a possible NullPointerException: it first trims header[i], and only afterwards checks for null, at which point it's too late. Also, String comparisons with "=="s is just wrong, as that checks that the memory locations of the two strings are the same, saying nothing about their contents. string == "" would anyway be the same as string.length() == 0 if it worked. Besides, nowadays the correct way to check for an empty string is `string.isEmpty()".
checkDate() does not need an if-else block: just do return header[0].equals(date);. (A completely other thing is that representing dates with Strings isn't necessarily the best idea ever when java.util.Date exists as well.)
The previous point also applies for checkFileName(). Those header[1] names are not readable anyway, you will want to pass the "header[1]" as an argument to the method (which should be static and private) and then call the argument "filename".
checkBusiness() is pretty good as it stands, just have it take the business as an argument instead of using "header[3]", just like I noted about the previous method.
Don't forget to write unit tests for your class.


csvshuf: a tool to shuffle CSV columns written in Python Edit: After reading Gareth's answer I have pushed an updated version of the code to github.

I needed to shuffle cells from specific columns in several CSV files. One of the requirements was to be able to do derangement (shuffling that leaves no element in its original place), I read about Sattolo's algorithm.

So I started writing csvshuf this afternoon. Code was originally based in csvcut. I tried to make it good but I am sure it can be improved as I have little experience programming in Python. I would be grateful if I get some code review.

Code:

import csv
import sys
import random
import getopt

# From https://softwareengineering.stackexchange.com/q/218255/149749
def shuffle_kfy(items):
    i = len(items) - 1
    while i > 0:
        j = random.randrange(i + 1)  # 0 <= j <= i
        items[j], items[i] = items[i], items[j]
        i = i - 1
    return items

def shuffle_sattolo(items):
    i = len(items)
    while i > 1:
        i = i - 1
        j = random.randrange(i)  # 0 <= j <= i-1
        items[j], items[i] = items[i], items[j]
    return items

def shuffle(items, mode):
    if mode == 'kfy':
        return shuffle_kfy(items)
    if mode == 'sattolo':
        return shuffle_sattolo(items)

    random.shuffle(items)
    return items

opts, args = getopt.getopt(sys.argv[1:], "c:C:d:o:q:tks", [])
if args:
    i = open(args[0], 'U')
else:
    i = sys.stdin

delimiter = ','
output_delimiter = ','
cols = None
no_cols = None
quotechar = None
search_mode = ''

if opts:
    opts = dict(opts)
    if '-c' in opts:
        cols = map(int, opts['-c'].split(','))
    elif '-C' in opts:
        no_cols = map(int, opts['-C'].split(','))
    if '-k' in opts:
        search_mode = 'kfy'
    elif '-s' in opts:
        search_mode = 'sattolo'
    if '-t' in opts:
        delimiter = "\t"
    elif '-d' in opts:
        delimiter = opts['-d']
    if '-o' in opts:
        output_delimiter = opts['-o']
    if '-q' in opts:
        quotechar = opts['-q']

if cols and 0 in cols or no_cols and 0 in no_cols:
    print("Invalid column 0. Columns are 1-based")
    exit(1)

reader = csv.reader(i, delimiter=delimiter, quotechar=quotechar)
headers = next(reader)

table = []
for c in range(len(headers)):
    table.append([])

for row in reader:
    for c in range(len(headers)):
        table[c].append(row[c])

if not cols and not no_cols:
    cols = range(len(headers))
elif no_cols:
    cols = list(set(range(len(headers))) - set(no_cols))

for c in cols:
    if c > len(headers):
        print('Invalid column {}. Last column is {}').format(c, len(headers))
        exit(1)
    table[c - 1] = shuffle(table[c - 1], search_mode)

table = zip(*table)

writer = csv.writer(sys.stdout, delimiter=output_delimiter)
writer.writerow(headers)
for row in table:
    writer.writerow(row)

Usage:

csvshuf -c1 foobar.csv
(shuffles the first column of each row of foobar.csv using Python's shuffle())

svshuf -c2 -k foobar.csv
(shuffles the second column of each row using Knuth-Fischer-Yeats algorithm.)

svshuf -c3 -s foobar.csv
(shuffles the third column of each row using Sattolo's algorithm.)

csvshuf foobar.csv
(shuffles all the columns of foobar.csv)

csvshuf -C1 foobar.csv
(shuffles all the columns but the first of foobar.csv)

head -10 foobar.csv | csvshuf -c 1,3
(shuffles the first and third columns of the first ten lines of foobar.csv)

csvshuf -c1,3 -d "|" foobar.csv
(shuffles the first and third columns of the pipe-delimited foobar.csv)

csvshuf -c 1,3 -t foobar.csv
(shuffles the first and third columns of the tab-delimited foobar.csv if present, the -d option will be ignored.)

csvshuf -c 1,2,3 -d "|" -o , foobar.csv
(shuffles the first three columns of the pipe-delimited foobar.csv; output will be comma-delimited.)

csvshuf -c 1,2,3 -o "|" foobar.csv
(shuffles the first three columns of the comma-delimited foobar.csv; output will be pipe-delimited.)

csvshuf -c 1,2 -d "," -q "|" foobar.csv
(shuffles the first two columns of the comma-delimited, pipe-quoted foobar.csv.)

There are no docstrings. It's hard to review code when we don't know what it is supposed to do.
The various shuffle functions both modify their items argument and return it. It would be clearer if these functions either modified their argument without returning it (like random.shuffle) or returned a new list without modifying the original (like random.sample). Doing both is redundant and confusing.
The shuffle functions can be simplified using reversed and range:

def shuffle_sattolo(items):
    """Shuffle items in place using Sattolo's algorithm."""
    _randrange = random.randrange
    for i in reversed(range(1, len(items))):
        j = _randrange(i)  # 0 <= j < i
        items[j], items[i] = items[i], items[j]

Note that I've cached random.randrange in a local variable. This is to avoid having to look it up again each time around the loop.
The shuffle_kfy function is redundant because random.shuffle already implements the Fisher–Yates shuffle.
Using a string argument to select a mode of operation is error-prone. If you mistyped the string, for example shuffle(items, 'satolo'), then you would not get an error, it would just silently do the wrong thing.
The shuffle function is redundant. Instead of assigning a search mode while parsing the arguments, assign a shuffle function:

shuffle = random.shuffle

# ...

if '-k' in opts:
    shuffle = shuffle_kfy
elif '-s' in opts:
    shuffle = shuffle_sattolo

Having code at the top level of a module makes it hard to test — for example, you can't test the shuffle function by itself, because you can't import the module without running all the top-level code. It is a good idea to put top-level code into a main function and guard it with if __name__ == '__main__':.
The code processes the command-line options by transforming them into a dictionary and then looking up each valid argument. But this means that there are no error messages for invalid arguments. If you look at the getopt examples in the manual, you'll see that they do:

for o, a in opts:
    if o == '-v':
        # process option
    else:
        # report error

The name i is normally used for index variables, so it's misleading to use it for an input file.
Error messages should go to standard error, not standard output:

sys.stderr.write("Invalid column 0. Columns are 1-based.\n")

This is particularly important here, because this program writes its results to standard output, and so probably the user is redirecting standard output to a file. If you write error messages to standard output, they will go to the file and the user will not see them.
It's a better idea to use the argparse module for parsing command-line arguments. With argparse the code is more explicit, it automatically emits error messages for invalid arguments, it has various types of built-in argument validation and conversion, and it has built-in support for the --help option. Something like this:

def column_list(string):
    """Validate and convert comma-separated list of column numbers."""
    try:
        columns = list(map(int, string.split(',')))
    except ValueError as e:
        raise argparse.ArgumentTypeError(*e.args)
    for column in columns:
        if column < 1:
            raise argparse.ArgumentTypeError(
                "Invalid column {!r}: column numbers start at 1."
                .format(column))
    return columns

def main():
    parser = argparse.ArgumentParser(
        description="Shuffle columns in a CSV file")
    parser.add_argument('infile', type=argparse.FileType('r'), nargs='?',
                        default=sys.stdin, help='Input CSV file')
    parser.add_argument('-s', '--sattolo',
                        action='store_const', const=shuffle_sattolo,
                        dest='shuffle', default=random.shuffle,
                        help="Use Sattolo's shuffle.")
    col_group = parser.add_mutually_exclusive_group()
    col_group.add_argument('-c', '--columns', type=column_list,
                           help="Comma-separated list of columns to include.")
    col_group.add_argument('-C', '--no-columns', type=column_list,
                           help="Comma-separated list of columns to exclude.")
    delim_group = parser.add_mutually_exclusive_group()
    delim_group.add_argument('-d', '--delimiter', type=str, default=',',
                             help="Input column delimiter.")
    delim_group.add_argument('-t', '--tabbed', dest='delimiter',
                             action='store_const', const='\t',
                             help="Delimit input with tabs.")
    parser.add_argument('-q', '--quotechar', type=str, default='"',
                        help="Quote character.")
    parser.add_argument('-o', '--output-delimiter', type=str, default=',',
                        help="Output column delimiter.")
    args = parser.parse_args()

    reader = csv.reader(args.infile, delimiter=args.delimiter,
                        quotechar=args.quotechar)

    # ... and so on ...


Singly Linked List "Strange sorting" exercise I'm currently working through "Algorithms in java" by Robert Sedgewick (3rd edition, german version) on my own and am trying to solve one of the exercises there.

One of these involves taking a singly linked list (type not specified) and rearranging the nodes. The sorting involves first having all nodes appear that were at "even" positions in the prior linked list and after that having all nodes appear that were at "uneven" positions in the prior linked list. All this while every node should keep their relative position. I chose to write this for a linked list with a null reference at its tail and a true node as head.

So short example: a list of values 4-5-7-3-2-1-null should rearrange to
4-7-2-5-3-1-null.

I wrote some code that worked (the strangeSort (Node x) method) but can't think of it as an elegant solution. I am convinced that there must be a way out there to make this neater, especially the for-loop in the strangeSort method. Base idea behind it was to make the original linked list basically 2 linked lists with different head nodes and then attaching one linked list at the end of the other.

The code:

public class Test {
    static class Node
    {
        int val; Node next;
        Node(int v){val=v;}
    }

    public static void main (String[] args)
    {
        /*Create Linked list of length N*/
        int N=Integer.parseInt(args[0]);
        Node head = new Node(0);
        Node x = head, x2;
        for (int i=1; i<N; i++, x = x2)
        {
            x2 = new Node(i);
            x.next = x2;
        }

        /*Prints out linked list for checking purposes*/
        for (Node t = head; t!=null; t=t.next)
        {
            System.out.print(t.val+" -> ");
        }

        /*Rearrange Linked list*/
        strangeSort(head);

        /*Prints out linked list after sorting for checking purposes*/
        System.out.println();
        for (Node t = head; t!=null; t=t.next)
        {
            System.out.print(t.val+" -> ");
        }

    }

    public static void strangeSort (Node x)
    {
        Node headu = x, heade = x.next;
        Node t,s;
        for (t=headu, s=heade ; t!=null && s!=null ; t=t.next , s=s.next)
        {
            if (t.next==null){break;}
            t.next = t.next.next;
            if (s.next ==null){break;}
            s.next = s.next.next;
        }
        if (t==null){t=heade;}
        else if (t.next==null){t.next = heade;}
    }
}

As you can see, the forloop of strangeSort contains a fairly inelegant way to make sure I don't get a NullPointerException in the end through the fact that I am traversing 2 nodes every iteration of the loop.

Interesting exercise.

I think you missed an easy solution to it which I'll get to, but a code-review first...

Vertical whitespace is a critical part of the readability of code... and you are missing a lot of it. Code like:

    if (t.next==null){break;}
    t.next = t.next.next;
    if (s.next ==null){break;}
    s.next = s.next.next;
}
if (t==null){t=heade;}
else if (t.next==null){t.next = heade;}

should be:

        if (t.next==null) {
            break;
        }
        t.next = t.next.next;
        if (s.next ==null) {
            break;
        }
        s.next = s.next.next;
    }
    if (t==null) {
        t=heade;
    } else if (t.next==null) {
        t.next = heade;
    }

and ...

static class Node
{
    int val; Node next;
    Node(int v){val=v;}
}

should be:

static class Node {
    int val;
    Node next;
    Node(int v) {
        val=v;
    }
}

Note that the community consensus for code style in Java is that {} braces should be opening at the end of the line, not the start of the next. I understand if your local work environment may have different "rules", but you should be aware that it is different to common Java style.

Your variable names are awkward. Things like "uneven" are not the opposite of "even" in this case, the opposite is "odd". Also, s and t are variables that I could not "imagine" what they are short for. If a 1-letter variable name is not special, it should at least make some sense... i is for index, use x and y in coordinate type variables, use p for position, or something.... but in your context t=headu, s=heade and I cannot think of something that s and t could mean.

Your main method, and the way it accepts the argument for the "size" of the test list, confused me a bit. I had to figure out the format of the input, etc. and how to make it run. You should comment things like that.

Now, about your algorithm.... it is not bad, it essentially builds two lists, and appends the one to the other, but I am concerned about the exit conditions of the loop... you have multiple of them (the for-loop conditions, and then multiple break conditions with "recovery" after the loop.

This is because you are tracking multiple pointers through the source list.

If you just iterate with one pointer in the source list, and have conditional code inside the loop, it's cleaner.

public static void strangerSort(Node list) {
    if (list == null || list.next == null) {
        // already stranger sorted.
        return;
    }

    Node eventail = list;
    Node oddhead = list.next;
    Node oddtail = oddhead;
    boolean even = false;
    // start from 3rd item in list
    // (guard condition guarantees at least 2 nodes)
    Node next = oddtail.next;
    while (next != null) {
        Node n = next;
        next = next.next;
        even = !even;
        if (even) {
            eventail.next = n;
            eventail = n;
        } else {
            oddtail.next = n;
            oddtail = n;
        }
    }
    eventail.next = oddhead;
    oddtail.next = null;
}


Fill in the blanks game So I've coded a fill in the blanks game but my last procedure, play_game() is far too long.
Trying to figure out how to divide it into two functions:

Initialize
process_answer

import time
from time import sleep

name = raw_input("Hey there, what's your name?") #Basic raw_input
intro = "Welcome to my Udacity, Mad-Libs game " + name #text plus value of "name"

blanks = ["__1__","__2__","__3__", "__4__"]

questions_easy = '''Python is an __1__ oriented __2__ programming language that has become very popular. It was created by __3__ Van Rossum.Python has been used widely, especially for __4__ learning.'''

questions_medium = '''Python has full support for object-__1__ programming including user-defined __2__, inheritance,
 and run-time binding of methods. Python has an extensive standard __3__, which is one of the main reasons for its popularity.
 Python was first created in __4__ 1989'''

questions_hard = '''Python is intended to be a highly readable __1__. It is designed to have an clear visual __2__.
Essencially the first step when creating a new program is creating a "draft" code also called __3__. This will allow you to have a basic idea on how the code
should be structured. When your code is complete and ready to testing for __4__ inputs and outputs.'''

answers_list_easy = ["object", "dynamic", "Guido", "machine"]
answers_list_medium = ["oriented", "classes", "library", "December"]
answers_list_hard = ["language", "layout", "pseudocode", "valid"]

game_data = {
   'easy': {
        'quiz': questions_easy,
        'answers': answers_list_easy,
        'message': "you chose easy, okay"
    },
   'medium': {
        'quiz': questions_medium,
        'answers': answers_list_medium,
        'message': "You chose medium, a fair challenge"
    },
   'hard': {
        'quiz': questions_hard,
        'answers': answers_list_hard,
        'message': "So you're a tough nugget aren't ya!!"
    }
}

def get_level(): #Inputs easy, medium or hard. Output, Relevant questions and answers.
    pick_level = raw_input("Type in your difficulty level. Choose from easy, medium or hard")
    while pick_level not in ["easy", "medium", "hard"]:
        pick_level = raw_input("Invalid input, please try again. Choose from easy, medium or hard")
    print game_data[pick_level]['message']
    return pick_level

def check_answer(user_answer, answers_list, answers_index):
    if user_answer == answers_list[answers_index]:
        return "right_answer"
    return "Wrong"
    pass

def you_lost():
    print "Wawaooohh, you lost..... :("
    time.sleep (3)
def you_win():
    print "We got ourselves a winner!!"
    time.sleep (3)

def play_game():
    level = get_level()
    quiz = game_data[level]['quiz']
    print quiz
    answers_list =  game_data[level]['answers']
    blanks_index = 0
    answers_index = 0
    guesses = 3

    while blanks_index < len(blanks):
        user_answer = raw_input("So what's your answer to question " + blanks[blanks_index] + "? : ")
        if check_answer(user_answer,answers_list,answers_index) == "right_answer":
            print "\n nice job! that is the right answer!\n"
            quiz = quiz.replace(blanks[blanks_index], user_answer.upper())
            blanks_index += 1
            answers_index += 1
            guesses = 3
            print quiz
            if blanks_index == len(blanks):
                return you_win()

        else:
            guesses -= 1
            if guesses == 0:
                return you_lost()
                break
            print "Wrong!! Please try again. you have " + str (guesses) + " guesses left!"

play_game()

from time import sleep

Since you already imported time and are only calling time.sleep, this import is obsolete.

name = raw_input("Hey there, what's your name?") #Basic raw_input
intro = "Welcome to my Udacity, Mad-Libs game " + name #text plus value of "name"

The comments at the end of the lines do not tell you anything that is not already in the code, so you don't need them. Comments should usually tell you, why something is done, not what is done. What is done is already in the code.

answers_list_easy = ["object", "dynamic", "Guido", "machine"]

Having the type in the variable name is not needed, and rather bad practice. You might want to change the type later, for example to a tuple. Using a plural name (like answers_easy or easy_answers) already tells you (and possibly other developers) that the variable contains a collection, probably a sequence of answers (and a list is a kind of sequence).

game_data = {
   'easy': {
        'quiz': questions_easy,
        'answers': answers_list_easy,
        'message': "you chose easy, okay"
    },
   'medium': {
        'quiz': questions_medium,
        'answers': answers_list_medium,
        'message': "You chose medium, a fair challenge"
    },
   'hard': {
        'quiz': questions_hard,
        'answers': answers_list_hard,
        'message': "So you're a tough nugget aren't ya!!"
    }
}

This is not pretty, and probably useless. Since you need to access these values via strings as keys anyway, you might as well just access the global variables. These nested dictionaries do not really simplify the access to the variables, rather make it harder, and since at no point in the future "easy" will suddenly point towards questions_medium, you do not need to abstract the variable access.

def get_level(): #Inputs easy, medium or hard. Output, Relevant questions and answers.
    pick_level = raw_input("Type in your difficulty level. Choose from easy, medium or hard")
    while pick_level not in ["easy", "medium", "hard"]:
        pick_level = raw_input("Invalid input, please try again. Choose from easy, medium or hard")
    print game_data[pick_level]['message']
    return pick_level

Printing the level here is a side effect that you might want to prevent. The method should do only what its name implies, which is getting the level. A better name might be ask_level or input_level, since get is normally used for just returning a value that is already there. Then you can print the return value where you call this method.

def check_answer(user_answer, answers_list, answers_index):
    if user_answer == answers_list[answers_index]:
        return "right_answer"
    return "Wrong"
    pass

This name is not good, because it does not tell you what kind of answer is being checked, and what a positive result means. A better name would be is_answer_correct, or just is_correct if the parameter names are explicit enough (which I would say here they are).

Considering that the method just does a simple logical comparison with values that are all provided as parameters, you might just remove the method and replace its calls with the comparison.

As already hinted by kyrill in a comment, the method should not return strings (which are even very inconsistent here), but Trueor False boolean values. Then you can handle printing out the strings where you call the method depending on its result. And the pass does not do anything, actually it never does anything, but everything that comes after a return will not be reached.

you_lost and you_win are bad method names, and the methods alltogether are obsolete, since it is more readible to just see these 2 lines in your code than not knowing what you_lost() actually does.

blanks_index = 0
# [...]
while blanks_index < len(blanks):
    # your code

Here you should iterate using a for-loop:

for blank in blanks:
    # your code


Search files content fast in powershell I need to search a large repository of files for a specific resource, and return it only if it is used or not.

It is working as expected, but it is very time consuming. To search for 50 resource names takes around 200 seconds. I have many more than 50.

I found this question that helped a bit, but I was wondering if there is anything else I can do to speed it up.

$searchList = Get-ChildItem -r *.cs -OutBuffer 1000000
$usedIcons = @()

foreach ($name in $testNames) {
    $pattern = "Resources."+$name
    $found = $searchList | Select-String -Pattern $pattern -SimpleMatch -Quiet
    if($found -eq $true){
        Write-Host $name
        $usedIcons += $name
    }
}

I also suggest to use a better RegEx but stay with PowerShell.

Without exactly knowing your $testnames you can manually or automatically build a RegEx with a lookbehind and alternations and read the *.cs files only once.

## build some sample files
'foo','bar','baz'|ForEach-Object{"Resources.$_" > "$_.cs"}

$testnames = [RegEx]"(?<=Resources\.)(foo|bar|baz)"
# (zero length look behind assertion)(alternation)

$Result =  Get-ChildItem *.cs -Recurse -File |
  Select-String -Pattern $testnames|
    Select-Object @{n='UsedIcons';e={$_.Matches.Groups[0].Value}} -Unique

$Result

Sample output:

UsedIcons
---------
bar
baz
foo


Desktop notifications for queue items I wasn't satisfied with Simon's alert on the orange alert. I wanted to know if even 1 review item was available, so I made some modifications to the userscript so that it runs on the Review page, and only alerts you when there are reviews that show a positive number.

This is Simon's version, and here is my version:

/** @preserve
// ==UserScript==
// @name Review Queue Notification
// @author Malachi Edited Simon Forsberg Created
// @description Shows a desktop notification when there review items in the queue.
// @namespace https://github.com/Zomis/SE-Scripts
// @grant GM_getValue
// @grant GM_setValue
// @grant GM_notification
// @match *://*.stackexchange.com/review*
// @match *://*.stackoverflow.com/review*
// @match *://*.superuser.com/review*
// @match *://*.serverfault.com/review*
// @match *://*.askubuntu.com/review*
// @match *://*.stackapps.com/review*
// @match *://*.mathoverflow.net/review*
// ==/UserScript==
*/

var KEY_NEXT = 'NextReload';
var DELAY = 60 * 1000; //60,000 milliseconds
var currentTime = Date.now ? Date.now() : new Date().getTime();
var lastTime = GM_getValue(KEY_NEXT, 0);
var nextTime = currentTime + DELAY;
GM_setValue(KEY_NEXT, nextTime);

var timeDiff = Math.abs(lastTime - currentTime);
setTimeout(function(){
    window.location.reload();
}, DELAY);

var title = document.title.split(' - '); // keep the site name
document.title = 'Desktop Notifications - ' + title[1];

// a way to detect that the script is being executed because of an automatic script reload, not by the user.
if (timeDiff <= DELAY * 2) {
    var notifications = [];

    var reviewCount = 0;
    var reviewItems = document.getElementsByClassName('dashboard-num');

    for (var i = 0; i < reviewItems.length; i++){
        reviewCount += parseInt((reviewItems[i].getAttribute("title")).replace(',',''), 10);
        console.log(reviewItems[i]);
    }
    console.log(reviewCount);
    if (reviewCount > 0) {
        notifications.push(reviewCount + ' Review Items');
    }

    if (notifications.length) {
        var details = {
            title: document.title,
            text: notifications.join('\n'),
            timeout: 0
        };
        GM_notification(details, null);
    }
}

This is also available on my GitHub as a userscript.

Follow-up question with new GitHub repo

This is mostly personal preference, but, I usually stack the @matches  in descending size:

// @match *://*.stackexchange.com/review*
// @match *://*.stackoverflow.com/review*
// @match *://*.superuser.com/review*
// @match *://*.serverfault.com/review*
// @match *://*.askubuntu.com/review*
// @match *://*.stackapps.com/review*
// @match *://*.mathoverflow.net/review*

into:

// @match *://*.stackexchange.com/review*
// @match *://*.stackoverflow.com/review*
// @match *://*.mathoverflow.net/review*
// @match *://*.serverfault.com/review*
// @match *://*.askubuntu.com/review*
// @match *://*.stackapps.com/review*
// @match *://*.superuser.com/review*

Just looks nicer, I suppose.

var title = document.title.split(' - ');:

You don't use title for anything other than title[1], so just use var title = document.title.split(' - ')[1] instead.

.replace(',',''): You should have a space after ','.

You could consider replacing the .length style for loop with a for (var i in reviewItems) style loop. If not, you can also declare reviewCount in the for loop like: for (var reviewCount = 0, i = 0;.

var details = {
        title: document.title,

Desktop Notifications - Code Review Stack Exchange sounds a little bulky and over-the-top. Review Items is a better name for it, and you could consider removing Stack Exchange, but, consider the effect that would have on MSE.

I can't see the point in using notifications as an array, as it should really just have the one notification.

if (reviewCount > 0) {
    notifications.push(reviewCount + ' Review Items');
}
if (notifications.length) {
    var details = {
        text: notifications.join('\n'),

if (reviewCount > 0) {
    notification = reviewCount + ' Review Items';
}
if (notification) {
    var details = {
        text: notification,

or even just move reviewCount + ' Review Items' into details, like:

if (reviewCount > 0) {
    var details = {
        text: reviewCount + ' Review Items',

As of ECMAScript 5, the default radix used in parseInt is supposed to be \$10\$, before that, \$0\$ would get parsed as a octal number instead of a decimal number, and, as it seems, you can just omit that optional radix argument entirely. Although, as @EthanBierlein pointed out in the comments; MDN recommends not to omit it.


Search DataTable by column for a specific row I would like to know if there are ways to make this run faster. Not a big concern right now, but I would like to think long term, it might be important.

This is C#, and I can not change the input parameters data types, but basically I have to make something like the SQL: SELECT returnCol FROM dt WHERE ColName = ColValue

As the datatable itself: it has more than 100 columns, but it should not too many rows, usually ~10 rows but it can spike up to 100 rows sometimes. This is part of a data parse that is happening once a minute, that is why I am trying to see if there are ways for enhancement.

private string FindInT(DataTable dt, string ColName, string ColValue, string returnCol)
        {
            foreach (DataRow row in dt.Rows)
            {
                if (row[ColName].ToString().ToLower().Trim() == ColValue.ToLower().Trim())
                {
                    return row[returnCol].ToString();
                }
            }
            return "";
        }

I have two suggestions:

Trim ColValue once rather than every iteration through the loop.
Use a case-insensitive string compare rather than performing .ToLower() on two different strings on every loop iteration.

Result:

private string FindInT(DataTable dt, string ColName, string ColValue, string returnCol)
{
    ColValue = ColValue.Trim();
    foreach (DataRow row in dt.Rows)
    {
        if (string.Equals(
            row[ColName].ToString().Trim(),
            ColValue,
            StringComparison.InvariantCultureIgnoreCase))
        {
            return row[returnCol].ToString();
        }
    }
    return "";
}


Randomized Lighting with Python I use this to automate the lights in my home while I am away at night (simulating presence).  The scheduling of when this runs is handled by my home automation software (Indigo Domotics).

I'm open to any/all critique, but am especially interested in whether doing the randomization within the Light object is the right approach (and if the method of randomizing is good/pythonic/efficient).  I'm a self taught programmer as a hobby only and am always trying to refine my thinking around code structure.

#!/usr/bin/python

from datetime import datetime
import random

# (min/max) seconds to delay before light turns on (randomized every time)
start_delay_range = (15, 70)
# (min/max) brightness (0-100, randomized every time)
brightness_range = (50, 85)
# (min/max) duration for light to stay on (randomized every time)
duration_range = (300, 900)

class Light():
    '''
    Define a light object.

    Attributes:
        name (str): Name of the light.
        device_id (int): The Indigo ID of the device.
        dimmable (bool): Set the light as dimmable or not.
    '''
    def __init__(self, name, device_id, dimmable):
        self.name = name
        self.device_id = device_id
        self.dimmable = dimmable

    def brightness(self):
        '''Return random brightness within <brightness_range>.'''
        return random.randint(brightness_range[0], brightness_range[1])

    def duration(self):
        '''Return random duration within <duration_range>.'''
        return random.randint(duration_range[0], duration_range[1])

    def start_delay(self):
        '''Return random delay within <start_delay_range>.'''
        return random.randint(start_delay_range[0], start_delay_range[1])

    def run(self):
        '''Turn on the light to <brightness> after <start_delay> for <duration>.'''
        if self.dimmable:
            # Turn on light
            indigo.dimmer.setBrightness(self.device_id,
                                        value = self.brightness(),
                                        delay = self.start_delay())
        else:
            # Turn on light
            indigo.device.turnOn(self.device_id,
                                 self.start_delay())

        # Turn off light after <delay>
        indigo.device.turnOff(self.device_id,
                              delay = self.duration())

# List of Light() objects to randomize
lights = [
    Light(name='Breakfast Room', device_id=1222428814, dimmable=True),
    Light(name='Kitchen Cabinet', device_id=18462733, dimmable=False),
    Light(name='Hallway', device_id=93680547, dimmable=True),
    Light(name='Living Room Recessed', device_id=7507220, dimmable=True),
    Light(name='Stairs', device_id=1842915774, dimmable=True),
    Light(name='TV Room', device_id=1569858222, dimmable=True)
]

def main():
    for light in lights:
        light.run()

if __name__ == '__main__':
    main()

Looking at the random number generation, a lot of duplicated code can be easily removed.

Starting at the 4th line: import random. The only function that is used is randint, so you could do from random import randint and then use randint everywhere, instead of random.randint.

Another repetition that can be avoided concern the arguments of randint: you can unpack the tuple using the *.

def start_delay(self):
    '''Return random delay within <start_delay_range>.'''
    return randint(*start_delay_range)

And your (repetitive) properties are now much shorter!


Migrating data using entity framework, linq and recursion I have a piece of code to migrate data from an old app to a new app. The data represents the user's websites.

In the old app the user only had one section for websites, on which you could add any website, even social links.

In the new app, the user can add personal websites, social links (only Facebook, Twitter and LinkedIn are considered social links) and what we call custom social networks (eg. Reddit, Meetup, Tumblr, etc)

This is the code:

public async Task ImportWebsites(int userId)
{
    string[] socialNetworksStr =
    {
        "linkedin.",
        "twitter.",
        "facebook.",
    };

    string[] customSocialNetworksStr =
    {   "deviantart.",
        "viadeo.",
        "reddit.",
        "xing.",
        "pinterest.",
        "tumblr.",
    };

    List<UserLinkDTO> personalWebsites = new List<UserLinkDTO>();
    List<UserSocialNetworkDTO> socialNetworks = new List<UserSocialNetworkDTO>();
    List<UserLinkDTO> customSocialNetworks = new List<UserLinkDTO>();
    List<UserLinkDTO> personalWebsitesRemoved = new List<UserLinkDTO>();

    using (OldContext ctx = new OldContext())
    {
        if (await (ctx.UserSettings.AnyAsync(u => u.UserId == userId)) == false)
            throw new RelatedEntityNotFoundException($"{userId} not be found!");

        personalWebsites = await ctx.PersonalWebsite.AsNoTracking()
                                    .Where(w => w.UserId == userId)
                                    .Select(w => new UserLinkDTO
                                    {
                                        UserId = w.UserId,
                                        Description = w.PersonalWebsiteDescription,
                                        Url = w.PersonalWebsiteURL,
                                        Name = w.PersonalWebsiteName
                                    }).ToListAsync().ConfigureAwait(false);
    }

    for (int i = 0; i < personalWebsites.Count; i++)
    {
        string url = personalWebsites[i].Url;
        if (!url.StartsWith("http://"))
        {
            personalWebsites[i].Url = "http://" + url;
        }
    }

    if (personalWebsites.Any())
    {
        foreach (var personalWebsite in personalWebsites)
        {
            foreach (var socialNetwork in socialNetworksStr)
            {
                if (personalWebsite.Url.Contains(socialNetwork))
                {
                    socialNetworks.Add(new UserSocialNetworkDTO() {
                        SocialNetworkId = (short)(Array.IndexOf(socialNetworksStr, socialNetwork) + 1),
                        Account = personalWebsite.Url
                    });
                    personalWebsitesRemoved.Add(personalWebsite);
                }
            }
            foreach (var customSocialNetwork in customSocialNetworksStr)
            {
                if (personalWebsite.Url.Contains(customSocialNetwork))
                {
                    customSocialNetworks.Add(personalWebsite);
                    personalWebsitesRemoved.Add(personalWebsite);
                }
            }
        }

        personalWebsites = personalWebsites.Except(personalWebsitesRemoved).ToList();
        var personalInfo = await PersonalInfoService.GetPersonalInfo(userId);

        personalInfo.Result.PersonalWebsites = personalWebsites;
        if(socialNetworks.Any()) personalInfo.Result.SocialNetworks = socialNetworks;
        if(customSocialNetworks.Any()) personalInfo.Result.CustomSocialNetworks = customSocialNetworks;

        var response = await PersonalInfoService.SavePersonalInfo(personalInfo.Result);
    }
}

    var response = await PersonalInfoService.SavePersonalInfo(personalInfo.Result);
}

My code does its job, but it takes some time, on those foreaches, and I think it's a little bit redundant, and I think it may be optimzed. How would you improve this?

PS I think I included all the necessary info, sorry If I forgot something.

The following code is O(n^2). More precisely, it's Theta(n^2) because it'll always be exactly n.

foreach (var personalWebsite in personalWebsites)
{
    // You can use a HashSet to eliminate the need for the inner loops, which
    // would reduce your computational complexity from Theta(n^2) to Theta(n) - see below
    foreach (var socialNetwork in socialNetworksStr)
    {
        // You could "convert" both of these to some kind of "normal form" (see below) to eliminate the need for the relatively inefficient "Contains" and enable the use of a HashSet
        if (personalWebsite.Url.Contains(socialNetwork))
        {
            socialNetworks.Add(new UserSocialNetworkDTO() { SocialNetworkId = (short)(Array.IndexOf(socialNetworksStr, socialNetwork) + 1), Account = personalWebsite.Url });
            personalWebsitesRemoved.Add(personalWebsite);

            // If you do stick with a loop here, presumably each personal web site only
            // matches one social media site, so you can break out of the loop here to
            // improve your computational complexity for this bit from Theta(n) to O(n)
        }
    }

    // Same comments as above
    foreach (var customSocialNetwork in customSocialNetworksStr)
    {
        if (personalWebsite.Url.Contains(customSocialNetwork))
        {
            customSocialNetworks.Add(personalWebsite);
            personalWebsitesRemoved.Add(personalWebsite);
        }
    }

You can reduce this to O(n) by reducing the URLs to some kind of "normal form" (e.g. http://www.linkedin.com/in/some.person would just be "normalized" to "linkedin.com") and building a Dictionary (hash table) or HashSet. This would reduce the computational complexity from Theta(n^2) to Theta(n).

Also, even if you do stick with the "inner" foreach loops, there's no need to iterate over all of the social network sites every single time; I assume that each personal web site can only match one social media site, so if you find one that it matches it reduces the computational complexity from Theta(n^2) to O(n^2).


AngularJS Translation Filter I'm very new to AngularJS. I'm using a directive for translation of category objects like:

{
"id":17,
"order":2,
"published":false,
"createdAt":"2014-12-29T16:22:43.000Z",
"updatedAt":"2014-12-29T16:23:22.000Z",
"CategoryTranslations":[
   {
      "id":13,
      "name":"Fishes",
      "description":"Fishes desc",
      "CategoryId":17,
      "LanguageId":1,
      "Language":{
         "id":1,
         "code":"en_EN",
         "name":"English"
      }
   }
]
 }

Filter:

app.filter('categoryTranslations', function() {
return function(categories, locale) {
    var translated = [];
    if (typeof categories != 'undefined') {
        for (var i = 0; i < categories.length; i++) {
            var category = categories[i];
            for (var j = 0; j < category.CategoryTranslations.length; j++) {
                var translation = category.CategoryTranslations[j];
                if (translation.Language.code == locale) {
                    category.defaultTranslation = translation;
                    translated.push(category);
                    break;
                }
            }
        }
    }
    return translated;
};
});

I really don't like the code and am looking for a better way to solve this problem.

I don't know much about angular.js myself, but I can give some small tips about your code.

Indentation

Code, in general, looks ugly without proper indentation.

I recommend that you indent the body of the second parameter passed into app.filter.

Why are you checking with 'undefined'?

On the line that goes:

if (typeof categories != 'undefined') {

Why are you comparing with 'undefined'?

If you want to check is categories was defined, you could just do

if (categories) {


Maximum contiguous sum in an array The following code is my solution for the following Daily Coding Challenge

  Given an array of numbers, find the maximum sum of any contiguous
  subarray of the array.

  For example, given the array [34, -50, 42, 14, -5, 86], the maximum
  sum would be 137, since we would take elements 42, 14, -5, and 86.

  Given the array [-5, -1, -8, -9], the maximum sum would be 0, since we
  would not take any elements.

  Do this in O(N) time.

I think this is done in O(N) time and is the best solution. If someone can think of a better way, I would be interested.

array = [4, -2, 7, -9]
running_sum = 0
for i in range(1,len(array)):
    if array[i-1] > 0:
        array[i] = array[i] + array[i-1]
    else:
        array[i-1] = 0
print(max(array))

I would merge the 2 loops (there is one in the max part), since not all parts are relevant for the maximum:

def best_subsum(array):
    running_sum = 0
    for i in range(1,len(array)):
        if array[i-1] > 0:
            array[i] = array[i] + array[i-1]
            if array[i] > running_sum: running_sum = array[i]
    return running_sum

array = [4, -2, 7, -9]
print(best_subsum(array))

Note that branch prediction shouldnt be too much of a problem, since running sum is not check until the same if statement in the next succeding iteration.


Prime number calculator I am starting to learn Ruby.

This is pretty much the first thing I have coded. It's very simple; it's a prime number calculator. Since this is my first Ruby code, I would like some review on the following:

Adherence to Ruby standards
Is it done in the Ruby way (the way a Rubyist would have coded it)?
Adherence to Ruby naming conventions

What I am not looking for is a review on the prime number algorithm. I know there are more efficient ones out there.

File: prime_numbers.rb

class PrimeNumber
    def is_prime_kata(number)
        if number == 1 then return false end

        max = Math.sqrt(number)

        (2..max).any? do |i|
            if number % i == 0 then return false end
        end

        true
    end
end

File: prime_numbers_test.rb

require 'test/unit'
require 'set'
require_relative 'prime_numbers.rb'

class PrimeNumberTest < Test::Unit::TestCase
    def test_is_prime
        primeNumbers = [2, 3, 5, 7, 11, 13, 17, 19, 23, 29, 31, 37, 41, 43, 47, 53, 59, 61, 67, 71, 73, 79, 83, 89, 97]

        primeNumber = PrimeNumber.new

        for i in primeNumbers
            result = primeNumber.is_prime_kata(i)
            assert_equal(true, result)
        end

        primeNumbersSet = Set.new(primeNumbers)
        allNumbersSet = Set.new(1..100)

        nonPrimeNumbersSet = allNumbersSet - primeNumbersSet

        for i in nonPrimeNumbersSet
            result = primeNumber.is_prime_kata(i)
            assert_equal(false, result)
        end
    end
end

Aiming at your questions, your naming conventions are good. However, in your test you tend to switch between camelCase and words_separated_by_underscores. I would suggest sticking to one or the other (we tend to use underscores). Your algorithm looks good and taking ANeves suggestions will make it look more "Ruby"ish.

However, I am going to suggest a much more efficient way of generating your prime numbers and none prime numbers in your Test. Instead of typing each prime number between 1 and 100 by hand into an Array, you could use the Array#Select method. It takes in an Enumerator, runs a given block against each item passed in, and returns an Array of only the items that return true.

prime_numbers = (1..100).select {|num| num.prime?}

This will given you the same Array you got by typing it all in manually. You can do the same for the none prime numbers.

non_prime_numbers = (1..100).select {|num| num unless num.prime?}

This'll return num only if it is not prime.

It'll save you a lot of typing and it will look more "Ruby"ish.


Exercism: clean and format a phone number in Clojure Problem statement

  Phone Number

  Write a program that cleans up user-entered phone numbers so that they can be sent SMS messages.

  The rules are as follows:

  If the phone number is less than 10 digits assume that it is bad number
  If the phone number is 10 digits assume that it is good
  If the phone number is 11 digits and the first number is 1, trim the 1 and use the last 10 digits
  If the phone number is 11 digits and the first number is not 1, then it is a bad number
  If the phone number is more than 11 digits assume that it is a bad number

Proposed Solution

(ns phone-number
  (:require [clojure.string :as str]))

(def non-digits #"\D")
(def parts #"^1?(...)(...)(....)$")
(def error-val ["000" "000" "0000"])

(defn- digits [n] (str/replace n non-digits ""))

(defn- trio-h [n]
  (if-let [ps (re-find parts n)]
    (rest ps)
    error-val))

(defn- trio [n] (-> n digits trio-h))

(defn number [n] (-> n trio str/join))

(defn area-code [n] (-> n trio first))

(defn pretty-print [n]
  (let [[area exch line] (trio n)]
    (format "(%s) %s-%s" area exch line)))

Questions

In pretty-print is the use of let necessary? Or is there a way in Clojure to splice in the result of (trio n) in-place for the call to format? The result of (trio n) is a vector containing three strings and the format call is expecting three %s strings to stick into the format string. If there is an alternative to my use of let, which is preferred?
Are the names ok? Do they read like Clojure names? Is there a convention for naming a one-off helper function?
Is it better to keep trio-h a separate helper function, or let it inside the using function, or would you just use the code in-place instead?
:refering clojure.string/replace triggers a warning (since it collides with clojure.core/replace) so I :refered all of clojure.string. Is there a way to :refer as to avoid the collision?
Do you have other improvements you could suggest?

Tests

See the Exercism page for the Phone Number problem in Clojure

I'm still learning clojure, but I'll contribute what I can.

I don't know if it is prefered or not, but if you wanted to get rid of the let, you could do something like (apply (partial format "(%s) %s-%s") trio) or (apply format (concat '["(%s) %s-%s"] trio)) instead.
Naming seems ok.
No comment.
I don't know.
Your regex for parts is suboptimal.  '.' will pick up any character, but you really only want digits.  Something like "^1?(\d{3})(\d{3})(\d{4})$" would be more accurate.


Summation Over Lines in a File (Advent of Code 2018 Day 1) Preface: I am learning Rust through the Advent of Code

The task is to read lines from a file, parse each as an integer, then provide a summation of all the numbers. My solution looks like this:

use std::fs::File;
use std::io::{self, prelude::*, BufReader};

fn main() -> io::Result<()> {
    let file = File::open("day01.txt")?;
    let reader = BufReader::new(file);

    let mut sum: i32 = 0;

    for line in reader.lines() {
      let value: i32 = line.unwrap().parse().unwrap();
      sum += value
    }

    println!("{}", sum);

    Ok(())
}

I'd like to use the iterator methods map and sum in my solution, but I haven't figured out how to do so with the type checker. Suggestions?

That's how you use Iterator::sum:

use std::fs::File;
use std::io::{self, prelude::*, BufReader};

fn main() -> io::Result<()> {
    let file = File::open("day01.txt")?;
    let reader = BufReader::new(file);
    let sum: i32 = reader
        .lines()
        .map(|line| line.unwrap().parse::<i32>().unwrap())
        .sum();

    println!("{}", sum);

    Ok(())
}

You must first say to what type the line must be parsed: parse::<i32>() and then, you must give the sum a type because the output of the Add trait is not the same as self and other.


Checking if a value is within a time range How can I improve the style to avoid so many float64()s?

func IsInTimeRange(tvalue int32,timeout int){
    t := time.Now().Unix()
    return int(math.Abs(float64(t)-float64(tvalue))) < timeout
}

For example,

package main

import (
    "fmt"
    "time"
)

func IsInTimeRange(tvalue int32, timeout int) bool {
    delta := int(time.Now().Unix() - int64(tvalue))
    return -timeout < delta && delta < timeout
}

func main() {}

tvalue int32 will overflow in year 2038, change it to tvalue int64.

package main

import "time"

func IsInTimeRange(tvalue int64, timeout int) bool {
    delta := int(time.Now().Unix() - tvalue)
    return -timeout < delta && delta < timeout
}

func main() {}


Passing multiple parameters to an SQL query I am trying to use this VBA code to pass a SQL stored procedure multiple values from an excel sheet.

In order to have the procedure run multiple times and insert multiple sets of information into the database, I copied and pasted the parameter section of the code each time I wanted to insert new data and hard coded which excel cell to pull data from.

Is there a way to consolidate the code so that I can reference cells A1:A10 for parameter 1, cells B1:B10 for parameter 2, cells C1:C10 for parameter 3 and so on without copying the code over again?

Sub Module2()
Dim cnSQL As ADODB.Connection
Dim sqlCommand As ADODB.Command
Dim prm As ADODB.Parameter

Set cnSQL = New ADODB.Connection
cnSQL.Open "Provider=SQLOLEDB.1; uid=test; Pwd=test; Initial Catalog = test; Data source=test"

Set sqlCommand = New ADODB.Command
sqlCommand.ActiveConnection = cnSQL
sqlCommand.CommandType = adCmdStoredProc
sqlCommand.CommandText = "EstimateInsertUpdate"

Set prm = sqlCommand.CreateParameter("CD", adVarChar, adParamInput, 10)
sqlCommand.Parameters.Append prm
sqlCommand.Parameters("CD").Value = Worksheets("Sheet1").Range("B5")

Set prm = sqlCommand.CreateParameter("Date", adDate, adParamInput)
sqlCommand.Parameters.Append prm
sqlCommand.Parameters("Date").Value = Worksheets("Sheet1").Range("F5")

Set prm = sqlCommand.CreateParameter("Test", adDecimal, adParamInput)
prm.Precision = 4
prm.NumericScale = 4
sqlCommand.Parameters.Append prm
sqlCommand.Parameters("Test").Value = Worksheets("Sheet1").Range("E5")

Set prm = sqlCommand.CreateParameter("EDtm", adDate, adParamInput)
sqlCommand.Parameters.Append prm
sqlCommand.Parameters("EDtm").Value = Worksheets("Sheet1").Range("D5")

Set prm = sqlCommand.CreateParameter("UserID", adVarChar, adParamInput, 10)
sqlCommand.Parameters.Append prm
sqlCommand.Parameters("UserID").Value = Worksheets("Sheet1").Range("G5")

sqlCommand.Execute

Set cnSQL = New ADODB.Connection
cnSQL.Open "Provider=SQLOLEDB.1; uid=test; Pwd=test; Initial Catalog = PMW; Data source=test"

Set sqlCommand = New ADODB.Command
sqlCommand.ActiveConnection = cnSQL
sqlCommand.CommandType = adCmdStoredProc
sqlCommand.CommandText = "EstimateInsertUpdate"

Set prm = sqlCommand.CreateParameter("CD", adVarChar, adParamInput, 10)
sqlCommand.Parameters.Append prm
sqlCommand.Parameters("CD").Value = Worksheets("Sheet1").Range("B6")

Set prm = sqlCommand.CreateParameter("Date", adDate, adParamInput)
sqlCommand.Parameters.Append prm
sqlCommand.Parameters("Date").Value = Worksheets("Sheet1").Range("F6")

Set prm = sqlCommand.CreateParameter("Test", adDecimal, adParamInput)
prm.Precision = 4
prm.NumericScale = 4
sqlCommand.Parameters.Append prm
sqlCommand.Parameters("Test").Value = Worksheets("Sheet1").Range("E6")

Set prm = sqlCommand.CreateParameter("EDtm", adDate, adParamInput)
sqlCommand.Parameters.Append prm
sqlCommand.Parameters("EDtm").Value = Worksheets("Sheet1").Range("D6")

Set prm = sqlCommand.CreateParameter("UserID", adVarChar, adParamInput, 10)
sqlCommand.Parameters.Append prm
sqlCommand.Parameters("UserID").Value = Worksheets("Sheet1").Range("G6")

sqlCommand.Execute

End Sub

Sub Module2 is a terrible name to use for a procedure. Not only it doesn't say anything about what it does, it looks like the [default] name of some standard code module.

Name things, it's important. Procedures do something, their names should start with a verb, and you should be able to read it and know roughly what it does just by the name.

Consider:

Public Sub InsertOrUpdateEstimates()

Also indent your code - it's harder to read code that begins in column 1 no matter what.

Instead of this:

Sub DoSomething()
DoSomeThings
DoSomeMoreThings
End Sub

Consider:

Sub DoSomething()
    DoSomeThings
    DoSomeMoreThings
End Sub

You're defining 5 parameters per command, but declare only 1 ADODB.Parameter object variable; this changes the meaning of the variable as the procedure executes, and this makes for bug-prone and confusing code. If a variable stands for "the UserID parameter", then it's reasonable to expect that variable to mean the same thing throughout the scope of the procedure. Same for the connection objects.

The problem is exactly that: the scope of the procedure is too wide - it's doing too many things.

You've essentially written a script - it goes top to bottom and executes an ordered sequence of operations and then completes.

You don't need to "consolidate" the code, you need to tear it apart and extract each responsibility into its own function, break it down into multiple, smaller, more specialized procedures.

You want to run a command for each row of a given range: you need a procedure that runs a command for a single row, and call it from another procedure.

The command takes 5 parameters; make the procedure receive 5 values as ...parameters:

Private Sub InsertOrUpdateEstimate(ByVal CD as String, ByVal theDate As Date, ByVal test As Single, ByVal EDtm As Date, ByVal UserID As String)

The body of that procedure will need to create an ADODB.Parameter object for each parameter value: instead of repeating the code for that 5 times, make another function that's responsible for creating an ADODB parameter - something like this:

Private Function CreateCommandParameter(ByVal name As String, ByVal value As Variant, Optional ByVal numPrecision As Integer = 4, Optional ByVal numScale As Integer = 4) As ADODB.Parameter

    Dim result As New ADODB.Parameter
    result.Direction = adParamInput
    result.name = name
    result.value = value

    Select Case VarType(value)
        Case VbVarType.vbBoolean
            result.Type = adBoolean

        Case VbVarType.vbDate
            result.Type = adDate

        Case VbVarType.vbCurrency
            result.Type = adCurrency
            result.Precision = numPrecision
            result.NumericScale = numScale

        Case VbVarType.vbDouble
            result.Type = adDouble
            result.Precision = numPrecision
            result.NumericScale = numScale

        Case VbVarType.vbSingle
            result.Type = adSingle
            result.Precision = numPrecision
            result.NumericScale = numScale

        Case VbVarType.vbByte, VbVarType.vbInteger, VbVarType.vbLong
            result.Type = adInteger

        Case VbVarType.vbString
            result.Type = adVarChar

        Case Else
            Err.Raise 5, Description:="Data type not supported"
    End Select

    Set CreateCommandParameter = result
End Function

Now, InsertOrUpdateEstimate can look like this:

Private Sub InsertOrUpdateEstimate(ByVal conn As ADODB.Connection, ByVal CD As String, ByVal theDate As Date, ByVal test As Single, ByVal EDtm As Date, ByVal UserID As String)
    With New ADODB.Command
        .ActiveConnection = conn
        .CommandType = adCmdStoredProc
        .CommandText = "EstimateInsertUpdate"
        .Parameters.Append CreateCommandParameter("CD", CD)
        .Parameters.Append CreateCommandParameter("Date", theDate)
        .Parameters.Append CreateCommandParameter("Test", test)
        .Parameters.Append CreateCommandParameter("EDtm", EDtm)
        .Parameters.Append CreateCommandParameter("UserID", UserID)
        .Execute
    End With
End Sub

Notice it's receiving the ADODB.Connection as a parameter, so it's responsible for executing a command, but not for setting up the connection.

So all that's left to do is to call that procedure inside the body of some loop that iterates all the rows we want to look at.

But there's more to consider: what if an error occurs because some of the data in row 7 is of the wrong type? What if the connection times out?

It would be a good idea to initiate a transaction before we start looping, and only commit the transaction once all rows have been correctly processed - and if any error happens, we can rollback the entire transaction, fix things and start over.

Think of a transaction as a bank transaction: you're moving dollars from one account to the other - if something wrong happens in the middle of the transaction, you don't want the money to be withdrawn from the source account but not deposited into the target account! A transaction either runs completely, or not at all: wrapping all these insert/update commands in a transaction is probably a good idea.

To do this correctly you need to handle runtime errors. Consider this procedure:

Public Sub InsertOrUpdateEstimates()
    Const connString As String = "..."
    Const fromRow As Long = 1
    Const toRow As Long = 10

    Dim CD As String
    Dim theDate As Date
    Dim test As Single
    Dim EDtm As Date
    Dim UserID As String

    On Error GoTo CleanFail

    Dim conn As ADODB.Connection
    Set conn = New ADODB.Connection
    conn.Open connString

    conn.BeginTrans

    Dim currentRow As Long
    With Sheet1
        For currentRow = fromRow To toRow
            CD = .Cells(currentRow, 2).value
            EDtm = .Cells(currentRow, 4).value
            test = .Cells(currentRow, 5).value
            theDate = .Cells(currentRow, 6).value
            UserID = .Cells(currentRow, 7).value
            InsertOrUpdateEstimate conn, CD, theDate, test, EDtm, UserID
        Next
    End With

    conn.CommitTrans

CleanExit:
    conn.Close
    Exit Sub

CleanFail:
    conn.RollbackTrans
    MsgBox Err.Description & vbNewLine & "Transaction was rolled back, no changes were made", vbExclamation
    Resume CleanExit

End Sub

Notice the Sheet1 object is being referred to directly instead of being fetched from the Worksheets collection everytime it's needed; the With block removes all redundancies here. Also we're reading the values using the Cells function instead of Range, so we can easily supply a row/column number to pick up a single value.

You can push the segregation of responsibilities to the next level by moving the whole ADODB-handling code to its own class - see materializing any ADODB query and creating ADODB parameters on the fly for more information; the For loop above could look something like this (and that replaces the helper CreateCommandParameter function and CreateCommandParameter procedure, too):

Const sql As String = "exec dbo.EstimateInsertUpdate ?,?,?,?,?;"
conn.BeginTrans

With Sheet1
    For currentRow = fromRow To toRow
        SqlCommand.ExecuteNonQuery conn, sql, _
            .Cells(currentRow,2).value, _
            .Cells(currentRow,4).value, _
            .Cells(currentRow,5).value, _
            .Cells(currentRow,6).value, _
            .Cells(currentRow,7).value
    Next
End With

conn.CommitTrans

The fewer things a procedure does, the clearer the code becomes; there shouldn't ever be a reason to copy+paste code - if you find yourself pressing Ctrl+C, ask yourself "is there not a better way?" before you press Ctrl+V.