Looping through an array of strings containing approximately 3 million elements I have a function that loops through an array of strings containing approximately 3 million lines of strings.  My problem is this loop is running too slowly and I want to know how to make it faster.

string[] words = { "Hello", "world", "!", "My", "name", "is", "Something", "."};
    List<MyClass> result = new List<MyClass>();

    for (int i = 0; i < words.Length; i++)
    {
        if (words[i] == "&&&")
        { }
        else if (words.Length > i + 2 && FileContains(words[i] + " " + words[i + 1] + " " + words[i + 2]))
        {
            result.Add(new MyClass(words[i] + " " + words[i + 1] + " " + words[i + 2]));
            words[i + 1] = "&&&";
            words[i + 2] = "&&&";
        }
        else if (words.Length > i + 1 && FileContains(words[i] + " " + words[i + 1]))
        {
            result.Add(new MyClass(words[i] + " " + words[i + 1]));
            words[i + 1] = "&&&";
        }
        else
            result.Add(new MyClass(words[i], Parent));

    }

And my FileContains function which loops through the array of strings:

public static bool FileContains(string Text)
{
    foreach (string line in ARRAYOFWORDS)  // Approximately 3 million strings
    {
        if (Text.ToLower() == line.Substring(0, line.LastIndexOf('|')))
        {
            return true;
        }
    }
    return false;
}

Well you can optimize FileContains very simply - stop calling Text.ToLower() on every iteration!

In fact, at that point it's really simple to rewrite. Here's the code for the original question, which didn't have the Substring part:

public static bool FileContains(string text)
{
    // Variable name changed to save my eyes from the horror.
    return lines.Contains(text.ToLower());
}

If you store the lines in a HashSet<string> instead of an array or a list, it will be blazingly fast.

With the Substring part, I'd create a new HashSet<string> with just those substrings, assuming you want to look up more than once:

HashSet<string> lineStarts;

...

lineStarts = new HashSet<string>(lines.Select(x => x.Substring(0, x.IndexOf('|'));

Then use lineStarts within FileContains.

Note that using ToLower is a fairly crude approach to case sensitivity. You should think carefully about what this needs to do in terms of culture-sensitivity etc. See the relevant MSDN page for lots more information. (Ain't culture fun?)


Improving a Medicare Beneficiary Identifier (MBI) generator I am new to coding and looking for a few pointers on how I can improve my first project.
At work, I often need to create Medicare Beneficiary Identifiers (MBI) when creating test patients with Medicare coverage, and have to look up the format every time. I thought creating an MBI generator would be a great first project.
Doing a quick search, I found a similar project on this site (which is what lead me here), but the questioner wants to create 10,000 records, whereas I may only need one or two MBIs at a time (their question can be found here: Sequential MBI generator). Being new and not understanding all of the code, I was nervous to follow in their path and end up with a ton of records, so I actually followed an example of a random password generator and tweaked it to meet my needs. As the outcome is vastly different from the linked example and seems verbose, I wondered if anyone with more experience would be able to give me some pointers to get me off to improve my work.
#MBI is 11 characters in the following format
# 1 - 2 - 3 - 4 - 5 - 6 - 7 - 8 - 9 - 10 - 11
# C - A - AN- N - A - AN- N - A - A - N  - N
# C = Numeric 1 - 9
# N = Numeric 0 - 9
# A = Alphabetic A...Z; Not S, L, O, I, B, Z
# AN = Either A or N

import random
letter = 'ACDEFGHJKMNPQRTUVWXY' # not = B, I, L, O, S, Z
digit = str('0123456789')
partdig = str('123456789')
dig_let = digit + letter

while 1:
     mbi_return = 1
     mbi_need = int(input("How many MBI numbers do you need to generate?: "))
      for x in range(0,mbi_need):
         mbi = ""
         for x in range(0, mbi_return):
            mbi_char0 = random.choice(partdig)
            mbi_char1 = random.choice(letter)
            mbi_char2 = random.choice(dig_let)
            mbi_char3 = random.choice(digit)
            mbi_char4 = random.choice(letter)
            mbi_char5 = random.choice(dig_let)
            mbi_char6 = random.choice(digit)
            mbi_char7 = random.choice(letter)
            mbi_char8 = random.choice(letter)
            mbi_char9 = random.choice(digit)
            mbi_char10 = random.choice(digit)
            mbi = (mbi_char0 + mbi_char1 + mbi_char2 + mbi_char3 + mbi_char4 + mbi_char5 +
            mbi_char6 + mbi_char7 + mbi_char8 + mbi_char9 + mbi_char10) # I imagine this could be much cleaner
            print(mbi)

Thanks in advance!

Instead of while 1: please just use while True. while 1: is a throwback from old versions of C that didn't have stdbool.h. while True: is much most explicit a about what your intent is.

At the top you have
letter = 'ACDEFGHJKMNPQRTUVWXY' # not = B, I, L, O, S, Z
digit = str('0123456789')
partdig = str('123456789')
dig_let = digit + letter

Python actually has built-ins for these:
from string import digits, ascii_uppercase

# letter is the set of ascii characters minus B, I, L. . .
letter = "".join(set(ascii_uppercase) - {'B', 'I', 'L', 'O', 'S', 'Z'})
# Just use digits instead of digit
partdig = digits[1:]  # Remove the first digit
dig_let = digits + letter

That saves you from needing to type out each of the letters to include.
Also note, even if string.digits didn't exist, you could have also defined digit as:
>>> digit = "".join(str(n) for n in range(0, 10))
>>> digit
'0123456789'

Also, all variable names here should be lowercase, separated by underscores. partdig should be part_dig, or part_digits, or even better: non_zero_digits.
I also think letter should be letters, since it's a collection of letters. It's a small change, but it lets your readers know that it's multiple letters, not just a single one.

Watch your indentation:
mbi_need = int(input("How many MBI numbers do you need to generate?: "))
 for x in range(0,mbi_need):

Especially in Python that matters a lot. If that was just a pasting error, it's a good idea to look over the code before posting just to double check that errors weren't introduced accidentally.
You're using a odd 5-space indentation in the loop though, which is part of the problem. Please use 4-space indentation.

for x in range(0,mbi_need):
    mbi = ""
    for x in range(0, mbi_return):

Both loops define a x variable! Since you never use x in either loop though, use _ instead:
for _ in range(0,mbi_need):
    mbi = ""
    for _ in range(0, mbi_return):

_ is a convention that says "I needed to create a name, but don't need the variable", which is the case here.

range(0, mbi_need)

0 is the implicit start; it's not necessary to specify it if you only otherwise need to specify the ending number. Just write:
range(mbi_need)

mbi_char0 = random.choice(partdig)
mbi_char1 = random.choice(letter)
mbi_char2 = random.choice(dig_let)
mbi_char3 = random.choice(digits)
. . .

Whenever you find yourself creating many similar variables, and you're differentiating them by putting numbers in the name, stop! You should likely be using a list instead. Because the make-up of the MBIs doesn't follow an easy pattern, fixing this isn't super straightforward, but it's still possible. First, I'd create a list holding the order of partdig, letter, dig_let, digits. . . which will define the order of the different character types:
mbi_pattern = [non_zero_digits, letters, digit_letters, digits,
               letters, digit_letters, digits, letters,
               letters, digits, digits]

This looks ugly, but it will clean up the code later. Sometimes all you can do is move the ugly bulk to the side. Once you've defined that list, creating a MBI is trivial and tiny:
mbi = "".join(random.choice(part) for part in mbi_pattern)
print(mbi)

Get each of the part sets, generate a random character from each of them, then join them into a string.

Your inner
for _ in range(mbi_return):

loop doesn't appear to be doing anything. It seems like it's doing a similar job as the other loop, except it will always be range(1), which will only run once, which means it isn't really a loop. I got rid of it because it isn't doing anything except complicating the code.
The same can be said about the while True as well. You want to generate 10000 codes, repeatedly, forever? The while True loop will never end since you never break from it. I also got rid of it because it is also complicating the code without good reason.

 mbi = ""

This isn't necessary. Even if you needed mbi in the outer scope, loops in Python don't create scopes like they do in other languages. mbi "defined" inside the loop can be accessed from outside of the loop.

In your remaining loop, you're creating an mbi, then just printing it. That doesn't allow you to do anything with the data though, like save it to file. It would be much cleaner to store the generated MBIs in a list so that they can potentially be used later. If you do that, your loop can be made into a list comprehension:
mbis = ["".join(random.choice(part) for part in mbi_pattern)
        for _ in range(mbi_need)]

Each MBI is generated using the same generator expression as before, but now it's wrapped in a list comprehension to generate multiple.

In the end, this is what I'm left with:
import random
from string import digits, ascii_uppercase

letters = "".join(set(ascii_uppercase) - {'B', 'I', 'L', 'O', 'S', 'Z'})
non_zero_digits = digits[1:]
digit_letters = digits + letters

mbi_pattern = [non_zero_digits, letters, digit_letters, digits,
               letters, digit_letters, digits, letters,
               letters, digits, digits]

mbi_need = int(input("How many MBI numbers do you need to generate?: "))

mbis = ["".join(random.choice(part) for part in mbi_pattern)
        for _ in range(mbi_need)]

print("\n".join(mbis))

And its usage:
How many MBI numbers do you need to generate?: >? 10
5V70VK4JP28
8Y12N77RC51
9JM2JN8RQ38
3X08DH7FH95
3MH6Y49KU87
6N70AC7MW75
9A67A62TU38
4A48C94QT38
2NP7TY0DC65
1GP8A57JQ27

As mentioned in the comment though, really, code should be tucked into functions. In larger programs, that eases testing and comprehension of your code.
Here, you could have a function that generates a single MBI, then use it to generate a list of them. I also always have a main function that ties the whole program together so I can control the execution of the code easier.
I also realized after I had my coffee that all the variables at the top are really constants, so they should be in UPPER_SNAKE_CASE.
After those changes, I'm left with:
import random
from string import digits, ascii_uppercase

LETTERS = "".join(set(ascii_uppercase) - {'B', 'I', 'L', 'O', 'S', 'Z'})
NON_ZERO_DIGITS = digits[1:]
DIGIT_LETTERS = digits + LETTERS

MBI_PATTERN = [NON_ZERO_DIGITS, LETTERS, DIGIT_LETTERS, digits,
               LETTERS, DIGIT_LETTERS, digits, LETTERS,
               LETTERS, digits, digits]

def generate_mbi():
    return "".join(random.choice(part) for part in MBI_PATTERN)

def main():
    mbi_need = int(input("How many MBI numbers do you need to generate?: "))
    mbis = [generate_mbi() for _ in range(mbi_need)]
    print("\n".join(mbis))

if __name__ == '__main__':
    main()


DisposableObject base class for C# I commonly run into the need to implement IDisposable in my code. To correctly dispose of both managed and unmanaged resources requires a reasonable amount of boilerplate code. Based on the documentation found here I have created the following base class.

My intent is that any object I create that needs to dispose of resources doesn't need to rewrite the boilerplate code. Rather it can inherit from this object and implement two abstract methods.

Does this appear correct? And has anyone else written something similar? I would appreciate comments on the style, correctness and how well this conforms to best practices.

public abstract class DisposableObject : IDisposable
{
    private bool _disposed = false;

    public bool Disposed
    {
        get { return _disposed; }
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    ~DisposableObject()
    {
        Dispose(false);
    }

    private void Dispose(bool disposing)
    {
        if (!_disposed)
        {
            if (disposing)
            {
                DisposeManagedResources();
            }

            DisposeUnmanagedResources();

            _disposed = true;
        }
    }

    protected abstract void DisposeManagedResources();
    protected abstract void DisposeUnmanagedResources();
}

EDIT: Final Implementation

public abstract class DisposableObject : IDisposable
{
    public bool Disposed { get; private set;}

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    ~DisposableObject()
    {
        Debug.Assert(Disposed, "WARNING: Object finalized without being disposed!");
        Dispose(false);
    }

    private void Dispose(bool disposing)
    {
        if (!Disposed)
        {
            if (disposing)
            {
                DisposeManagedResources();
            }

            DisposeUnmanagedResources();
            Disposed = true;
        }
    }

    protected virtual void DisposeManagedResources() { }
    protected virtual void DisposeUnmanagedResources() { }
}

I would think on making two changes:

Make one or both Dispose*Resources methods virtual instead of abstract. Though it highly depends on how often do you need to handle unmanaged resources. I can't remember last time I was handling them and I would hate overriding this method in each class just to make it empty.
I would add some logging either to your finalizer method or to Dispose(bool disposing) method in order to catch situations when disposable object wasn't disposed correctly by calling Dispose() method. Most developers are looking for such information and you have a good place to inject it.


Convert a string in time format to minutes I am new to C#. I just created a method where I can convert a string with the format HH:MM AM/PM to minutes. I was wondering if there is a better or more effective way to achieve the same result.

static void Main(string[] args)
    {

        string time1 = "11:15 AM";
        string time2 = "11:15 PM";

        calculateTimeInMinutes(time1);
        calculateTimeInMinutes(time2);

        void calculateTimeInMinutes(string currentTime)
        {
            if (currentTime.Contains("PM"))
            {
                int hours = Convert.ToInt32(currentTime.Substring(0, 2));
                int HoursInminutes = (12 + (hours % 60)) * 60;
                string minutesInString = currentTime.Split(':')[1];
                int minutes = Convert.ToInt32(minutesInString.Remove(2));
                int totalMinutes = HoursInminutes + minutes;
                Console.WriteLine(totalMinutes);
            }
            else
            {
                int hours = Convert.ToInt32(currentTime.Substring(0, 2));
                int HoursInminutes = (hours % 60) * 60;
                string minutesInString = currentTime.Split(':')[1];
                int minutes = Convert.ToInt32(minutesInString.Remove(2));
                int totalMinutes = HoursInminutes + minutes;
                Console.WriteLine(totalMinutes);
            }
        }

        Console.ReadKey();
    }
}

The code is easy to understand, and it seems to produce the correct result.

A couple of things:

1)
The code for "AM" and "PM" are almost identical with the difference of 12 (the PM-addend). You should not repeat yourself.

2) You "split" the string three times: one for AM/PM, one for hours, and one for minutes. Instead you could use string[] parts = input.Split(':', ' '); to split to the useful parts in one operation.

3) You should maybe check the input for correct format and throw some meaningful exceptions in case of wrong format.

All in all my reviewed code would be something like:

int CalculateTimeInMinutes(string input)
{
  if (string.IsNullOrWhiteSpace(input))
    throw new ArgumentNullException(nameof(input));

  string[] parts = input.Split(':', ' ');

  if (parts.Length != 3)
    throw new ArgumentOutOfRangeException(nameof(input));

  if (parts[2].ToUpper() != "AM" && parts[2].ToUpper() != "PM")
    throw new ArgumentOutOfRangeException(nameof(input), "Missing AM/PM qualifier");

  if (!int.TryParse(parts[0], out int hours))
  {
    throw new ArgumentOutOfRangeException(nameof(input), "Invalid hours part.");
  }

  if (hours < 0 || hours > 12)
    throw new ArgumentOutOfRangeException(nameof(input), "Hours must be between 00 and 12 (inclusive)");

  // TODO: The same checks for minutes.
  int.TryParse(parts[1], out int minutes);

  int toPM = parts[2].ToUpper() == "PM" ? 12 : 0;
  int hoursInminutes = (toPM + (hours % 60)) * 60;
  int totalMinutes = hoursInminutes + minutes;
  return totalMinutes;
}

That said, there are well known objects and apis that can do the same operation in a few lines of codes:

int CalculateTimeInMinutes(string input)
{
  DateTime time = DateTime.Parse(input, new CultureInfo("en-US"));
  return (int)(time - time.Date).TotalMinutes;
}


PHP grouping based on a value I'm asking in arbitrary sense to avoid going into any unwanted specifics.

Suppose we're to put balls in baskets.
If the there are <6 balls, they don't get to go in any basket.
If there are between 6-12 balls they take up 1 basket.
But if the balls are >12, they take up 2 baskets.

Following is WORKING FINE:

if (count($balls)<=5) {
    $baskets = 0;
} else if (count($balls) > 5 && count($balls) <12) {
    $baskets = 1;
} else {
    $answer = ceil(count($balls)/12);
}

Question: Is there a better, smarter, and/or quicker way to approach the problem, may be through loops or something?

The middle else if statement is redundent and can be removed. The reason that is redundent is that the only two cases are either equal or less than 5 balls gives 0 baskets and for everything else the number of baskets can be evaluated to be ceil(count($balls)/12).

Also using short hand will make it more readable I find.

$baskets = count($balls) <=5 ? 0 : ceil(count($balls)/12);


Reflection of an object in a 3D world The following is a excerpt from a class that is supposed to represent a reflection of an object in a 3D world, like in a mirror.

After I started I discovered that mirroring is a property of an object, it is either mirrored, or it isn't. Multiple mirrors don't come up with unique objects but just keep flipping from one to the other, with differing rotations.

So with that in mind, how can I re-factor this class, to more suitably deal with the problem?

package au.id.rleach.efficientmultiblocks;

import org.spongepowered.api.util.Axis;

import java.util.EnumMap;
import java.util.LinkedHashSet;
import java.util.Set;

public class Reflexion {
    private final EnumMap<Axis, Boolean> map = new EnumMap<Axis,Boolean>(Axis.class);

    public Reflexion() {
        for(Axis x:Axis.values()){
            map.put(x, false);
        }
    }

    public Reflexion(Axis...axises){
        this();
        for(Axis x : axises){
            map.put(x,true);
        }
    }

    public Reflexion(Reflexion r, boolean x, boolean y, boolean z){
        //^ is xor
        map.put(Axis.X, x ^ r.map.get(Axis.X));
        map.put(Axis.Y, y ^ r.map.get(Axis.Y));
        map.put(Axis.Z, z ^ r.map.get(Axis.Z));
    }

    int getFlip(Axis x){
        return map.get(x) ? -1 : 1;
    }

    static Set<Reflexion> all(){
        return all(new Reflexion());
    }

    static Set<Reflexion> all(Reflexion r){
        Set<Reflexion> out = new LinkedHashSet<Reflexion>();
        //Loop over all values for each axis.
        boolean[] tf = {true, false};
        for(boolean x : tf){
            for(boolean y : tf){
                for(boolean z : tf){
                    out.add(new Reflexion(r, x, y, z));
                }
            }
        }
        return out;
    }
}

From what I understand, you want to make sure that new objects are not created when they are identical to what has been created before.

This is relatively easy, and would include either having a static map or a map inside another object, such as a ReflectionFactory. I'd personally go with ReflectionFactory as I don't like static things when you can avoid them.

Here's an example factory:

public class ReflexionFactory {
    private final Map<Integer, Reflexion> map = new HashMap<>();

    public ReflexionFactory() {
       for (boolean x : tf) {
            for (boolean y : tf) {
                for (boolean z : tf) {
                    Reflexion refl = new Reflexion(this, x, y, z);
                    map.put(refl.getKey(), refl);
                }
            }
        }
    }

    private Reflexion getReflection(int value) {
        return map.get(value);
    }

    Reflexion get(boolean x, boolean y, boolean z) {
        int value = 0;
        if (x) value += 1;
        if (y) value += 2;
        if (z) value += 4;
        return get(value);
    }

    public Set<Reflexion> all(Reflexion r) {
        ...
        for (boolean x : tf) {
            for (boolean y : tf) {
                for (boolean z : tf) {
                    out.add(this.get(x, y, z));
                }
            }
        }
        return out;
    }
}

Some changes in the Reflexion class:

public class Reflexion {

    Reflexion(ReflexionFactory factory, boolean x, boolean y, boolean z) {
        this.factory = factory;
        ...
    }

    public int getKey() {
        int value = 0;
        if (isFlippedX()) value += 1;
        if (isFlippedY()) value += 2;
        if (isFlippedZ()) value += 4;
        return value;
    }
}

There are a few methods I've left up to you to implement, but I hope you get the idea of them.

So what is the most important change?

The idea is to keep a map of all created reflections, stored with an id, and when you previously created the new reflection, you look them up through the factory (which contains the map).

Essentially, this creates what would be called a "Multiton". A class which has a limited amount of instances. (Compared to a "singleton" which only has one instance)


Different versions of the Operate method I have code that basically is the same, but depending on the number of arguments you provide (up to 5), it will operate with them in a very repetitive way. I have the feeling that this can be optimized in some way, since maintaining and creating all the overloads by hand is a tedious and error-prone task.
The code is 5 versions (overloads) of the Operate method. As you can see, it increasingly deal with 2, 3, 4 and 5 arguments (each one is of a given type), so it can adapt from 2 to 5 type arguments.
public static Either<ErrorList, TResult> Operate<T1, T2, TResult>(Either<ErrorList, T1> a, Either<ErrorList, T2> b,
    Func<T1, T2, Either<ErrorList, TResult>> onSuccess)
{
    var success = from av in a.RightValue
                  from bv in b.RightValue
                  select onSuccess(av, bv);
    var errorList = new[]
        {
            a.LeftValue,
            b.LeftValue,
        }
        .SelectMany(x => x.ToEnumerable())
        .Aggregate((x, y) => new ErrorList(x.Concat(y)));

    return success.ValueOr(() => errorList);
}

public static Either<ErrorList, TResult> Operate<T1, T2, T3, TResult>(Either<ErrorList, T1> a,
    Either<ErrorList, T2> b, Either<ErrorList, T3> c,
    Func<T1, T2, T3, Either<ErrorList, TResult>> onSuccess)
{
    var success =
        from av in a.RightValue
        from bv in b.RightValue
        from cv in c.RightValue
        select onSuccess(av, bv, cv);

    var errorList = new[]
        {
            a.LeftValue,
            b.LeftValue,
            c.LeftValue,
        }
        .SelectMany(x => x.ToEnumerable())
        .Aggregate((x, y) => new ErrorList(x.Concat(y)));

    return success.ValueOr(() => errorList);
}

public static Either<ErrorList, TResult> Operate<T1, T2, T3, T4, TResult>(Either<ErrorList, T1> a, Either<ErrorList, T2> b, Either<ErrorList, T3> c, Either<ErrorList, T4> d,
    Func<T1, T2, T3, T4, Either<ErrorList, TResult>> onSuccess)
{
    var success = from av in a.RightValue
                  from bv in b.RightValue
                  from cv in c.RightValue
                  from dv in d.RightValue
                  select onSuccess(av, bv, cv, dv);

    var errorList = new[]
        {
            a.LeftValue,
            b.LeftValue,
            c.LeftValue,
            d.LeftValue,
        }
        .SelectMany(x => x.ToEnumerable())
        .Aggregate((x, y) => new ErrorList(x.Concat(y)));

    return success.ValueOr(() => errorList);
}

public static Either<ErrorList, TResult> Operate<T1, T2, T3, T4, T5, TResult>(
    Either<ErrorList, T1> a,
    Either<ErrorList, T2> b,
    Either<ErrorList, T3> c,
    Either<ErrorList, T4> d,
    Either<ErrorList, T5> e,
    Func<T1, T2, T3, T4, T5, Either<ErrorList, TResult>> onSuccess)
{
    var success = from av in a.RightValue
        from bv in b.RightValue
        from cv in c.RightValue
        from dv in d.RightValue
        from ev in e.RightValue
        select onSuccess(av, bv, cv, dv, ev);

    var errorList = new[]
        {
            a.LeftValue,
            b.LeftValue,
            c.LeftValue,
            d.LeftValue,
            e.LeftValue,
        }
        .SelectMany(x => x.ToEnumerable())
        .Aggregate((x, y) => new ErrorList(x.Concat(y)));

    return success.ValueOr(() => errorList);
}

Is there a better way to do this without repeating almost the same code? Imagine that I would like to create another overload for 6, 7, 8... arguments. It can be really tricky and sub-optimal.

You can try to extract the common piece of code into a helper method:
private static Either<ErrorList, TResult> Operate<TResult>(ErrorList[] errorLists, Either<ErrorList, TResult> success)
{
    var errorList = errorLists
        .SelectMany(x => x.ToEnumerable())
        .Aggregate((x, y) => new ErrorList(x.Concat(y)));

    return success.ValueOr(() => errorList);
}

Then you can reduce the overloads' body only to calculate the helpers inputs:
public static Either<ErrorList, TResult> Operate<T1, T2, T3, T4, T5, TResult>(
    Either<ErrorList, T1> a, Either<ErrorList, T2> b, Either<ErrorList, T3> c, Either<ErrorList, T4> d, Either<ErrorList, T5> e,
    Func<T1, T2, T3, T4, T5, Either<ErrorList, TResult>> onSuccess)
  => Operate<TResult>(
    new [] { a.LeftValue, b.LeftValue, c.LeftValue, d.LeftValue, e.LeftValue },
    onSuccess(a.RightValue, b.RightValue, c.RightValue, d.RightValue, e.RightValue));


Vector addition and Pythagorean Theorem code I would like to share my code that is meant to help me add 2 vectors together. It is very simple and I would like to kindly ask you whether you could review my code. Also, I decided to keep separate variable for angle in degrees % 90. Do you think that was a good idea?

import math

def pythagorean_theorem(a,b, c=None):
    if (None is not a and None is not b) and None is c:
        return math.sqrt(float(a) ** 2 + float(b) ** 2)
    elif (a is None or b is None) and c is not None:
        if a is None:
            return math.sqrt(float(c)**2 - float(b)**2)
        else:
            return math.sqrt(float(c)**2 - float(a)**2)
    else:
        return -1

def find_x_component(vector1):
    alpha = math.radians(vector1[1] % 90)

    if vector1[1] > 0 and vector1[1] < 90:
        return (math.cos(alpha)) * vector1[0]
    elif vector1[1] > 90 and vector1[1] < 180:
        return 0 - ((math.sin(alpha)) * vector1[0])
    elif vector1[1] > 180 and vector1[1] < 270:
        return 0 - ((math.cos(alpha)) * vector1[0])
    elif vector1[1] > 270 and vector1[1] < 360:
        return (math.sin(alpha)) * vector1[0]
    elif vector1[1] == 180:
        return 0 - vector1[0]
    elif vector1[1] == 0:
        return vector1[0]
    else:
        return 0

def find_y_component(vector1):
    alpha = math.radians(vector1[1] % 90)

    if vector1[1] > 0 and vector1[1] < 90:
        return (math.sin(alpha)) * vector1[0]
    elif vector1[1] > 90 and vector1[1] < 180:
        return (math.cos(alpha)) * vector1[0]
    elif vector1[1] > 180 and vector1[1] < 270:
        return 0 - ((math.sin(alpha)) * vector1[0])
    elif vector1[1] > 270 and vector1[1] < 360:
        return 0 - (math.cos(alpha)) * vector1[0]
    elif vector1[1] == 270:
        return 0 - vector1[0]
    elif vector1[1] == 90:
        return vector1[0]
    else:
        return 0

def vector_addition(vector1, vector2):

    # Calculating X and Y components
    vector1X = find_x_component(vector1)
    vector1Y = find_y_component(vector1)

    vector2X = find_x_component(vector2)
    vector2Y = find_y_component(vector2)

    return [vector1X + vector2X, vector1Y + vector2Y, pythagorean_theorem(vector1X + vector2X,vector1Y + vector2Y)]

vectorA = [2, 45]
vectorB = [6, 73.5]
print(f'Coordinates of the endpoint of your vector are {vector_addition(vectorA, vectorB)[0:2]} and its magnitude is {vector_addition(vectorA, vectorB)[2]}.')

This is actually a good place for a class. It allows you to define methods on the vector object itself and it also allows you to document your data format (it took me a bit to figure out that you initialize your vector as [magnitude, angle_from_x_axis_in_degrees]. It also means that you don't need to repeat yourself quite so often. Internally I would store the vector as its x and y components, since that is usually more useful (especially when adding two vectors):

from math import sqrt, cos, sin, radians

class Vector2D:
    """A 2D vector that supports addition, calculation of magnitude
       and initialization from (magnitude, angle)."""

    def __init__(self, x, y):
        """Standard constructor with x and y components."""
        self.x = x
        self.y = y

    @classmethod
    def from_mag_and_angle(cls, magnitude, angle):
        """Alternative constructor from magnitude and angle from x-axis in degrees."""
        x = magnitude * cos(radians(angle))
        y = magnitude * sin(radians(angle))
        return cls(x, y)

    def __add__(self, other):
        """v1 + v2 if both are 2D vectors"""
        if not isinstance(other, self.__class__):
            return NotImplemented
        return self.__class__(self.x + other.x, self.y + other.y)

    @property
    def magnitude(self):
        """Magnitude/length of vector"""
        return sqrt(self.x**2 + self.y**2)

    def __str__(self):
        """Readable representation of vector as list"""
        return f"[{self.x}, {self.y}]"

This uses two differently decorated methods. @classmethod is basically a way to define alternative constructors (here instead of giving the components we give the magnitude and angle). @property allows a method to be accessed like an attribute, so we can write v.magnitude, instead of v.magnitude(). This way we do not need to update the magnitude if we change the x or y components, it is calculated whenever we access the attribute.

Finally, getting the x and y components does not need all your special cases. The formula is valid in all four quadrants. Only when doing the opposite (getting the angle) do you need to special case (actually you don't because it is done for you in math.atan2).

The __add__ and __str__ methods are magic (or dunder) methods. They allow you to give custom classes built-in behaviour. The former allows you to write v1 + v2 and the latter defines str(v). __str__ is also called by str.format, so printing the coordinates of the vector is a lot easier.

You might want to define, e.g. __mul__ for multiplication with another vector or a scalar.

This can be used similarly to your code:

if __name__ == "__main__":
    vector_A = Vector2D.from_mag_and_angle(2, 45)
    vector_B = Vector2D.from_mag_and_angle(*[6, 73.5])
    vector_C = vector_A + vector_B

    print(f'Coordinates of the endpoint of your vector are {vector_C} and its magnitude is {vector_C.magnitude}.')

The execution of this code is protected by a if __name__ == "__main__": guard to allow importing from this script from another script.

You can use tuple unpacking to unpack your list into the two arguments of the classmethod (as done for the second vector) or just specify the parameters directly (as done for the first).


Iterator for a list of objects Given a list of objects, return an iterator for such a list. Looking for code-review, optimizations and best practices. Verifying complexity to be O(n) where n is sum of all objects, including elements of collections if contained. previously solved here.

public class FlattenIterator implements Iterator<Object> {

    private final Stack<Iterator<?>> iteratorStack;
    private Object next;

    public FlattenIterator(Iterable<?> list) {
        if (list == null) {
            throw new NullPointerException();
        }

        iteratorStack = new Stack<Iterator<?>>();
        iteratorStack.push(list.iterator());
    }

    private void moveToNext() {
       while ((next == null) && !iteratorStack.empty()) {
            Iterator<?> itr = iteratorStack.peek();

            // if iterator is empty, then pop it from stack.
            if (!itr.hasNext()) {
                iteratorStack.pop();
            } else {
                final Object next = itr.next();
                if (next instanceof Iterable) {
                    iteratorStack.push(((Iterable<?>) next).iterator());
                    moveToNext();
                } else {
                    this.next = next;
                }
            }
        }
    }

    /**
     * Returns if there are any objects left to iterate over. This method
     * can change the internal state of the object when it is called, but repeated
     * calls to it will not have any additional side effects.
     */
    public boolean hasNext() {
        moveToNext();
        return next != null;
    }

    /**
     * Returns the next element in our iteration, throwing a NoSuchElementException
     * if none is found.
     */
    public Object next() {
        moveToNext();
        if (next == null) {
            throw new NoSuchElementException();
        }

        Object objectToReturn = next;
        next = null;
        return objectToReturn;
    }

    public void remove() {
        throw new UnsupportedOperationException();
    }
}

public class FlattenIteratorTest {

    @Test
    public void testIterator() {
        /**
         *  < <1, 2>, <"abc", "xyz">  <<1, 2>, <"abc", "xyz">> , a, 10 >
         */
        List<Object> genericList = new ArrayList<Object>();

        // integer list
        List<Integer> listInteger = new ArrayList<Integer>(Arrays.asList(1, 2));

        // string list
        List<String> listString = new ArrayList<String>(Arrays.asList("abc", "xyz"));

        // nested lists.
        List<Object> nestedList = new ArrayList<Object>();
        nestedList.add(listInteger);
        nestedList.add(listString);

        genericList.add(listInteger);
        genericList.add(listString);
        genericList.add(nestedList);
        genericList.add('a');
        genericList.add(10);

        /**
         * A very simple test case.
         */
        FlattenIterator fi = new FlattenIterator(genericList);
        final List<Object> listExpected = new ArrayList<>(Arrays.asList(1, 2, "abc", "xyz", 1, 2, "abc", "xyz", 'a', 10));
        final List<Object> listActual = new ArrayList<>();
        while (fi.hasNext()) {
           listActual.add(fi.next());
        }
        assertTrue(listExpected.equals(listActual));
    }

    @Test
    public void testEmpty() {
        List<Integer> list = new ArrayList<>();
        list.addAll(list);
        FlattenIterator fi = new FlattenIterator(list);
        assertFalse(fi.hasNext());
    }

}

I'm going to hit the high notes on my phone and swing back later with details unless someone beats me to it.

Allow null

Add a separate boolean field to track when next holds the actual next element rather than using null as a signal that it needs to be populated so that null elements can be supported.

Avoid Recursion

In this case, there's no reason for moveToNext to call itself. Continuing the loop should have the same effect.

Use Generics

Even though you're stuck using Object for variables which may hold an element or an iterable, you can still add a generic type T to the class and specify it as the return type from next. At least, I think you can. :)

Testing

When using the @Test annotation, you don't have to prefix each test method name with test. However, you really need more descriptive method names, and each test should validate one specific condition.

Burn this into your brain: Arrange, Act, Assert. Arrange the test fixture in the initial state, perform the action being tested, and assert the results. It can become tedious to setup the fixture each time, but helper methods can ease the pain and promote DRYness.


Wagner-Fischer algorithm Please let me know if there is any other optimal way of doing this.

public class StringDistance {

public static void main(String[] args) {
    int i, j, m, n, temp, tracker;

    int[][] d = new int[100][100];

    String s = "kittenede";
    String t = "sitting";

    char u[] = s.toCharArray();
    char v[] = t.toCharArray();

    m = u.length;
    n = v.length;

    for (i = 0; i <= m; i++) {
        d[i][0] = i;
    }
    for (j = 0; j <= n; j++) {
        d[0][j] = j;
    }

    for (j = 1; j <= m; j++) {
        for (i = 1; i <= n; i++) {
            if (u[j - 1] == v[i - 1]) {
                tracker = 0;
            } else {
                tracker = 1;
            }
            temp = Math.min((d[i - 1][j] + 1), (d[i][j - 1] + 1));
            d[i][j] = Math.min(temp, (d[i - 1][j - 1] + tracker));

        }
    }

    System.out.println("The levenstien distance" + " " + d[n][m]);
}
}

Bug
Your program computes the distance between "kittenede" and "s" as 1.  For some reason you switched your two array indices when you initialized them:

for (i = 0; i <= m; i++) {
    d[i][0] = i;
}
for (j = 0; j <= n; j++) {
    d[0][j] = j;
}

should be:
for (i = 0; i <= m; i++) {
    d[0][i] = i;
}
for (j = 0; j <= n; j++) {
    d[j][0] = j;
}

With this change, the program correctly computes the distance between "kittenede" and "s" as 9.
Fixed length array
There's no need for this:
int[][] d = new int[100][100];

when you already know the lengths of your two strings.  You should just move that line lower and use the n and m you compute:
int[][] d = new int[n+1][m+1];


LeetCode 665: Non-decreasing Array (C) I'm posting a solution for LeetCode's "Non-decreasing Array". If you'd like to review, please do. Thank you!
Problem
Given an array nums with n integers, your task is to check if it could become non-decreasing by modifying at most 1 element.
We define an array is non-decreasing if nums[i] <= nums[i + 1] holds for every i (0-based) such that (0 <= i <= n - 2).
Example 1:

Input: nums = [4,2,3]
Output: true
Explanation: You could modify the first 4 to 1 to get a non-decreasing array.

Example 2:

Input: nums = [4,2,1]
Output: false
Explanation: You can't get a non-decreasing array by modify at most one element.

Constraints:

1 <= n <= 10 ^ 4
-10 ^ 5 <= nums[i] <= 10 ^ 5

Code
// Since the relevant headers are already included on the LeetCode platform,
// the headers can be removed;
#include <stdio.h>
#include <stdbool.h>

static const bool checkPossibility(
    int *nums,
    const int nums_size
) {

    if (nums_size < 3) {
        return true;
    }

    int max_changes = 0;

    for (int index = 1; index < nums_size - 1; ++index) {
        if (!(nums[index] >= nums[index - 1] && nums[index + 1] >= nums[index])) {
            if (nums[index + 1] >= nums[index - 1]) {
                ++max_changes;
                nums[index] = nums[index - 1];

            } else {
                if (nums[index] < nums[index - 1] && nums[index + 1] < nums[index]) {
                    return false;

                } else if (nums[index] <= nums[index + 1]) {
                    nums[index - 1] = nums[index];

                    if (!(index - 1) || nums[index - 2] <= nums[index - 1]) {
                        ++max_changes;

                    } else {
                        return false;
                    }

                } else {
                    nums[index + 1] = nums[index];
                    ++max_changes;
                }
            }
        }

    }

    return max_changes < 2;
}

int main() {

    static const int nums_size = 3;
    int nums_array[nums_size] = {4, 2, 1};
    int (*nums)[nums_size] = &nums_array;

    fputs(checkPossibility(*nums, nums_size) ? "true" : "false", stdout);

    return 0;
}

The code mutates an array. It is not good.
The code does too much. You may return false as soon as max_changes reaches 2 (no need to examine the rest).
More functions please. It is very hard to follow the complicated decision making. Consider
int find_first_violation(int * nums, int size)
{
    int i = 0;
    for (; i < size; i++) {
        if (nums[i] < nums[i-1]) {
            break;
        }
    }
    return i;
}

Then the business logic would be:
    int violation = find_first_violation(nums, size);

    if (violation == size) {
        // array is already non-decreasing
        return true;
    }
    if (violation == size - 1) {
        // easily fixable: increase nums[size - 1]
        return true;
    }

    // Now fix the violation
    // violation == 1 is fixable by decreasing nums[0]. No action needed.
    // Otherwise, we only care about the case where nums[violation] is too
    // small - less than two preceding numbers. It is only fixable by
    // increasing it, effectively setting it equal to nums[violation - 1].

    if ((violation > 1) && (nums[violation] < nums[violation - 2])) {
        nums[violation] = nums[violation - 1];
    }

    // Finally, the core argument to have more functions: there
    // must be no more violations.

    return find_first_violation(nums + violation, size - violation) == size - violation;

Of course the first two conditions can be combined into violation >= size - 1. Of course increasing of nums[violation] can be virtual, without mutating the array (if nums[violation - 1] > nums[violation + 1] we may immediately return false;).


Java Is my class file methods well written? the purpose of this class file is to design a Saving account that stores a saving account's annual interest rate and balance. I have not wrote the test program yet, seeing that I believe I made a few mistakes with my class file. Also, at the end of classfile, I hid a method because I didn't understand how to word that method to add the interest rate to the balance.

Here are some additional information that could help you understand where I was going.

Name of class :SavingAccount Class

The Constructor should accept the amount of the savings account's starting balance.

Methods:
-Subtracting the amound of withdrawal.
-Adding the amound of a deposit.
-Adding the amount of monthly interest to the balance.
Other information:
- The annual rate is 5% (.05)
-monthly interest = Annual interest rate divided by 12.
- Getting the monthly interest rate added to balance: Add the monthly rate to the   balance. With the new result, add to the balance.

Class File

 public class SavingsAccount
{

private double balance;
private double startBalance;
private double AnnualRate = 0.5;
private double MonthlyInterest;
private double withdrawl;
 private double NewWithdrawl;
 private double deposit;
 private double NewDeposit;

public SavingsAccount()
{
balance = 0.0;
}

public SavingsAccount(double startBalance)
{
balance = startBalance;
}

public double getAnnualRate()
{
return AnnualRate;
}

public double MonthlyInterest()
{
MonthlyInterest= AnnualRate/12;
return MonthlyInterest;
}

public double getWithdrawl()
{
NewWithdrawl = balance - withdrawl;
return NewWithdrawl;
}
public double deposit()
{
NewDeposit = balance + deposit;
return NewDeposit;
}

/* ( The objecttive is to "Add the amount of monthly interest to the balance", not sure        where this was going)

public  getMonthlyInterestRate()
{
MonthlyBalance = MonthlyInterest x balance;
NewMonthlyBalance = MonthlyBalance + balance;
return NewMonthlyBalance;
 }
*/

}

Thanks for going through what I wrote, I hope I didn't make a huge mistake with my class file.

First and most important remark : do not use double or float to represent money. Use BigDecimal instead. And make a Money class that combines the amount with the currency. edit : it's also possible to use long or int, if you always calculate in cents, this would make a Money class even more useful, to hide away that implementation detail.
Think about rounding and precision. How many fractional digits can an annual rate have, how many can a monthly rate have...? How will you round if needed?
Don't hard code the values of the rates. If there's anything banks like to change it is their rates. :)
I would find it very odd if the date of the withdrawals and deposits did not matter in calculating the interests.
I'm not sure how NewWithdrawl and NewDeposit are supposed to get their values, from the posted code. As is these fields don't seem to make much sense, it feels like they want to be parameters of methods yet to be defined.
getWithdrawl() (sic) seems to do the same sort of operation as deposit(), yet the former is named as a getter method (while it isn't one) and the latter is not. Preferably they'd be named consistently.


Decimal to Binary in Rust As a beginner in Rust i am trying to practice Rust with some little programs
so here is my implementation of Decimal to Binary.

use std::io;

pub fn run() {
    let number = input("Enter an input");
    println!("{}", to_binary(number));
}

fn input(msg: &str) -> i32 {
    println!("{}", msg);
    let mut number = String::new();
    io::stdin()
        .read_line(&mut number)
        .expect("Failed to read input");

    match number.trim().parse() {
        Ok(num) => num,
        Err(_e) => panic!("Not a number"),
    }
}

fn to_binary(mut decimal: i32) -> i32 {
    if decimal == 0 {
        decimal
    } else {
        let mut bits = String::new();

        while decimal > 0 {
            if decimal % 2 == 0 {
                bits.push_str("0");
            } else {
                bits.push_str("1");
            }

            decimal /= 2;
        }

        // reverse the bits
        match bits.chars().rev().collect::<String>().parse() {
            Ok(num) => num,
            Err(_e) => panic!("Something went wrong"),
        }
    }
}

so feel free to advice anything you consider.
i just have three questions in particular

isn't it better to write input as a macro ?
how can i write to_binary as pure function ?
wouldn't be better to return binary type from to_binary instead i32

isn't it better to write input as a macro ?

No. At best it might work as a generic function.

  how can i write to_binary as pure function ?

It already is a pure function, it doesn't mutate or depend on external state

  wouldn't be better to return binary type from to_binary instead i32

You should return a String. Converting to i32 is pretty weird. The numbers inside i32 are already in binary. The point of a to_binary function is pretty much always because you for some reason want a textual representation in binary.

As a quick side note, Rust already has the ability to convert values to binary:

format!("{:b}", value);

Your to_binary function would be a bit simpler if build a Vec or Vec  and then converted that instead of building a String.


Regex to match each line in a file, with windows and/or linux line break included, even for missing line break at EOF My requirement is to match each line of a text file, including the line terminator of each, at most excluding the terminator of the last line, to take into account the crippled, non POSIX-compiant files generated on Windows; each line terminator can be either \n or \r\n.
As a consequence, no character in the file should be left unmatched.
The best regex I could come up with is this:
\n|\r\n|[^\r\n]++(\r\n|\n)?

Is this the best I can write, performance-wise?
Please, if you use the ^/$ anchors or similar, comment about that, because their behavior is dependent on whether the engine considers them as multiline by default.

I think this would work:
.*(\r?\n|$)

.* matches anything except line breaks (at least '\n', but some regex engines also treat other characters as line breaks).
(\r?\n|$) matches the line break or the end of the string (in case the last line is missing a line break.


Generate acronym from phrase I've posted the code for the following question. What could be improved?

  Q: Write a program that allows the user to type in a phrase and then
  outputs the acronym for that phrase.

  Note: The acronym should be all uppercase, even if the words in the
  phrase are not capitalized.

def main():

    ## input the phrase
    phrase = input("Enter the phrase: ")

    ## split the phrase into substrings
    phrase_split = phrase.split()

    acronym = ""

    ## iterate through every substring
    for i in phrase_split:
        acronym = acronym + i[0].upper()

    print("The acronym for your phrase is ",acronym + ".")

main()

You should name your function more descriptively, maybe acronym.
You should wrap your code under a if __name__ == "__main__": guard to allow importing parts of your script from other scripts.
You should separate the user input from the logic.
Use docstrings to describe what functions do.
Use str.join together with a generator expression to join multiple strings.
Use the new f-strings introduced in Python 3.6.

Taking this gives you:

def user_input():
    """Get the user input for a phrase"""
    return input("Enter the phrase: ")

def acronym(words):
    """Turn a list of words into an acronym"""
    return "".join(word[0] for word in words).upper()

def main():
    words = user_input().split()
    print(f"The acronym for your phrase is {acronym(words)}.")

if __name__ == "__main__":
    main()


Spotify Bestbefore Coding Challenge in Python I'm trying to solve the Spotify bestbefore programming challenge found here http://www.spotify.com/us/jobs/tech/best-before/

I have written the code in Python but when I send it in, it says "Wrong Answer". I have checked my code for bugs many times but unable to find out what is wrong. I know the answers to other's people's code is on the web but I do not want to look at theirs. I'm a firm believer in self-integrity and I really want to solve this myself. I'm just trying to found out some test cases where my code below does not work. Thanks

import sys

def values(year,month,day):
    valid = True
    #print "Debug: " + str(a) + " " + str(b) + " " + str(c)
    #Year
    fullYear = year
    if year < 2000:
        fullYear = year + 2000

    #Month
    if month > 12:
        valid = False

    #Day
    if month in (4,6,9,11):
        if day > 30:
            valid = False
    elif month == 2:
        is_leapyear = detectLeapyear(int(fullYear))
        if (is_leapyear):
            if day > 29:
                valid = False
        else:
            if day > 28:
                valid = False
    else:
        if day > 31:
            valid = False

    if not valid:
        return 0
    else:
        return 1

def detectLeapyear(in_year):
    rem4 = in_year % 4
    if not rem4:
        rem100 = in_year % 100
        if not rem100:
            rem400 = in_year % 400
            if not rem400:
                return 1
            else:
                return 0
        else:
            return 1
    else:
        return 0

if __name__ == '__main__':
    for line in sys.stdin:
        #date = sys.argv[1]
        date = line.strip()
        sArray = date.split('/')
        validArray = []

        x = int(sArray[0])
        y = int(sArray[1])
        z = int(sArray[2])

        #year-month-day
        if values(x,y,z):
            validArray.append([x,y,z])

        if values(x,z,y):
            validArray.append([x,z,y])

        if values(y,x,z):
            validArray.append([y,x,z])

        if values(y,z,x):
            validArray.append([y,z,x])

        if values(z,x,y):
            validArray.append([z,x,y])

        if values(z,y,x):
            validArray.append([z,y,x])

        if validArray:
            validArray = sorted(validArray, reverse=False)
            #Zero Padding of Numbers
            if validArray[0][0] < 2000:
                fyear = str(validArray[0][0] + 2000)

            if validArray[0][1] < 10:
                fmonth = "0" + str(validArray[0][1])
            else:
                fmonth = str(validArray[0][1])

            if validArray[0][2] < 10:
                fday = "0" + str(validArray[0][2])
            else:
                fday = str(validArray[0][2])
            #print "=>" + str(validArray[0][0]) + "-" + str(validArray[0][1]) + "-" + str(validArray[0][2])
            print fyear + "-" + fmonth + "-" + fday

            #print validArray
        else:
            print date + " is illegal"

These are the Test cases I have and they all seem to work:

12/11/10
02/4/67
31/9/73
31/12/2999
2000/6/12
2000/06/12
2000/12/6
2000/12/06
13/12/67
02/4/67
31/9/73
99/99/99

Program output:

2010-11-12
2067-02-04
31/9/73 is illegal
2067-12-31
2067-06-12
2067-06-12
2067-06-12
2067-06-12
2067-12-13
2067-02-04
31/9/73 is illegal
99/99/99 is illegal

I just went over the last version of your code you posted on freenode's #python, and there are two major errors in it:

You never check if month or day are 0, and your month > 12 check gets overwritten by the later if clauses. With those fixed, it passes.

Same issues are already in the code above, but I did not check if it has others as well.

the flag variable approach is very error prone and hard that way - it would be better to return False right away if you determine that, not tag it along with the chance that it might get flipped back erroneously.


Synesthesia for the rest of us (i.e. algorithm for unique string colorization) I'd like to form an algorithmic connections between  a particular string and and a particular color, in such that each unique string is always the same color.

My particular use case is for a kiosk where each user should immediately notice if someone else is logged in, so my strings are employee first names. Reinforcing this visually by assigning each user a different color. And yes I know i could just build a list or add a property to the user object but this is more fun. Here's my current approach:

public object Convert(object value, Type targetType, object parameter, CultureInfo culture)
{
    float r = 0;
    float g = 0;
    float b = 0;
    var name = value as string;
    if (name.IsNullOrEmpty()) return new SolidColorBrush(Colors.Black);
    var pos = 0;

    foreach (var letter in name)
    {
        var index = letter % 32;
        if (pos == 0) r = (float)index / AlphabetLength;
        if (pos == 1) g = (float)index / AlphabetLength;
        b = (float)index / AlphabetLength;
        pos++;
    }

    return new SolidColorBrush(Color.FromScRgb(1f, r, g, b));
}

private const int AlphabetLength = 26;

Kind of came up with this quickly so there are a bunch of weaknesses. What do you think?

Code Style

Where does the 32 come from? It seems strange to have AlphabetLength assigned as a constant, but 32 is an unexplained magic number. Especially if its value is actually based on the length of the alphabet, you should make it clear what the significance of that number is.
The method signature is pretty weird. Looking it up, apparently it's from some IValueConverter interface. You'd be better off extracting everything you have here into a private method which takes a string parameter and returns a SolidColorBrush. Then you can call that from Convert, doing any casting or validation you need there.
You have a mix of var and explicit type names. It'd be more consistent to declare your floats as var r = 0f, etc.
Your RGB values seem to be based entirely on the first, second and last characters of the string. That means there's really no reason to loop over it, you can just assign them directly
index is casted to a float everywhere it's used. It'd be cleaner to have it assigned as a float in the first place.

Algorithm

Currently, your names can be any colour at all. That's going to make displaying them hard, because you'll potentially have to modify the background colour to make sure the name reads clearly. You might be better off instead using HSV (Hue, Saturation, Value) colour space with a fixed V, then play with the H and S. It might also be good to use a restricted range of S so you don't end up with lots of washed-out, similar-looking colours
Using only the first, second and last characters in a name means you're going to have a lot of clashes, where different names show as the same colour. A different approach might be to do something like name.GetHashCode() to get an integer representing the string. You could then use that to generate the HSV (or RGB) components a few ways, like passing it as a seed to new Random (which will be consistent each time). Colour space is relatively limited, so you'll never get away from clashes altogether, but I think this will make them far less common.


Sorted unidirectional list with add, delete and find I am aware of the fact that a sorted unidirectional list has only very few use cases, but nevertheless, this python code feels fairly long for Python code. How can I improve it?

class List:
    def __init__(self, list):
        self.first = list

    def __repr__(self):
        element = self.first
        elementsList = ''
        while (element != None):
            elementsList += str(element) + '\n'
            element = element.next
        return elementsList

    """Searches for names in list, starts at 0, returns predecessor as object if possible, returns -1 if not found"""
    def search(self, name):
        if self.first.name == name:
            return 0
        else: return self.search_rec(self.first, name, 1)

    """Recursive workhorse for search"""
    def search_rec(self, item, name, i):
        if item.next != None:
            if item.next.name == name:
                return i, item.name
            else:
                return self.search_rec(item.next, name, i+1)
        else: return -1

class Element:
    """Class to implement elements of the list"""

    """Append item to list or create one"""
    def __init__(self, name, listPos=None):
        if listPos != None:
            listPos = listPos.parent.first
            self.parent = listPos.parent
            previous = None
            while (listPos.name < name and listPos.next != None):
                previous, listPos = listPos, listPos.next
            if previous != None:
                previous.next, self.next = self, listPos
            else: self.parent.first = self
        else:
            self.parent = List(self)
        self.name = name
        self.next = listPos

    def __del__(self):
        print(repr(self) + ' dies now...')

    def delete(self):
        if (self == self.parent.first): self.parent.first = self.next
        else:
            listPos = self.parent.first
            while (listPos.next != self):
                listPos = listPos.next
            if listPos.next.next != None: listPos.next = listPos.next.next
            else: listPos.next = None

    def __repr__(self):
        return '[%s]' % self.name

    def __str__(self):
        if self.next != None:
            next = 'Next element: ' + self.next.name
        else:
            next = '*end of the list*'
        return repr(self) + '\n' + next

x = Element('test')
a = Element('d', x)
y = Element('c', x)
z = Element('a', x)

print(x.parent)

a.delete()

print(a.parent)

print(x.parent.search('c'))

For starters, you should read PEP 8, the Python style guide. A few things that jump out from your code:

Variable names should be lowercase_with_underscores, not dromedaryCase.
Docstrings go inside the function body, not above the header.
Always put newlines after if, elif and else for multi-line statements. You do it inconsistently, which makes your code quite hard to follow.

Following these guidelines will make your code easier for other Python programmers to read.

Here are some brief, general comments on your code:

Pick better variable names. In the init function for the List class, you’ve got the variable name list. Pick something more meaningful (such as unidirectional_list, or whatever is appropriate).

Although List and Element aren't keywords, they are sufficiently generic that I’d suggest picking different ones.
Difference between __str__ and __repr__. The goal of __repr__ is to generate a string which can be eval()'d to return an equivalent object; the goal of __str__ is to return a human-readable interpretation.

The __repr__ method in List looks like it should really be __str__, and the __repr__ method for Element returns a singleton rather than something I can eval() to get another instance of the Element class.

See a question about this on Stack Overflow for more details.
Use a list comprehension in the __repr__/__str__ for List. Since we know that element.first is a Python list (although it’s not obvious from the attribute name; I’d suggest changing it), we can rewrite this using a list comprehension, then use join to construct the string. This is better than repeatedly appending to a string.

So you'd have something like:

def __str__(self):
    return '\n'.join(str(e) for e in self.first)

which is a lot cleaner.
Be explicit about comparisons to None. Maybe it’s just me, but I think x is not None is better than x != None.
Don't put examples/tests in the body of the script. At the bottom of the code are a few examples, which are helpful when I'm trying to review the code, but not in general use. Right now, if you import the file, all of those bits run and litter your output.

Either wrap them in an if __name__ == '__main__' block (see this Stack Overflow question for details), or consider using a dedicated library for tests. Then these examples will be executed if the file is run directly, but skipped if you import it in a larger file.


Class member method based multithreading [C++] In briefly: I'm working on a little chat program just for fun.

I am using threads to control two main parts of the application. I am making a socket controller class based on the following design:

//this is a "main.cpp example" for the design

std::mutex mtx;

class AThreadedClass
{
public:
    AThreadedClass()
    {
        running = true;
        value1 = 0;
        value2 = 0;
        value3 = 0;
    }

    //first method which is used for threads
    void run()
    {
        {
            std::lock_guard<std::mutex> lck(mtx);
            value1 = 0; value2 = 0; value3 = 0;
        }

        while (running)
        {
            {
                std::lock_guard<std::mutex> lck(mtx);
                value1 += 2; value2 +=1; value3 -= 1;
                if (value1 > 1000000)
                {
                    running = false;
                }
            }
        }
    }

    //second method used as thread
    //this will possible be just a data
    //retrieving function, not necessarly
    //used as thread
    void writeOut()
    {
        while(running)
        {
            std::lock_guard<std::mutex> lck(mtx);
            std::cout << value1 << ';' << value2 << ';' << value3 << '\n';
        }
    }
private:
    int value1, value2, value3;
    std::atomic<bool> running;
};

int main()
{
    AThreadedClass class1;
    std::thread t1(&AThreadedClass::run, &class1);
    std::thread t2(&AThreadedClass::writeOut, &class1);

    t1.join();
    t2.join();

    std::cout << "Threads finished successfully!\n";

    return 0;
}

So, here are the things I was wondering about:

Is this a good way to manage things? (I mean, using classes and their member functions for threads could be a stupid idea :/)
Are there possible deadlocks, when I am mixing std::atomic with std::mutex like in the example? (So far it worked perfectly for me)

And if I didn't notice an existing question about this, please point to it.

I think you have an OK approach, however it's hard to tell if it's going to work for you because at the moment you seem to be at concept phase.  As you develop the application, you may find that having different methods for different thread loops may mean your class is responsible for too much.  I tend to prefer to have a class for each thread I create, which has a main thread function.  An example of which would be the WorkerThread in this question I posted.

Nested scopes in functions

As far as your coding style goes, I'm not super keen on the way you're using braces to create scoping for your mutex uses:

{
    std::lock_guard<std::mutex> lck(mtx);
    value1 = 0; value2 = 0; value3 = 0;
}

I'd be inclined to create a function with a name if I felt like I needed to have the extra scoping and put the body into it.  Your run method also re-initializes the values even these have been set in the constructor, it's unclear if this is on purpose or not.

Global Mutex

You've got std::mutex mtx; as a global.  If you're only going to be using it from within your AThreadedClass, it would be better as a static class member.  I'd also update its name to reflect what it is it's used to lock.

Atomic Vs Volatile

Whilst atomic is the safe option for running, I'm not sure you really need it to be atomic.  You never update its value outside of the mtx lock.  I would expect that marking it as volatile should be enough to prevent the compiler from optimizing out the checks in the while loops.


Avoid using object to hold int, bool, double application setting values I have some code to handle integer, double, Boolean, and string settings. Right now, each setting variant extends an abstract class, where the setting value is stored as an object.

Is there any way to avoid this? What I'd like to do is have IntSetting objects return their Value as an integer, DoubleSetting objects return their Value as a double and so on and so forth.

Here is the code referenced above:

public abstract class ModelSetting
{
    public object Value { get; set; }
    public abstract string ToString();

    public ModelSetting(object value)
    {
        Value = value;
    }
}

public class IntSetting : ModelSetting
{
    public IntSetting(int value)
        : base(value)
    {
    }

    public override string ToString()
    {
        return (bool)Value ? "1" : "0";
    }
}

public class DoubleSetting : ModelSetting
{
    public DoubleSetting(double value)
        : base(value)
    {
    }

    public override string ToString()
    {
        return ((double)Value).ToString("%g");
    }
}

As you say, returning an object isn't ideal. This is a relatively straightforward use case for generics:

public abstract class ModelSetting<T>
{
    public T Value { get; set; }
    public abstract string ToString();

    public ModelSetting(T value)
    {
        Value = value;
    }
}

Then implementing classes can use:

public class DoubleSetting : ModelSetting<double>
{
    //...The rest of the class
}

This:

public abstract string ToString();

Is bad. ToString is already defined on Object, so by declaring an abstract version here, you're not overriding, you're hiding. This is far less frequently done, and for good reason- it breaks our basic expectations of polymorphism. You should only do it with good reason, and you don't have one here. Just get rid of the ToString redeclaration, it gives you nothing.

You require a value in your constructor, but then let Value be set publicly. This doesn't really make much sense- somebody could just pass default(T) to the constructor, making the constructor meaningless. You're giving two different ways to set the value, for no particular reason. It's not a big deal, but is needlessly confusing.

Consider whether you actually need to be able to change Value for an existing ModelSetting instance. If so, remove the constructor. If not (and this is probably better!), make the property private set. This makes the value immutable, which makes it much easier to reason about the class. For example, if I hold an instance, I know I can pass that instance to whoever I want without having to worry about them changing the value without me expecting it.

return (bool)Value ? "1" : "0";

Shouldn't this be the BoolSetting version, rather than the IntSetting one? At least you can take comfort that this bug is a demonstration of one of the advantages of the strong-typedness that you get from the generic approach!


Exception handling continuing the excecution This code is for continuing the execution after an exception, and this is ugly:

    int step=0;

    do{
        try{
                switch(step)
                {
                case 0:
                    step=1;
                    methodAAAAACanThrowException();
                    break;
                case 1:
                    step=2;
                    methodBBBBBCanThrowException();
                    break;
                case 2:
                    step=3;
                    methodCCCCCanThrowException();
                    break;
                    //....

                    //more and more

                    //....
                case 25:
                    step=26;
                    methodZZZZZCanThrowException();
                }
        }catch( Exception ex)
        {

            Logger.getLogger(ABC.class.getName()).log(Level.SEVERE, null, ex);
        }
    }while(step<26);
}

but writing a single try-catch loop for every call, seems even uglier..

Is there any other more elegant option for doing that?
thanks

How about something like this?

Define a one-method interface like so:

public interface TryAndLogFailure { void execute(); }

Then define a method which takes an object which implements the interface and does your exception handling for you:

public void tryAndLogFailure(TryAndLogFailure talf) {
  try {
    talf.execute();
  } catch (Exception e) {
    logger.log(...);
  }
}

Now your calls to the methods can be sequential:

tryAndLogFailure(new TryAndLogFailure{ public void execute() { methodAAAACanThrowException(); } });
tryAndLogFailure(new TryAndLogFailure{ public void execute() { methodBBBBCanThrowException(); } });
// etc...
tryAndLogFailure(new TryAndLogFailure{ public void execute() { methodZZZZCanThrowException(); } });

Admittedly, this could probably be worked a little more to make it prettier, but this is my first crack off the top of my head. If only Java had support for closures.

(Also - I should note that I agree with Winston. The reason why this code feels ugly is because the concept itself is ugly. Catching Exception is almost never a good idea and requires strong justification. Given more context, we may be able to help you redesign the code to avoid this sort of code smell.)


Singly linked list abstraction in ES5 JavaScript Below is the code for, Singly linked list implementation,

function List(){
    this.start=null;
    this.end=null;
}

List.makeNode=function(){
    return {data:null,next:null};
};

List.prototype.add=function (data){
    if(this.start===null){
        this.start=List.makeNode();
        this.end=this.start;
    }else{
        this.end.next=List.makeNode();
        this.end=this.end.next;
    }
    this.end.data=data;
};

List.prototype.traverse = function(){
    var current = this.start;
    while (current !== null) {
        current = current.next;
    }
};

List.prototype.insertAsFirst = function(d) {
    var temp = List.makeNode();
    temp.next = this.start;
    this.start = temp;
    temp.data = d;
};
List.prototype.delete = function(data) {
    var current = this.start;
    var previous = this.start;
    while (current !== null) {
        if (data === current.data) {
            if (current === this.start) {
                this.start = current.next;
                return;
            }
            if (current == this.end) this.end = previous;
            previous.next = current. next;
            return;
        }
        previous = current;
        current = current.next;
    }
};
List.prototype.insertAfter = function(t, d) {
    var current = this.start;
    while (current !== null) {
        if (current.data === t) {
            var temp = List.makeNode();
            temp.data = d;
            temp.next = current.next;
            if (current == this.end) this.end = temp;
                current.next = temp;
                return;
            }
            current = current.next;
        }
};
List.prototype.item=function(i){
    var current = this.start;
    while (current !== null) {
        i--;
        if(i===0) return current;
        current = current.next;
    }
    return null;
};
List.prototype.each = function(f) {
    var current = this.start;
    while (current !== null) {
        f(current);
        current = current.next;
    }
};

From the above code,

1) Is List a proper abstraction?

2) Does List ensure encapsulation?

3) Is the usage of List.prototype justified?

4) Does makeNode justified as static function?

You don't need to track the end of the list. You can just check that you've reached it if an element's next is null.

makeNode doesn't do anything. Every time you use it you set data instantly after, so data should be a parameter. Setting next to null isn't that useful because if you don't change it you can simply check node.next === undefined and get the same thing, while using less memory.

Try

function MakeNode(data) {
  return { data: data };
}

In general it's not that useful to separate the node and list abstractions. If you want a list just pass around the first node of the list. It makes many algorithms much simpler to read and write.

I'd start with

function Node(data, next) {
  this.data = data;
  this.next = next;
}

This sets next to undefined if you just do new Node(5).


Generating time series for generator ON or OFF Want to create time series for the nuclear power plant performance using the equation for Time to fail and time to repair for 8736 hours in the year so that I have the time series in hours when the generator is operating in when is not. The starting condition is that the generator is operating on the first hour.

For sure there is a more elegant solution for simulating this I'm just not able to find it. Any comment or help will be appreciated. The code is working but I think there is a better solution

TTF<-2940 # MEDIUM TIME TO FAIL(hours)
MTTR<-60 # MEDIUM TIME TO REPAIR (hours)
TTF<--MTTF*log(runif(100))# equation for Time to fail
TTR<--MTTR*log(runif(100))# equation for Time to repair
mix<-rep(0,length(TTF)+length(TTR))
sw<-rep(0,length(TTF)+length(TTR))
for(i in 1:length(TTF)){
mix[2*i-1]<-TTF[i]
sw[2*i-1]<-1
mix[2*i]<-TTR[i]
}
cmix<-cumsum(mix)
ccmix<-cbind(cmix[1:which(cmix>8736)],sw[1:which(cmix>8736)])
ccmix[dim(ccmix)[1],1]<-8736

G1<-round(ccmix)
# transform binary values
G1[,2][G1[,2] == 1] <- 400 # 400 MW is the capacity of power plant

G1 <- cbind(G1, c(G1[1,1], diff(G1[,1])))
a1 <- rep(G1[,2], G1[,3]) ## GENERATING 8736 Values

So the desired output are 8736 values of 400 when is ON and 0 when is OFF. Finally I need to simulate 100000 years of generation.

I don't know of any method, but here are some small improvements:

(1) You want mix to be TTF and TTR intertwined. An easy, vectorized way to do that is:

mix <- c(rbind(TTF,TTR))

(2) You want sw to be an alternating vector of the capacity and 0. This can be vectorized as well:

cap <- 400
sw <- rep(c(cap,0), length(TTF))

(3) Next, simply find the index which is the first value above 8736 and rep that:

final <- 8736
ind <- which.max(cumsum(mix) > final)
out <- rep(sw[1:ind], round(mix)[1:ind])[1:final]

So full code would be:

MTTF <- 2940
MTTR <- 60
final <- 8736
cap <- 400
TTF <--MTTF*log(runif(100))
TTR <--MTTR*log(runif(100))

mix <- c(rbind(TTF,TTR))
sw <- rep(c(cap,0), length(TTF))
ind <- which.max(cumsum(mix) > final)
out <- rep(sw[1:ind], round(mix)[1:ind])[1:final]


Cancellable progress form using async/await Introduction:

For a project I'm working on I used a BackGroundWorker to lift some heavy tasks away from the UI thread and report progress. For this I created some sort of modal dialog with a label and progress bar which were updated from the ProgressChanged method of the backgroundworker.

I read a very interesting article on async/await and I thought I might update my code, since the BackGroundWorker is, despite being handy, getting outdated anyway. After some more reading on this matter I found an article with code that had the backgroundworker instance inside the modal dialog form and not on the caller form.

This gave me the idea to do the same but using async/await instead. This is a first result, which is working, and I was wondering if I am doing things right. Any tips and/or improvements are much appreciated, certainly regarding scalability/extensibility and the use of async/await.

The form:

The code:

public partial class CancellableProgress : Form
{
    private readonly IEnumerable<KeyValuePair<string, Action>> _tasks;
    private CancellationTokenSource _cancelTokenSource;
    private bool _isTaskCompleted;

    public CancellableProgress(string message, Action action)
        : this(new[] { new KeyValuePair<string, Action>(message, action) })
    {
    }

    public CancellableProgress(IEnumerable<KeyValuePair<string, Action>> tasks)
    {
        InitializeComponent();
        Load += CancellableProgress_Load;
        _tasks = tasks;
    }

    private void CancellableProgress_Load(object sender, EventArgs e)
    {
        PerformTasks(_tasks);
    }

    private async void PerformTasks(IEnumerable<KeyValuePair<string, Action>> tasks)
    {
        _cancelTokenSource = new CancellationTokenSource();
        var token = _cancelTokenSource.Token;
        var progressHandler = new Progress<string>(value => MessageLabel.Text = value);
        var progress = progressHandler as IProgress<string>;

        try
        {
            await Task.Run(() =>
            {
                foreach (var task in tasks)
                {
                    if (token.IsCancellationRequested)
                    {
                        token.ThrowIfCancellationRequested();
                    }
                    progress.Report(task.Key);
                    task.Value();
                }
            }, token);
            _isTaskCompleted = true;
        }
        catch (OperationCanceledException)
        {
            MessageLabel.Text = @"Cancelled!";
        }
        catch (Exception ex)
        {
            MessageLabel.Text = ex.Message;
        }
        finally
        {
            _cancelTokenSource.Dispose();
            DialogResult = _isTaskCompleted ? DialogResult.OK : DialogResult.Cancel;
        }
    }

    private void btnCancel_Click(object sender, EventArgs e)
    {
        CancelTaskButton.Enabled = false;
        _cancelTokenSource.Cancel();
    }
}

Example usage:

This is some code on how I use the modal form.

private void TaskButton_Click(object sender, EventArgs e)
{
    var operations = new[]
    {
        new KeyValuePair<string, Action>("Operation 1", LongOperation),
        new KeyValuePair<string, Action>("Second operation", LongOperationTwo)
    };

    using (var progress = new CancellableProgress(operations))
    {
        progress.ShowDialog();
    }
}

private static void LongOperation()
{
    for (var i = 1; i <= 5; i++)
        System.Threading.Thread.Sleep(1000);
}

private static void LongOperationTwo()
{
    for (var i = 1; i <= 5; i++)
        System.Threading.Thread.Sleep(1000);
}

Clean code, easy to follow its meaning. Some minor remarks:

var progressHandler = new Progress<string>(value => MessageLabel.Text = value);
var progress = progressHandler as IProgress<string>;

This looks like you can just define it as

IProgress<string> progressHandler = new Progress(value => MessageLabel.Text = value);

Or just a Progress<string> type, really:

var progressHandler = new Progress(value => MessageLabel.Text = value);

Unless I'm mistaking, you don't actually need it as an interface. Note that I also removed the explicit type parameter since that should be inferred from the argument passed to it.

MessageLabel.Text = @"Cancelled!";

The verbatim string isn't necessary here

if (token.IsCancellationRequested)
{
    token.ThrowIfCancellationRequested();
}

This seems redundant to me.

It should be either this

if (token.IsCancellationRequested)
{
    throw new OperationCanceledException();
}

or this:

token.ThrowIfCancellationRequested()

This is also reinforced by the documentation

private async void PerformTasks()

Asynchronous methods should return Task or Task<T>, the former being for traditional void methods and the latter being for methods with T return type.

The reason for this is that a void method cannot be awaited (since it doesn't return a Task) and will in effect be a fire-and-forget kind of thing: it will start executing your asynchronous code on a separate thread and continue doing business as normal. However once the main thread reaches its end, the application will exit regardless of whether or not your asynchronous code has finished as well. This obviously leads to nasty bugs.

You won't notice this in this case because you're using a GUI which means there will always be a main thread active (as long as the application runs).

However in case you ever want to re-use this code for a console application or just in a class library where you don't know the client, I would suggest making it async Task just for good measure.

The only exception to this are asynchronous event handlers since they have to adhere to the convention that event handlers return void. This also explains why events are "dangerous" to use in asynchronous code.


Validating a password format How should I make this more Ruby-like or just "better"?

def password_format_is_valid?(submitted_password)

  #Gets regular expression for password format validation from settings and applies it
  regex = Regexp.new(Setting['global_admin.password_format_regex'])
  if submitted_password =~ regex then
    return true
  else
    self.errors.add(:password, Setting['global_admin.password_format_error_message'])
    return false
  end

end

It seems like you need a Rails validation callback (and a virtual attribute for submitted_password). I'd write:

attr_accessor :submitted_password
validate :password_format_is_valid?

def password_format_is_valid?
  regex = Regexp.new(Setting['global_admin.password_format_regex'])
  unless submitted_password =~ regex
    self.errors.add(:password, Setting['global_admin.password_format_error_message'])
    false
  end
end

Comments:

In the vein of Lisp, the last expression of a body in Ruby is the return value of the method/block, so no need to use an explicit return (in fact it's unidiomatic and discouraged)
Note that Rails can validate fields with regular expression, you should use the predefined validations whenever possible: validates_format_of.


Minimax for Tic-Tac-Toe I have written AI for Tic-Tac-Toe using Negamax in JavaScript. The Negamax object is translated from an implementation written in Python. The code runs on a Node.js server and the player interacts with it via web socket. The code works and the game is unbeatable meaning it always wins if it can and if it can't, will force a tie.

I am looking for advice on how to optimize this implementation of MiniMax. I am also fairly new to Node.js and am wondering if this will run in a non-blocking way (assuming an instance of the object for each connected user.

Minimax Object:

var MiniMax = function(){
  //init values and options
  this.bestMove = 0;
  this.MAX_DEPTH = 6;
}

MiniMax.prototype = {
  //function called from game, bestmove will return the computer move
  buildTree: function(board, player, cb){
    this.bestMove = 0;
    var alpha = this.buildTree_r(board, player, 0);
    cb(this.bestMove);
  },
  //recursive function to build minimax tree and rate the value of the board
  buildTree_r: function(board, currPlayer, depth){
    if(depth > this.MAX_DEPTH){
      return 0;
    }
    //Set the otherplayer for the next game state and to check for loss
    var otherPlayer;
    if(currPlayer == board.X){
      otherPlayer = board.O;
    } else {
      otherPlayer = board.X;
    }
    //check for a winner in the boardstate, if currPlayer we win, else we lose in this tree
    var winner = board.getWinner();
    if(winner == currPlayer){
      return 1;
    } else if(winner == otherPlayer){
      return -1;
    }
    //check for a full board and therefore cats game in this true
    if(board.isFull()){
      return 0;
    }
    //this is where we begin to rank moves, get an array of moves, set alpha low, instantiate parallel
    //subAlpha list  to movelist to remember move ranks
    var moveList = board.getMoves();
    var alpha = -1;
    var saList = [];
    for(var i=0; i<moveList.length; i++){

      var boardCopy = board.copy(); //Copy current gamestate
      boardCopy.move(currPlayer, moveList[i]); //Make a move for in the gamestate for each possible move
      //console.log(boardCopy.gamestate);

      var subalpha = -this.buildTree_r(boardCopy, otherPlayer, depth + 1); //pass new gamestate into recursion
      if(alpha < subalpha){ //if move is better than alpha, increase alpha
        alpha = subalpha;
      }
      if(depth == 0){ //only if we are looking at REAL gamestate do we push an alpha to the list
        saList.push(subalpha);
      }
    }
    if(depth == 0){
      var posMoves = [];
      for(var n=0; n<saList.length; n++){
        if(saList[n] == alpha){
          posMoves.push(moveList[n]);
        }
      }
      this.bestMove = this.rand(posMoves); //in future pick random..
    }
    return alpha;
  },
  rand: function(list){
    var item = list[Math.floor(Math.random() * list.length)];
    return item;
  }
}

Board Object:

var Board = function(){
  this.empty = 0;
  this.X = 1;
  this.O = 2;
  this.wins = [
    [0,1,2],
    [3,4,5],
    [6,7,8],
    [0,3,6],
    [1,4,7],
    [2,5,8],
    [0,4,8],
    [2,4,6]
    ];
  this.gamestate = [0,0,0,0,0,0,0,0,0];
}

Board.prototype = {
  copy: function(){
    var b = new Board();
    for(var i=0; i<9; i++){
      b.gamestate[i] = this.gamestate[i];
    }
    return b;
  },
  move: function(player, pos){
    this.gamestate[pos] = player;
  },
  getMoves: function(){
    var moves = [];
    for(var i=0; i<9; i++){
      if(this.gamestate[i] == this.empty){
        moves.push(i);
      }
    }
    return moves;
  },
  isFull: function(){
    for(var i=0; i<9; i++){
      if(this.gamestate[i] == this.empty){
        return false;
      }
    }
    return true;
  },
  getWinner: function(){
    for(var i=0; i<this.wins.length; i++){
      var a, b ,c;
      a = this.gamestate[this.wins[i][0]];
      b = this.gamestate[this.wins[i][1]];
      c = this.gamestate[this.wins[i][2]];

      if(a == b && a == c && a != this.empty){
        return a;
      }
    }
    return this.empty;
  }
}

This would be a good place to use a ternary comparison operator:

var otherPlayer;
if(currPlayer == board.X){
  otherPlayer = board.O;
} else {
  otherPlayer = board.X;
}

Then it would become a single line:

var otherPlayer = currPlayer == board.X ? board.O : board.X;

You could do this in a few other places as well.

for(var i=0; i<moveList.length; i++){

You use spaces around your operators for the most part, you should be consistent with this.

var b = new Board();

In the Minimax object code, you use good variables names.  Not so much in the Board object code.

Otherwise, this looks good to me, very clean, neat, and well documented.


Simple Nagios Scraper - version 2 Version 1 - Beginner web scraper for Nagios

Current version changes:

Moved NAGIOS_DATA dictionary to separate file (and added to .gitignore)
Used functions with DOCSTRINGS
Removed the multiple redundant print() statements
Actually read the PEP8 standards, and renamed variables to match the requirements

Again, beginner Python programmer. I appreciate the feedback!

import requests
from scraper import NAGIOS_DATA
from bs4 import BeautifulSoup
from requests.auth import HTTPBasicAuth, HTTPDigestAuth

def get_url_response(url, user, password, auth_type):
    """Get the response from a URL.

    Args:
        url (str): Nagios base URL
        user (str): Nagios username
        password (str): Nagios password
        auth_type (str): Nagios auth_type - Basic or Digest

    Returns: Response object
    """

    if auth_type == "Basic":
        return requests.get(url, auth=HTTPBasicAuth(user, password))
    return requests.get(url, auth=HTTPDigestAuth(user, password))

def main():
    """
    Main entry to the program
    """

    # for nagios_entry in ALL_NAGIOS_INFO:
    for url, auth_data in NAGIOS_DATA.items():
        user, password, auth_type = auth_data["user"], auth_data["password"], \
            auth_data["auth_type"]
        full_url = "{}/cgi-bin/status.cgi?host=all".format(url)
        response = get_url_response(full_url, user, password, auth_type)
        if response.status_code == 200:
            html = BeautifulSoup(response.text, "html.parser")
            for i, items in enumerate(html.select('td')):
                if i == 3:
                    hostsAll = items.text.split('\n')
                    hosts_up = hostsAll[12]
                    hosts_down = hostsAll[13]
                    hosts_unreachable = hostsAll[14]
                    hosts_pending = hostsAll[15]
                    hosts_problems = hostsAll[24]
                    hosts_types = hostsAll[25]
                if i == 12:
                    serviceAll = items.text.split('\n')
                    service_ok = serviceAll[13]
                    service_warning = serviceAll[14]
                    service_unknown = serviceAll[15]
                    service_critical = serviceAll[16]
                    service_problems = serviceAll[26]
                    service_types = serviceAll[27]
                # print(i, items.text) ## To get the index and text
            print_stats(
                user, url, hosts_up, hosts_down, hosts_unreachable,
                hosts_pending, hosts_problems, hosts_types, service_ok,
                service_warning, service_unknown, service_critical,
                service_problems, service_types)

    # print("Request returned:\n\n{}".format(html.text))
    # To get the full request

def print_stats(
        user, url, hosts_up, hosts_down, hosts_unreachable, hosts_pending,
        hosts_problems, hosts_types, service_ok, service_warning,
        service_unknown, service_critical, service_problems, service_types):
    print("""{}@{}:
                Hosts
    Up\tDown\tUnreachable\tPending\tProblems\tTypes
    {}\t{}\t{}\t\t{}\t{}\t\t{}
                Services
    OK\tWarning\tUnknown\tCritical\tProblems\tTypes
    {}\t{}\t{}\t{}\t\t{}\t\t{}""".format(
        user, url, hosts_up, hosts_down, hosts_unreachable, hosts_pending,
        hosts_problems, hosts_types, service_ok, service_warning,
        service_unknown, service_critical, service_problems, service_types))

if __name__ == '__main__':
    main()

scraper.py source:

NAGIOS_DATA = {
    'http://192.168.0.5/nagios': {
        'user': 'nagiosadmin',
        'password': 'PasswordHere1',
        'auth_type': 'Basic'
    },
    'https://www.example.com/nagios': {
        'user': 'exampleuser',
        'password': 'P@ssw0rd2',
        'auth_type': 'Digest'
    },
}

There are still a couple of rouge non-PEP8-compliant variable names: serviceAll and hostsAll.

This is a minor detail, but to avoid too much nesting I would suggest inverting this condition if response.status_code == 200:. Then you can write it like this:

if response.status_code != 200:
    continue # or raise an exception

html = BeautifulSoup(response.text, "html.parser")

IMO, such code is much easier to read. These kind of checks are also called guards (https://en.wikipedia.org/wiki/Guard_(computer_science)).

Instead of iterating through all the td tags, I would store them in a list and then extract the necessary elements with an index:

td_elements = list(html.select('td'))
hosts_all = td_elements[3].text.split('\n')
service_all = td_elements[12].text.split('\n')

Next, I would like to focus on the print_stats function. It takes way to many parameters and has become tough to work with. I suggest storing all variables you extract from the HTML in a dictionary, which you can then pass to the print_stats function.

extracted_information = {
    'hosts_up': hosts_all[12],
    'hosts_down': hosts_all[13],
    'hosts_unreachable': hosts_all[14],
    'hosts_pending': hosts_all[15],
    'hosts_problems': hosts_all[24],
    'hosts_types': hosts_all[25],
    'service_ok': service_all[13],
    'service_warning': service_all[14],
    'service_unknown': service_all[15],
    'service_critical': service_all[16],
    'service_problems': service_all[26],
    'service_types': service_all[27],
}

Then you would call the print_stats function like this: print_stats(user, url, extracted_information).

Of course, we now have to rewrite the print_stats function itself. The Python format function can also take named parameters. For example: "{param1} and {param2}".format(param1="a", param2="b") would return string "a and b". Using this we can rewrite the template string and pass the "unpacked" extracted_information dictionary to the format function.

def print_stats(user, url, extracted_information):
    template = """{user}@{url}:
                Hosts
    Up\tDown\tUnreachable\tPending\tProblems\tTypes
    {hosts_up}\t{hosts_down}\t{hosts_unreachable}\t\t{hosts_pending}\t{hosts_problems}\t\t{hosts_types}
                Services
    OK\tWarning\tUnknown\tCritical\tProblems\tTypes
    {service_ok}\t{service_warning}\t{service_unknown}\t{service_critical}\t\t{service_problems}\t\t{service_types}"""

    print(template.format(user=user, url=url, **extracted_information))