Bit manipulation tool set I made a tool set to facilitate bit manipulation as follows. Any comments are welcome, be it about the interface design, the implementation, or even the naming. Also, I want to make sure that the tool set is not lacking something for use-cases I failed to catch :-) Original code and unit tests can be found here and here.

/// The index is zero-based, starting from the least significant bit.
/// The most significant bit or sign bit is the head bit.

template <typename T>
constexpr unsigned num_bits(T = 0) {
  return sizeof(T) << 3;
}

template <typename T>
constexpr unsigned head_bit_idx(T = 0) {
  return num_bits<T>() - 1;
}

template <typename T>
T bit_flag(unsigned idx) {
  // since C++14, `1 << (num_bits<int> - 1)` is well-defined and makes `INT_MIN`
  return static_cast<T>(1) << idx;
}

template <typename T>
constexpr T head_bit_flag() {
  return static_cast<T>(1) << head_bit_idx<T>();
}

template <typename T>
bool test_bit(T x, unsigned idx) {
  return x & bit_flag<T>(idx);
}

template <typename T>
bool test_head_bit(T x) {
  return test_bit(x, head_bit_idx(x));
}

template <typename T>
T set_bit(T x, unsigned idx) {
  return x | bit_flag<T>(idx);
}

template <typename T>
T set_head_bit(T x) {
  return set_bit(x, head_bit_idx<T>());
}

template <typename T>
T clear_bit(T x, unsigned idx) {
  return x & ~bit_flag<T>(idx);
}

template <typename T>
T clear_head_bit(T x) {
  return clear_bit(x, head_bit_idx<T>());
}

template <typename T>
T flip_bit(T x, unsigned idx) {
  return x ^ bit_flag<T>(idx);
}

template <typename T>
T flip_head_bit(T x) {
  return flip_bit(x, head_bit_idx<T>());
}

template <typename T>
unsigned num_bits_set(T x) {
  unsigned cnt = 0;
  // behavior of signed type underflow is unspecified
  std::make_unsigned_t<T> v = x;
  while (v) {
    ++cnt;
    v &= v - 1;
  }
  return cnt;
}

False optimization (and makes it hard to read).

template <typename T>
constexpr unsigned num_bits(T = 0) {
  return sizeof(T) << 3;
}

This should be sizeof(T) * 8 that's so much easier to read. And if it quicker to shift that is what the compiler will actually do. Don't try and outsmart the compiler the only thing that will is happen is that you will outsmart yourself.

Also its not actually 8. TO be compliant and accurate use CHAR_BIT.

This is a bit inefficient.

template <typename T>
unsigned num_bits_set(T x) {
  unsigned cnt = 0;
  // behavior of signed type underflow is unspecified
  std::make_unsigned_t<T> v = x;
  while (v) {
    ++cnt;
    v &= v - 1;
  }
  return cnt;
}

You are using a runtime loop (over each bit). You could use a compile time loop over each byte. Because the number of bits set for each specific value for a byte it is easy to pre-calculate.

Note: For each byte there are only CHAR_BIT number of bits (usually 8). Which is 256 values. So you can swap space for time here for a single byte.

unsigned num_bits_set_count_value[] = {
0, 1, 1, 2, 1, 2, 2, 3, 1, 2, 2, 3, 2, 3, 3, 4,  // 000-015
1, 2, 2, 3, 2, 3, 3, 4, 2, 3, 3, 4, 3, 4, 4, 5,  // 016-031
1, 2, 2, 3, 2, 3, 3, 4, 2, 3, 3, 4, 3, 4, 4, 5,  // 032-047
2, 3, 3, 4, 3, 4, 4, 5, 3, 4, 4, 5, 4, 5, 5, 6,  // 048-063
1, 2, 2, 3, 2, 3, 3, 4, 2, 3, 3, 4, 3, 4, 4, 5,  // 064-079
2, 3, 3, 4, 3, 4, 4, 5, 3, 4, 4, 5, 4, 5, 5, 6,  // 080-095
2, 3, 3, 4, 3, 4, 4, 5, 3, 4, 4, 5, 4, 5, 5, 6,  // 096-111
3, 4, 4, 5, 4, 5, 5, 6, 4, 5, 5, 6, 5, 6, 6, 7,  // 112-127
// ---
1, 2, 2, 3, 2, 3, 3, 4, 2, 3, 3, 4, 3, 4, 4, 5,  // 128-143
2, 3, 3, 4, 3, 4, 4, 5, 3, 4, 4, 5, 4, 5, 5, 6,  // 144-159
2, 3, 3, 4, 3, 4, 4, 5, 3, 4, 4, 5, 4, 5, 5, 6,  // 160-175
3, 4, 4, 5, 4, 5, 5, 6, 4, 5, 5, 6, 5, 6, 6, 7,  // 176-191
2, 3, 3, 4, 3, 4, 4, 5, 3, 4, 4, 5, 4, 5, 5, 6,  // 192-207
3, 4, 4, 5, 4, 5, 5, 6, 4, 5, 5, 6, 5, 6, 6, 7,  // 208-223
3, 4, 4, 5, 4, 5, 5, 6, 4, 5, 5, 6, 5, 6, 6, 7,  // 224-239
4, 5, 5, 6, 5, 6, 6, 7, 5, 6, 6, 7, 6, 7, 7, 8}; // 240-255

// Use compile time meta program
// to unwind a multi byte value into a set of adds
// these will all be inlined.
template <int x>
unsigned num_bits_set_count(char const* byteStream) {
    return num_bits_set_count_value[*byteStream]
         + num_bits_set_count<x - 1>(byteStream + 1);
}
template<> unsigned num_bits_set_count<0>(char const*) {return 0;}

template <typename T>
unsigned num_bits_set(T x) {
   return num_bits_set_count<sizeof(T)>(reinterpret_cast<char const*>(&x));
}

Overall its fine. But you can get the same functionality from using std::bitset and its more readable.

 int a {86};
 std::bitset<sizeof(a) * CHAR_BIT>   x(a);
 // test bit 5
 x[5]

 // set bit 5
 x[5] |= 1;

 // reset bit 5
 x[5] ^= 1;


Creating multiple lists of named tuples based on condition I have created a function which is used to create a list of named tuples. The tuple which is created is dependent on the format of 'instructions' in which there are 3 different formats. Instructions is just a list of lists. I wanted to improve the readability/maintainability of this function and had 3 ideas in mind but not sure how to implement them.

Should the named tuple declarations live somewhere outside the function and be called in somehow?
Should list comprehensions be used?
Could this be set up using a factory pattern instead of using the if statements? How would that be done?

Ignoring the naming convention as it's just generic... any further feedback would be much appreciated.

instructions = [['1'],['2','bob','MATCH'],['3','alice','55']]

def assign_tuple_lists(instructions):
    """Creates named tuple lists from instructions"""

    Items = namedtuple('Items', 'timestamp, user, action')
    Cars = namedtuple('Cars', 'timestamp, user, reserve')
    Timing = namedtuple('Timing', 'timestamp')

    items = []
    cars = []
    timing = []
    for instruction in instructions:
        if len(instruction) < 2:
            timing.append(Timing._make(instruction))
        elif instruction[2] == 'MATCH':
            cars.append(Cars._make(instruction))
        else:
            items.append(Items._make(instruction))

    return timing, cars, items

Output:

timing = [Timing(timestamp = '1')]
cars = [Cars(timestamp = '2',user = 'bob',reserve = 'MATCH')]
timing = [Item(timestamp = '3',user = 'alice',action = '55')]

You can make a separate method that decided which type the instruction is:

def get_parser(instruction):
    if len(instruction) < 2:
        return Timing
    if instruction[2] == 'MATCH':
        return Cars
    return Items

and then just use that:

def assign_tuple_lists2(instructions):
    """Creates named tuple lists from instructions"""
    result  = defaultdict(list)
    for instruction in instructions:
        parser = get_parser(instruction)
        result[parser.__name__].append(parser(*instruction))

    return result

this uses a collections.defaultdict instead of separate lists per type

defaultdict(list,
            {'Timing': [Timing(timestamp='1')],
             'Cars': [Cars(timestamp='2', user='bob', reserve='MATCH')],
             'Items': [Items(timestamp='3', user='alice', action='55')]})


Lock-free producer/consumer implementation I know of BlockingCollection in .net, but this is rather for my understanding of the pattern. Is this implementation correct ?

Consumer

class Consumer
{
    readonly Queue<int> _q;

    public Consumer(Queue<int> q)
    {
        this._q = q;
    }

    public void Consume()
    {
        for (int i = 0; i < 10; i++)
        {
            while (_q.Count == 0)
            {
            }

            Console.WriteLine("consuming " + _q.Dequeue());
        }

    }
}

Producer

class Producer
    {
        readonly Queue<int> _q;
        readonly int _maxSize;

        public Producer(Queue<int> q, int maxSize)
        {
            this._q = q;
            this._maxSize = maxSize;
        }

        public void Produce()
        {
            for (int i = 0; i < 10; i++)
            {
                while (_q.Count == _maxSize)
                {
                }

                Console.WriteLine("producing " + (i + 1));
                _q.Enqueue(i + 1);
            }
        }
    }

Client

class Program
    {
        static void Main(string[] args)
        {
            Queue<int> q = new Queue<int>();
            var prod = new Producer(q, 10);
            var consumer = new Consumer(q);

            var producerThread = new Thread(new ThreadStart(prod.Produce));
            producerThread.Start();

            var consumerThread = new Thread(new ThreadStart(consumer.Consume));
            consumerThread.Start();
        }
    }

The way I see it, this handles the race condition in Inadequate implementation by introducing while instead of if

The program runs fine on my machine, so is this correct or am i missing something? why the need for synchronization?

The bigest problem is that code while(true){} will eat up all CPU that is available for the thread. Try running your program on a single core machine (it will run fine because you are only producing 10 items but try producing infinite number - they should still be consumed properly). The minimum change should be while(condition) Thread.Yield() or Thread.Sleep(). Yielding will still eat up all CPU but sleeping will put your thread down for longer than might be needed - this is why synchronization is used - to notify the other thread instead of making it sleep and try guessing when the new data will arrive.

More important issue that requires synchronization is that Queue.Enqueue() and Queue.Dequeue() will most probably not be atomic operations - what happens if you enqueue an item while it is dequeing? This is why classes like ConcurrentQueue have been created in .NET Framework.

Next problem is a situation with multiple consumers. Assume two consumers and one item in the queue. If both of them run at the same time, they will both at the same time detect that q.Count == 1. Now the first of them dequeues the only item from the queue. Now the second one tries to dequeue but there is nothing there anymore - it will throw an exception.


Placement of jQuery function that toggles p tags I have a page with lots of jquery.  There are lots of functions inside:

$( document ).ready(function() {
//lots of functions
});

After that "document ready" snippet closes, I have a function written like this:

$(function(){
        $('#number-of-coins input[type=radio]').change(function () {
            var numcoins=$("input[name=numbercoins]:checked").val();
            if (numcoins == "2") {
                $( "p.two" ).show();
                $( "p.three" ).hide();
            } else if (numcoins == "3") {
                $( "p.two" ).hide();
                $( "p.three" ).show();
            } else {
                $( "p.two" ).hide();
                $( "p.three" ).hide();
            }
        });
    });

What this does is not important, but it does work properly. My question is, would it be more efficient to take this section below (without the "function" wrapper) and move it into the "document ready" snippet?  Or does it not matter either way?  I did that as a test and it worked, so it would seem to me that every function should just go inside of the "document ready" snippet, since it's slightly less code not having to write the function wrapper each time.  Is that accurate or does it depend?

 $('#number-of-coins input[type=radio]').change(function () {
            var numcoins=$("input[name=numbercoins]:checked").val();
            if (numcoins == "2") {
                $( "p.two" ).show();
                $( "p.three" ).hide();
            } else if (numcoins == "3") {
                $( "p.two" ).hide();
                $( "p.three" ).show();
            } else {
                $( "p.two" ).hide();
                $( "p.three" ).hide();
            }
        });

Putting a function inside the jQuery function $(function(){}) is shorthand for $(document).ready(), they are the same thing.

So, the answer is: yes, it will be more efficient.

jQuery's .ready method just waits for the DOM to be ready before firing the code inside but when it comes to setting event handlers, you actually don't have to wait for the DOM to be ready at all. jQuery allows you to attach your events to the document object itself and when those events bubble up, jQuery will call them.

So instead of using

$('#number-of-coins input[type=radio]').change(function () { ... });

You can use

$(document).on("change", '#number-of-coins input[type=radio]', function(){...});

The second one does effectively the samething as the first one, but the first one needs to be inside of a $(function(){..}) or $(document).ready() and the second one does not.

For more info, see Difference between .on('click') vs .click().


Univariate linear regression from scratch in Python I am relatively new to machine learning and I believe one of the best ways for me to get the intuition behind most algorithms is to write them from scratch before using tons of external libraries.

This classifier I wrote seems to be yielding reasonable results based on the dataset I provided. This dataset is based on the number of hours that a student studied for a test (x), and the score this same student got in the test (y).

I tried to exploit OOP as much as I could, instead of using a procedural approach to write the algorithm.

Would you mind giving me your opinions and comments about this code? This is also important because I'll be adding to my portfolio. Are there some missing good practices in the code? What would you recommend keeping and removing in a professional setting or for life as a developer?

Univariate linear regression algorithm:

# Linear equation based on: y = m * x + b, which is the same as h = theta1 * x + theta0
import numpy as np

class LinearRegressionModel():
    """
    Univariate linear regression model classifier.
    """

    def __init__(self, dataset, learning_rate, num_iterations):
        """
        Class constructor.
        """
        self.dataset = np.array(dataset)
        self.b = 0  # Initial guess value for 'b'.
        self.m = 0  # Initial guess value for 'm'.
        self.learning_rate = learning_rate
        self.num_iterations = num_iterations
        self.M = len(self.dataset) # 100.
        self.total_error = 0

    def apply_gradient_descent(self):
        """
        Runs the gradient descent step 'num_iterations' times.
        """
        for i in range(self.num_iterations):
            self.do_gradient_step()

    def do_gradient_step(self):
        """
        Performs each step of gradient descent, tweaking 'b' and 'm'.
        """
        b_summation = 0
        m_summation = 0
        # Doing the summation here.
        for i in range(self.M):
            x_value = self.dataset[i, 0]
            y_value = self.dataset[i, 1]
            b_summation += (((self.m * x_value) + self.b) - y_value) # * 1
            m_summation += (((self.m * x_value) + self.b) - y_value) * x_value

        # Updating parameter values 'b' and 'm'.
        self.b = self.b - (self.learning_rate * (1/self.M) * b_summation)
        self.m = self.m - (self.learning_rate * (1/self.M) * m_summation)
        # At this point. Gradient descent is finished.

    def compute_error(self):
        """
        Computes the total error based on the linear regression cost function.
        """
        for i in range(self.M):
            x_value = self.dataset[i, 0]
            y_value = self.dataset[i, 1]
            self.total_error += ((self.m * x_value) + self.b) - y_value
        return self.total_error

    def __str__(self):
        return "Results: b: {}, m: {}, Final Total error: {}".format(round(self.b, 2), round(self.m, 2), round(self.compute_error(), 2))

    def get_prediction_based_on(self, x):
        return round(float((self.m * x) + self.b), 2) # Type: Numpy float.

def main():

    # Loading dataset.
    school_dataset = np.genfromtxt(DATASET_PATH, delimiter=",")

    # Creating 'LinearRegressionModel' object.
    lr = LinearRegressionModel(school_dataset, 0.0001, 1000)

    # Applying gradient descent.
    lr.apply_gradient_descent()

    # Getting some predictions.
    hours = [10, 20, 30, 40, 50, 60, 70, 80, 90, 100]
    for hour in hours:
        print("Studied {} hours and got {} points.".format(hour, lr.get_prediction_based_on(hour)))

    # Printing the class attribute values.
    print(lr)

if __name__ == "__main__": main()

Dataset snippet:

32.502345269453031,31.70700584656992
53.426804033275019,68.77759598163891
61.530358025636438,62.562382297945803
47.475639634786098,71.546632233567777
59.813207869512318,87.230925133687393
55.142188413943821,78.211518270799232
52.550014442733818,71.300879886850353
45.419730144973755,55.165677145959123

About OOP

  I tried to exploit OOP as much as I could, instead of using a procedural approach to write the algorithm.

Although I believe that your approach was fine, using OOP for the sake of OOP is something I would rather warn against. There is a talk about this here.

Comments

def __init__(self, dataset, learning_rate, num_iterations):
    """
    Class constructor.
    """

The comment Class constructor is redundant and unecessary, I would instead explain the parameters of __init__ in the doc string.

    self.M = len(self.dataset) # 100.

Is the # 100 saying that the len(self.dataset) is always going to be 100? It might be 100 in this case, but I highly doubt you can ensure that.

Default values

Have you considered putting default values for learning_rate and num_iterations? If we want a default of 100 and 0.001 for num_iterations and learning_rate respectively, you could rewrite __init__ like:

def __init__(self, dataset, learning_rate=0.001, num_iterations=100):

Private methods

Do you really want do_gradient_step(self) to be considered public? Yes, there are no "true" private methods, but the convention is to put one underscore before the name to indicate it is private. Honestly, I would just call it _step(self).

Indentation

if __name__ == "__main__": main()

should really be:

if __name__ == "__main__":
    main()

To comply with PEP 8.


Vigenere Cipher - make code more concise? I'm a beginner in programming overall, started about a month ago and so I'm still trying to fully grasph the concept of OOP, for instance. Here I used a class for the encryption and decryption processes, but everything else is in functions in the main part of the program (getting the cipher key, user message etc). What do you guys think? Is my implementation good? How could I improve it?

# Vigenere Cipher program

class Vigenere:
    """Vigenere Cipher program."""

    def __init__(self, message, key):
        self.message = message
        self.key = key

    def __str__(self):
        print("Vigenere Cipher program.")

    def __encrypt(self):
        encrypted = ""
        i = 0
        for letter in self.message:
            if ord(letter) + self.key[i] > 90:
                new_key = ord(letter) + self.key[i] - 90
                new_letter = chr(64 + new_key)
                encrypted += new_letter
                i += 1
                if len(self.key) == i:
                    i = 0
            else:
                encrypted += (chr(ord(letter) + self.key[i]))
                i += 1
                if len(self.key) == i:
                    i = 0
        return encrypted

    def __decrypt(self):
        decrypted = ""
        i = 0
        for letter in self.message:
            if ord(letter) - self.key[i] < 65:
                new_key = 65 - (ord(letter) - self.key[i])
                new_letter = chr(91 - new_key)
                decrypted += new_letter
                i += 1
                if len(self.key) == i:
                    i = 0
            else:
                decrypted += chr(ord(letter) - self.key[i])
                i += 1
                if len(self.key) == i:
                    i = 0
        return decrypted

def get_message():
    userMessage = ""
    final_message = ""
    flag = True
    while flag:
        userMessage = input("\nYour message: ")
        if len(userMessage) < 1:
            print("\nYou must enter something!")
            continue
        userMessage = userMessage.replace(" ", "")
        for i in range(len(userMessage)):
            if not userMessage[i].isalpha():
                continue
            else:
                final_message += userMessage[i]
        if len(final_message) < 1:
            continue
        else:
            flag = False
    return final_message.upper()

def get_key():
    userKey = ""
    keys = []
    final_keys = []
    flag = True
    while flag:
        userKey = input("\nYour key: ")
        if len(userKey) < 1:
            print("You must enter something!")
            continue
        userKey = userKey.replace(" ", "")
        for i in range(len(userKey)):
            if not userKey[i].isalpha():
                continue
            else:
                keys.append(ord(userKey[i].upper()))
                final_keys.append(keys[i] - 64)
        if len(final_keys) < 1:
            continue
        flag = False
    return final_keys

def encrypt_or_decrypt():
    userChoice = ""
    flag = True
    while flag:
        userChoice = input("\nEncrypt or decrypt? (E/D): ")
        if userChoice not in ("e", "E", "d", "D"):
            continue
        else:
            flag = False
    return userChoice

if __name__ == "__main__":
    flag = True
    while flag:
        userChoice = encrypt_or_decrypt()
        message = get_message()
        key = get_key()
        cipher = Vigenere(message, key)
        if userChoice in ("e", "E"):
            encrypted = cipher._Vigenere__encrypt()
            print(encrypted)
        else:
            decrypted = cipher._Vigenere__decrypt()
            print(decrypted)
        userInput = input("\nAgain? (Y/N): ")
        while userInput not in ("y", "Y", "n", "N"):
            userInput = input("\nAgain? (Y/N): ")
        if userInput in ("y", "Y"):
            continue
        elif userInput in ("n", "N"):
            print("Exiting.")
            flag = False
    input("\n\nPress the enter key to exit.")

As far as OOP languages go, Python has a very weak sense of encapsulation.

The only thing we do have are putting a single _ in front of a method or variable name, to signal that it is an internal variable/method and should not be used unless within the class itself and maybe derived classes. This is a pure convention, but should be understood similarly to protected in other languages.

In addition to this, when we put two underscores in front__ of the name, this signals that the variable/method is what we would call private in other languages. For these variables, Python does some name wrangling, so for some object a = A(), a.__x becomes accessible only as a._A__x. This should already signal that you should not normally use this from outside the class, these variables are definitely not part of the public API.

So, to put it shortly, just rename your __encrypt and __decrypt methods to encrypt and decrypt.

This has the added advantage that if you have a different encryption class (which you seem to do, called Caesar), the interface does not change. With both you can do cipher.encrypt(...) and cipher.decrypt(...), so the only thing you have to change when changing the encryption engine, is swap the actual objects.


Having a string representing items in a cart, elaborate taxes and totals. Interview task in Python A couple of weeks ago I had to do a coding challenge for a job interview. I did the challenge using Python, the code was working and I got the job. But I would like to know how (if possible) I can improve/refactor the code I've written.
The reviewer told my future employer there were too many if statements and that I could have written better code.
I guess this is an example of too many if statements:
def get_row_tax_percentage(product_name: str):
    exempt = is_exempt(product_name)
    imported = is_imported(product_name)
    if exempt and imported:
        return Decimal(".05")
    elif exempt and not imported:
        return Decimal("0")
    elif imported:
        return Decimal(".15")
    else:
        return Decimal(".1")

Link to the task description: https://github.com/xpeppers/sales-taxes-problem
Link to my solution: https://github.com/soulpaul/sales-taxes-solution
I think this could be a chance to learn something new. I don't need you guys to write code for me, but please point me to the right direction.
Thanks to anyone who is going to spend some time on this.

That really depends on how the task is stated. If it's stated like your code, just in English, with four different (and possibly independent) values, then I'd say your way is actually ok. Because then if one of them changes, you only need to change the value, not rewrite logic.
But the task description says:

Basic sales tax is applicable at a rate of 10% on all goods, except books, food, and medical products that are exempt. Import duty is an additional sales tax applicable on all imported goods at a rate of 5%, with no exemptions.

So it is in fact stated as two separate taxes that should be determined separately and just added. Make the code's logic reflect the real logic.
For extra clarity, use the proper names as well. For example:
def get_row_tax_percentage(product_name: str):
    basic_sales_tax = 10 if not is_exempt(product_name) else 0
    import_duty = 5 if is_imported(product_name) else 0
    return Decimal(basic_sales_tax + import_duty) / 100

Or maybe with 0 if is_exempt(product_name) else 10, I can't decide which I find clearer and/or closer to how the task says it.


Readable code with many components (Swing) Please help me to make more readable and simple code like this. What if I had 20 components? Is it right to organize code like this?

package newpackage.view;

import java.awt.GridLayout;
import javax.swing.JButton;
import javax.swing.JLabel;
import javax.swing.JPanel;
import javax.swing.JPasswordField;
import javax.swing.JRadioButton;
import javax.swing.JTextField;

public class UserPanel extends JPanel{

    private JLabel idLbl;
    private JLabel nameLbl;
    private JLabel passwordLbl;
    private JRadioButton adminRad;
    private JRadioButton userRad;
    private JTextField idFld;
    private JTextField nameFld;
    private JPasswordField passwordFld;
    private JButton submitBtn;

    public UserPanel() {
        setLayout(new GridLayout(4,2));
        idLbl = new JLabel("ID");
        nameLbl = new JLabel("Name");
        passwordLbl = new JLabel("Password");
        adminRad = new JRadioButton("Admin");
        userRad = new JRadioButton("User");
        idFld = new JTextField();
        nameFld = new JTextField();
        passwordFld = new JPasswordField();
        submitBtn = new JButton("Submit");
        add(idLbl);
        add(idFld);
        add(nameLbl);
        add(nameFld);
        add(passwordLbl);
        add(passwordFld);
        add(adminRad);
        add(userRad);
        add(submitBtn);
    }
}

Try to NOT add each UI component in Class fields. Keep then local if possible, as close to usage as possible. Too much class fields does not make class easy readable - as reader think that all of them are do matter.

JLabel and other components, content(text) of which will never be changed, are good candidates to be declared as local variable or without variable at all:

add(new JLabel("text"));


Enum vs int wrapper struct I am writing code that has objects having integer member variables where the integer value has a specific constant meaning, which normally means "use an enum".

Suppose the values are the days of the week, Monday = 0, Tuesday = 1, and so on.  (In my real case, there are several different "types" involved, some of which have 10-20 possible values.)

The code uses data objects which are auto-generated, and these data objects can only have certain, already specified types (int, string, bool, double), otherwise they could just use enums themselves.  The question is, should I use an enum, or a struct with static readonly definitions and implicit casting.  I favor the struct version.

Here is an example of using an enum:

enum Days
{
    Monday = 0,
    Tuesday = 1,
    Wednesday = 2,
    Thursday = 3,
    Friday = 4,
    Saturday = 5,
    Sunday = 6,
}

And here is my preferred struct code:

struct Day
{
    readonly int day;

    public Day(int day)
    {
        this.day = day;
    }
    public static implicit operator int(Day value)
    {
        return value.day;
    }
    public static implicit operator Day(int value)
    {
        return new Day(value);
    }

    public static readonly Day Monday = 0;
    public static readonly Day Tuesday = 1;
    public static readonly Day Wednesday = 2;
    public static readonly Day Thursday = 3;
    public static readonly Day Friday = 4;
    public static readonly Day Saturday = 5;
    public static readonly Day Sunday = 6;
}

My code frequently involves assigning values to the members of the data object.  This means if I use the struct version, I can say for example Day myDay = dataObject.Day rather than Days myDay = (Days)dataObject.Day.  With the amount of logic code that will be affected by this, I think the struct version is a win for readability because it removes a lot of explicit casts.

I don't want to just use plain ints in my code because I like function signatures that indicate the purpose of the "int" being passed or returned.  But I don't want the messiness that comes from casting enums all over.

I'm asking for arguments against the struct version.  Would it disgust you if you found this pattern in code somewhere?

Clarification:

For the data objects to allow user types, other code which depends on the data objects would have to know about these types.  The two "areas" of code are logically separate, and do not reference each other at all.  I suppose a middle layer that separates the data objects from my code would be possible, but since all it would do is convert the enums to ints and back, it would be an awful lot of code to accomplish very little.

Another reason that these values sometimes must be represented as integers is that they are put through arithmetic operations sometimes.  This is another place where explicit casting just gets annoying.

Finally, the definition of the possible values (days of week, in this example) is far less likely to change than the logic code.  So conciseness and readability in the definition code isn't as valuable as in the logic code.

Normally it would disgust me, but you appear to have a valid reason to use it, which stems from the pre-existing disgusting situation that you have to cope with these data objects that use ints.

The only thing I would ask is, why can these data objects not use enums? What is it about their auto-generation that precludes enums from being used?  I do not think that enums receive any special handling at the IL level, they are handled just like primitive types are.


Function to determine what color shirt somebody should wear to a football game depending on what seat they have I was tasked with writing a function to find what color shirt someone should wear to our college football game depending on what seat they sit in. Here is the function I came up with:

function getColor(section, row) {
  const colorA = "orange";
  const colorB = "white";
  section = section.toUpperCase();

  /* Case 1: Section is a special section */
  if (section.includes("TER") || section.includes("WC") || section.includes("EC")) {
    /* Numbers in special sections are guaranteed to be single digit,
       So just grab the last character in the section. */
    const number = section.slice(-1);
    // Even numbers are colorB, odd are colorA
    return (number % 2 === 0) ? colorB : colorA;
  }
  /* Case 2: Section is a single letter (No numbers) */
  else if  (section.length === 1) {
    /* A through Q have different row numbers than R through W */
    const section_divider = getSectionDivider(section);

    /* Odd-lettered (A, C, etc.) sections are colorA when above
       the section divider. Even-lettered section are colorB when
       above the section divider. */
    if (section.charCodeAt(0) % 2 === 0) {
      return (row >= section_divider) ? colorB : colorA;
    } else {
      return (row >= section_divider) ? colorA : colorB;
    }
  }
  /* Case 3: Section is two letters (No numbers) */
  else if (section.length == 2 && section.charAt(0) >= 'A' && section.charAt(0) <= 'Z'  &&
           section.charAt(1) >= 'A' && section.charAt(1) <= 'Z') {
    /* Double lettered sections are always the same color as the
       single letter's bottom row */
    return getColor(section.charAt(0), 0);
  }
  /* Case 4: Section is a single letter followed by a number */
  else if (!isNaN(section.slice(1))) {
    /* Row is WC */
    if (isNaN(row)) {
      /* WC rows should just match the section below them */
      return getColor(section, 0);
    }

    /* X3 and Z15 are special cases where the whole section isn't one
       color. They are both colorB above the section divider and colorA below. */
    if (section === "X3" || section === "Z15") {
      const section_divider = 16; // Just in case it changes in the future.
      return (row <= section_divider) ? colorA : colorB;
    } else {
      /* This is the number. (i.e. X4 => 4, Z13 => 13) */
      const number_in_section = parseInt(section.slice(1));
      return (number_in_section % 2 === 0) ? colorA : colorB;
    }
  }
  /* Case 5: Section is a double letter followed by a number */
  else {
    /* Double-letter-number sections are always the opposite of the
       single-letter version of its self's top row.
       (i.e. XX13 is whatever X13's top row isn't) */
    return getColor(section.slice(1), 1000) === colorA ? colorB : colorA;
  }
}

Is there a better way of solving this problem? Is the code clear?

I see 2 points where you can work to increase the quality of your code.

simplify some conditions check
get rid of comments

simplify some conditions check

Consider the following code:

/* Case 3: Section is two letters (No numbers) */
else if (section.length == 2 && section.charAt(0) >= 'A' && section.charAt(0) <= 'Z'  &&
       section.charAt(1) >= 'A' && section.charAt(1) <= 'Z') {

I think you could make it shorter with just a regular expression:

else if (section.match(^[A-Z]{2}$)) {

It is not just short and fast to read, but, in this case, it show you what is the "rule" that the string should satisfy.

get rid of comments

Your code is full of comments, and that made it difficult to reed.

Consider that many developer, me included, don't trust the comments and read it just when something weird happened. And in this case too there are very few times the comments helps.

The reason is you should update the comments as you update the code, but this never happened as it is more work and you don't have time.

Compilers or interpeter never care the comments.

The other point is that the developers could read and understand the code, and your effort should put in made the code readable for human beings.

Let me show some examples.

  /* Case 1: Section is a special section */
  if (section.includes("TER") || section.includes("WC") || section.includes("EC")) {

Consider the followhing change. As you can see you don't need the comment any more.

if (isASpecialSection(section)) {

...

function isASpecialSection(section) {
    return section.includes("TER") || section.includes("WC") || section.includes("EC");
}

Another example:

/* Case 2: Section is a single letter (No numbers) */
else if  (section.length === 1) {

Even the condition is short, you should use the same approach, as it is not clear in which case you are here.

Look at the comment, it "lied", because you just check that the string have length 1, not that is only letters. Maybe you don't have the case today, but data could change.

So remove the comment and change the check:

else if (section.match(/^[A-Z]$/)) {

Not all comments are bad, you should avoid those comments that tell what the code does, because you can read the code and so it is a waste of time for the developers that read your code.

Too many comments like in your code make difficult to read the code.
To put all those comments you break the indentation.

Try to make your code more readable by using proper names or functions to encapsulate complex conditions.

Naming is hard and take time, I know, but it pay back.


Tree containing different data types For a while I was thinking of a way I could have multiple objects of different types that related to each other be included in a tree.

For example, say I have CEO, Manager, Representative which all implement interface IEmployee.

I have a tree:

class ObjectNode<T> : IEnumerable<ObjectNode<T>>
{
    private object _obj;
    private Type _type;
    private readonly ICollection<ObjectNode<T>> _children = new LinkedList<ObjectNode<T>>();

    public ObjectNode<T> Parent { get; set; }

    public ObjectNode(T data)
    {
        _obj = data;
        _type = data.GetType();
    }

    public ICollection<ObjectNode<T>> Children
    {
        get { return _children; }
    }

    public ObjectNode<T> AddChild(T data)
    {
        ObjectNode<T> childNode = new ObjectNode<T>(data) { Parent = this };
        _children.Add(childNode);
        return childNode;
    }

    public void AddChildren(params T[] children)
    {
        int len = children.Length;
        for (int i = 0; i < len; ++i)
        {
            AddChild(children[i]);
        }
    }

    // The generics are necessary here in order to properly typecast object.
    // CS0693: Type parameter "T" has the same name as the type paramater from outer type "File.ObjectNode<T>"
    #pragma warning disable 0693
    public T Object<T>()
    {
        return (T)_obj;
    }
    #pragma warning restore 0693

    public new Type GetType()
    {
        return _type;
    }

    // IEnumerable implementation.
}

I can't do var employee = new ObjectNode<IEmployee>(new CEO()); and
then use employee.CEOMethod(); because employee is of type IEmployee.
I can't make employee ObjectNode<CEO> because then AddChild(T)
will be expecting type CEO. And that limits my data type to only one.
It also will not properly take my methods, since it'll be
expecting methods under File.ObjectNode<File.CEO>.

And say I wanted to add a child of CEO, the subordinates, which are of type Manager. How did I solve this problem? I added a "layer of abstraction."

If I wanted to access a CEO method, I could just do:
employee.Object<CEO>().CEOMethod(); and it would do as I need how I need.

If I wanted to add children to CEO, I could use:
var manager = employee.AddChild(new Manager { Field = 1; }) and do this X amount of times, loop through employee.Children and type check using GetType() like so:

foreach (var child in employee.Children)
        {
            if (child.GetType() == typeof(Manager))
            {
                child.Object<Manager>().ManagerMethod();
            }
            if (child.GetType() == typeof(Representative))
            {
                child.Object<Representative>().RepresentativeMethod();
            }
        }

Or simply manager.Object<Manager>.DoSomething();.

My design also allows me to have multitudes of types of IEmployee under CEO, not just Manager. CEO can have children such as AccountManager, TeamManager, etc., and the same would apply to AccountManager and TeamManager's children.

Am I doing something horribly wrong? Is this design bad? Is there a better approach that's just as flexible and allows me to traverse it like a tree? I'm trying to achieve a more efficient "data group" design implementation for entities that subordinate other entities. I feel this is almost "hacky."

NOTE: The code here is the real implementation, but the use of "Manager", "Employee", and "Representative" were examples of how it can be used.

Remarks

What's the point of using an interface and a generic collection if you're only going to call type-specific methods? Instead of having a ManagerMethod, RepresentativeMethod, and so on, add a single EmployeeMethod to that interface and let each class provide their own implementation. Doing type checks all over the place doesn't scale very well, and makes it difficult to add more types.
Why does ObjectNode<T> implement IEnumerable<ObjectNode<T>>? It already exposes its children via the Children property.
ICollection<T> provides modification methods, so other code can add nodes without going through your AddChild/AddChildren methods, so you can't guarantee that all parents are set properly. You'll want to use IReadOnlyCollection<T> or IReadOnlyList<T> here.
Related: the Parent property's setter is public, but it doesn't do anything to inform the previous and new parents about the change. Other code could easily mess up a tree.
Do you have a particular reason for using a linked list instead of a 'normal' list?
That object _obj field should be of type T.
There is no need to explicitly store the type of the given data. Just call its GetType() method when you need to (if at all).
obj.GetType() == typeof(Class) can also be written as obj is Class (with the added benefit that it won't throw an exception when obj is null).
Why does ObjectNode<T> override the GetType() method to return the type of its content instead of its own type? That sort of surprising behavior makes code harder to understand and maintain, and could cause subtle bugs.
That AddChildren method could use a foreach loop instead of a for loop.
Don't ignore warning 0693, use meaningful type parameter names instead. T is used a lot when there's only one type parameter, but when there are multiple it's often used as a prefix instead (for example: IDictionary<TKey, TValue>, Func<T1, T2, TResult>).

Alternative approach

A generic tree class doesn't seem to suit this particular example. You end up with less than ideal names (Subordinates and Superior make a lot more sense here than Children and Parent). It's also cumbersome to use, with lots of type checking and casting required.

I don't know what specific requirements you're working with (if any), but I'd probably go for something like this:

interface IEmployee
{
    // Assuming each employee only has a single boss:
    IEmployee Superior { get; }

    // Employees without subordinates just return an empty collection:
    IEnumerable<IEmployee> Subordinates { get; }

    // Anything else that's common to employees goes here
}

together with an (abstract) base class that implements superior/subordinate storage. Just keep things clean and simple, there's no need for wrapping things in a separate tree node class. Note that this interface is essentially a tree node, and can easily be traversed with a recursive method.

Also, rather than doing lots of type checks and calling type-specific methods, I'd make as much use of that interface as possible. What's the point of an interface if you're not going to treat those objects similarly anyway?

Finally, it looks like this was just an example, and that you're actually looking for a general solution to a broader problem. I get the impression that you're either trying to generalize too soon (a rule of thumb I once heard recommended to generalize only after seeing a similar problem trice) or that you're trying to over-engineer this. Is there a concrete problem that you want to solve?


Easier way to find matching array from a lot of arrays I have the following arrays which save the MAC-adresses of a lot of bluetooth beacons and their positions in a building:

private static final String[][] MAC_ADDRESSES_A_BLUETOOTH = {{"B09122F5D126", "210.105"}, {"B09122F5F26B", "136.121"}, {"B09122F5F619", "181.172"}};
...
private static final String[][] MAC_ADDRESSES_Q_BLUETOOTH = {{"B09122F5D124", "210.105"}, {"B09122F5F26Z", "136.121"}, {"B09122F5F61Q", "181.172"}};

Then I'm looking for the matching triple based on a current triple:

private String[][] compareArrays(String[] beacon) {
        if (Arrays.equals(beacon, getFirstArray(MAC_ADDRESSES_A_BLUETOOTH))) {
            return MAC_ADDRESSES_A_BLUETOOTH;
        } else if (Arrays.equals(beacon, getFirstArray(MAC_ADDRESSES_B_BLUETOOTH))) {
            return MAC_ADDRESSES_B_BLUETOOTH;
        } else if (Arrays.equals(beacon, getFirstArray(MAC_ADDRESSES_C_BLUETOOTH))) {
            return MAC_ADDRESSES_C_BLUETOOTH;
        } else if (Arrays.equals(beacon, getFirstArray(MAC_ADDRESSES_D_BLUETOOTH))) {
            return MAC_ADDRESSES_D_BLUETOOTH;
        } else if (Arrays.equals(beacon, getFirstArray(MAC_ADDRESSES_F_BLUETOOTH))) {
            return MAC_ADDRESSES_F_BLUETOOTH;
        } else if (Arrays.equals(beacon, getFirstArray(MAC_ADDRESSES_G_BLUETOOTH))) {
            return MAC_ADDRESSES_G_BLUETOOTH;
        } else if (Arrays.equals(beacon, getFirstArray(MAC_ADDRESSES_H_BLUETOOTH))) {
            return MAC_ADDRESSES_H_BLUETOOTH;
        } else if (Arrays.equals(beacon, getFirstArray(MAC_ADDRESSES_I_BLUETOOTH))) {
            return MAC_ADDRESSES_I_BLUETOOTH;
        } ...
        } else {
            return null;
        }
}

This code looks horrible and it bugs me. Is there an easier way to do this without having to write 16 if statements? getFirstArray returns all first values from a two-dimensional array.

For readability, use an Object instead of a 2d array.

E.G:

public class BluetoothMacAddress {
    private String macAddress;
    private float positionInBuilding;
}

Use java.util.List, then you can have a list of lists:

private sttaic final List<MacAddress> MAC_ADDRESSES_A_BLUETOOTH = Arrays.asList(
    new BluetoothMacAddress("B09122F5D126", "210.105"),
    new BluetoothMacAddress("B09122F5F26B", "136.121"),
    new BluetoothMacAddress("B09122F5F619", "181.172"));

private static final List<List<MAC_ADDRESSES>> MAC_ADDRESSES = Arrays.asList(
    MAC_ADDRESSES_A_BLUETOOTH,
    MAC_ADDRESSES_B_BLUETOOTH
    ...
);

Your new method would then look something like this:

private MacAddress compareArrays(String[] beacon) {
    for (List<MacAddress> macAddresses : MAC_ADDRESSES ) {
        if (Arrays.equals(beacon, getFirstArray(macAddresses))) {
            return macAddresses;
        }
    }
    // not found
    return null;
}

You'd have to change the beacon parameter to be a MacAddress too, or convert the MacAddress to a String[] (or visa-versa).


LINQ query to select subjects of interest Here's the query:

 using (var db = CreateContext())
        {
            // performing the check for HasBeenAdded inline here, is this only one db call?
            return db.Subjects.Select(s => new SubjectDto(s){HasBeenAdded = db.Interests.Any(x => x.SubjectId == s.SubjectId)}).ToList();
        }

Basically I create a DTO from a subject and then populate a property of that DTO (HasBeenAdded) based on whether or not that entry's foreign key exists in another table.  Is this the right way to go?

In terms of "SQL" what you need is a "Left Outer Join". So I would suggest this:

using (var db = CreateContext())
{
    var subjectDtos = from subject in Subjects
                      join interest in Interests on subject.SubjectId equals interest.SubjectId into si
                      from interest in si.DefaultIfEmpty()
                      select new SubjectDto { Subject = subject, HasBeenAdded = interest != null };
    return subjectDtos.Distinct().ToList();
}

And here is why:

I think that LINQ query is more readable in 'SQL like' format.
With the join clause you retrieve only the "subjects" that have an "interest". But with potential duplications if more than one "interest" has the ID of the same "subject" (For this reason I introduced the "Distinct" in the query, you can remove if the duplicate results interest you).
As a matter of consistency I suggest you rather than use the constructor, initialize the property directly.
For "DefaultIfEmpty" steatment docs go here. The Left Outer Join is there.

UPDATE My first code does not work. Fixed and tested now :)


Select the correct input when using input group after pressing a button Introduction: I'm creating a form using bootstrap and I have different input groups with the same structure and functionality.

All of them have a '-' button on the left and a '+' button on the right of the input field and, when one of them is pressed, the input value has to change.

Problem: As the buttons are inside a div with class="input-group-btn" and they are not siblings of the input field, I don't know which is the best way to select it.

I have managed to make it work but I'm not sure if it's the best way to do it.

JavaScript and HTML

$(".increase").click(function() {
  var amount = parseInt($(this).parents('.input-group').children('input').val());
  if (amount < 20) {
    $(this).parents('.input-group').children('input').val(amount + 1);
  }
});

$(".decrease").click(function() {
  var amount = parseInt($(this).parents('.input-group').children('input').val());
  if (amount > 0) {
    $(this).parents('.input-group').children('input').val(amount - 1);
  }
});
<form class="form-inline">

        <button type="button" class="btn btn-default decrease">
        </button>

      <input type="text" class="form-control" id="capacity" value="0" disabled="">

        <button type="button" class="btn btn-default increase">
        </button>

        <button type="button" class="btn btn-default decrease">
        </button>

      <input type="text" class="form-control" id="rooms" value="0" disabled="">

        <button type="button" class="btn btn-default increase">
        </button>

</form>

Bootply: http://www.bootply.com/EA5a48SCdg

If the input group perhaps contains additional input elements this won't work. I think that you should bind the button to the input field by declaring a new attribute to the button which is connected to the input's id. Like so:

<button type="button" class="btn btn-default increase" data-bind="rooms"></button>

It now contains the input ID which this button is connected to. Now in your JavaScript try something like this:

$(".increase").click(function() {
  var input = $('#' + $(this).data('bind'));
  input.val(input.val++);
});

It will use the data-bind attribute to connect to the specific input element and increase this value by 1 (++).


Check repository for a nested entity I have a Product entity and a ProductCategory entity. Given the Product entity, my application checks the database via a repository to see if the ProductCategory already exists. If the category is not found in the database, then the application should persist it.

Currently, I have this code in the service layer as follows:

public class ProductCategoryMapper implements DataMapper<ProductCategory> {

    @NonNull
    private final ProductCategoryRepository categoryRepository;

    @Override
    public void map(Product product) {
        ProductCategory category = productCategoryRepository.findByProductTagAndMake(product);

        if (category == null) {
           category = productCategoryRepository.save(new ProductCategory(product.getTag(), product.getMake()));
        }
        product.setProductCategory(category);
    }
}

But should this method be in the Product class as:

public void mapCategory(); // or perhaps setCategory();

This seems to be better OO but then the Product entity would have a reference to the repository (which I think is bad from a design / DDD perspective b/c the entity knows about repositories). Should the code stay in the service layer, be moved to the entity, or is there some other option?

It appears that this Product entity is meant to be a POJO.  POJOs should be responsible only for holding the relevant data in memory to be accessed by, and possibly manipulated by, someone else.

The second you put in logic that is not directly related to the POJO, such as persistence, the flexibility of your application drops like a stone.  You will be unable to use this POJO in other contexts where the repository will not be relevant, or worse, when this comes from a different repository altogether (it does happen!).


Generate an ID number for each new row of text that is pasted into the worksheet This code checks if "B2" and so on is filled in and if it is then it will generate a new ID number for the row of data that was transferred to the worksheet on the next empty row, I know this code creates opportunities for bugs so I want to make it better but I don't really know how I could do it. (PS: I am very bad at loops so if the solution is a loop please word yourself as if you were talking to a newcomer

'Creating a variable for all the inputs from the textboxes
Dim inputs As Variant
    inputs = Array(TextBox1.Text, TextBox2.Text, TextBox3.Text, TextBox4.Text, TextBox10.Text, TextBox6.Text, TextBox7.Text, TextBox8.Text, TextBox9.Text)
    ' shipFrom, shipTo, shipDate, NP, desc, gramEx, tareWeight, weight, dims

'Declaring a variable for the next available row where the inputs are going
Dim nextRowB As Range
    Set nextRowB = Sheets("Arkiv").Range("B" & Rows.Count).End(xlUp).Offset(1, 0)
    nextRowB.Resize(1, UBound(inputs) + 1).Value = inputs

'Delaring ID number for each row as a variable
Dim ID As Range
    Set ID = Sheets("Arkiv").Range("A" & Rows.Count).End(xlUp).Offset(1, 0)
    If IsEmpty(Sheets("Arkiv").Range("B2").Value) Then 'How can I make this line better? I know this way of doing it creates oppertunities for many bugs

        MsgBox "Please fill in the <Ship From> box"

    Else

        Sheets("Arkiv").Range("A2").Value = "1"

    End If

    If Not IsEmpty(Sheets("Arkiv").Range("C2").Value) Then

        ID = Sheets("Arkiv").Range("A" & Rows.Count).End(xlUp).Value + 1

    End If

    Sheets("Arkiv").Activate
    MsgBox "The ID number for this data is: " & Sheets("Arkiv").Range("A" & Rows.Count).End(xlUp).Value

Creating an Unique ID Without Loops

The problem with incrementing the next ID based on the last cell in a column is you will have to sort the data to ensure that it is in ID order.  This really can take away from the user experience.    A better approach would be to use the WorksheetFunction.Max() to find the current max ID.

"I am very bad at loops"

You should start by watching these videos:

Excel VBA Introduction Part 5 - Selecting Cells (Range, Cells, Activecell, End, Offset)
Excel VBA Introduction Part 15 - Do Until and Do While Loops
Excel VBA Introduction Part 15a - Find and FindNext
Excel VBA Introduction Part 16 - For Next Loops
Excel VBA Introduction Part 17 - For Each Loops

Notice that the first video teaches you how to define ranges.  Well,  If you are having problems with loops in Excel then chances are you are having problems defining the range to loop over.

The best advice that I can give you when you are having problems with a coding pattern is to practice it.  Printout a worksheet with several different types of loops and every morning choose a couple of the patterns and type them up several times until you can recite them from memory.  Another great way to practice is to start answering questions on Stackoverflow.com.  This will challenge you with unique situations that you would not normally see.  Your answers will get progressively better or time as you respond to the comments from other users.

Miscellaneous

TextBox1, TextBox2 TextBox3 ... ugh!!  In the time it takes you to the write comments to describe what each one does, you could have just gave them meaningful names.  I generally preface all my textboxes with txt like this txtShipFrom, txtShipTo, txtShipDate but simply using shipFrom, shipTo, shipDate also works.

Sheets("Arkiv") is used 6 times.  Using With  Sheets("Arkiv") will not only make you code easier to read, modify, and debug but is also more efficient (the compiler only has to resolve the reference 1 time).

Refactored code:

Sub AddNewShippingRow_Click()
    Dim inputs As Variant
    Dim ID As Long
    Dim nextRow As Range

    ID = getNextShippingID
    Set nextRow = getNextShippingRow

    inputs = Array(ID, shipFrom.Text, shipTo.Text, shipDate.Text, NP.Text, desc.Text, gramEx.Text, tareWeight.Text, Weight.Text, dims.Text)

    nextRow.Resize(UBound(inputs) + 1).Value = inputs
End Sub

Function getNextShippingRow() As Range
    With Sheets("Arkiv")
        Set getNextShippingRow = .Cells(.Rows.Count, 1).End(xlUp).Offset(1)
    End With
End Function

Function getNextShippingID() As Long
    With Sheets("Arkiv")
        getNextShippingID = WorksheetFunction.Max(.Range("A1", .Cells(.Rows.Count, 1).End(xlUp))) + 1
    End With
End Function

In my refactored code I simplified the main subroutine by using helper functions to perform some of the tasks.  The fewer tasks that a subroutine performs the easier it is to read, modify and debug.

For instance say that you wanted to change the way you create your ID's because we are using a helper function we can modify and test that function without have to run the larger block of code.

Here I modify and tested 4 different versions getNextShippingID() without having to make any changes to the main routine.  Of course, I would have to change the datatype of the main routines ID variable but that is not to after I completed my testing.

Function getNextShippingID() As Double
    getNextShippingID = CLng(Date) & (Timer * 100)
End Function

Function getNextShippingID() As Double
    Const StartDate As Date = #1/1/2019#
    getNextShippingID = CLng(Date - StartDate) & (Timer * 100)
End Function

Function getNextShippingID() As Double
    getNextShippingID = Format(Now, "ddmmyyhhmmssnn")
End Function

Function getNextShippingID() As String
    Dim s As String
    s = CreateObject("Scriptlet.TypeLib").GUID
    getNextShippingID = Mid(s, 2, Len(s) - 4)
End Function


Encrypting/decrypting messages in java I am trying to implement message encryption/decryption with password in java. Here is the code I'm using:
import java.security.MessageDigest;
import java.security.spec.InvalidKeySpecException;
import java.util.Base64;
import javax.crypto.Cipher;
import javax.crypto.SecretKey;
import javax.crypto.SecretKeyFactory;
import javax.crypto.spec.IvParameterSpec;
import javax.crypto.spec.PBEKeySpec;
import javax.crypto.spec.SecretKeySpec;

public class Cryptography {

  public static final int SECRET_KEY_ITERATIONS = 45928;
  public static final int SALT_ITERATIONS = 11879;
  public static final int IV_ITERATIONS = 13275;
  public static final int KEY_LENGTH = 256;

  private static Cipher cipher;
  private static MessageDigest sha256;
  private static SecretKeyFactory secretKeyFactory;

  static {
    try {
      cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
      sha256 = MessageDigest.getInstance("SHA-256");
      secretKeyFactory = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA256");
    } catch (Exception e) {
      e.printStackTrace();
    }
  }

  public static String encryptText(String password, String plaintext) throws Exception {
    byte[] passwordBytes = password.getBytes();
    SecretKey secretKey = getKeyFromPassword(password, getSaltFromPassword(passwordBytes));
    IvParameterSpec ivParameterSpec = getIvFromPassword(passwordBytes);
    cipher.init(Cipher.ENCRYPT_MODE, secretKey, ivParameterSpec);
    return Base64.getEncoder().encodeToString(cipher.doFinal(plaintext.getBytes()));
  }

  public static String decryptText(String password, String ciphertext) throws Exception {
    byte[] passwordBytes = password.getBytes();
    SecretKey secretKey = getKeyFromPassword(password, getSaltFromPassword(passwordBytes));
    IvParameterSpec ivParameterSpec = getIvFromPassword(passwordBytes);
    cipher.init(Cipher.DECRYPT_MODE, secretKey, ivParameterSpec);
    return new String(cipher.doFinal(Base64.getDecoder().decode(ciphertext)));
  }

  private static SecretKey getKeyFromPassword(String password, byte[] salt)
      throws InvalidKeySpecException {
    PBEKeySpec spec = new PBEKeySpec(password.toCharArray(), salt, SECRET_KEY_ITERATIONS, KEY_LENGTH);
    byte[] bytes = secretKeyFactory.generateSecret(spec).getEncoded();
    return new SecretKeySpec(bytes, "AES");
  }

  private static byte[] getSaltFromPassword(byte[] passwordByteArray) {
    byte[] bytes = sha256.digest(passwordByteArray);
    for (int i = 0; i < SALT_ITERATIONS; i++) {
      bytes = sha256.digest(bytes);
    }
    return bytes;
  }

  private static IvParameterSpec getIvFromPassword(byte[] passwordByteArray) {
    byte[] bytes = sha256.digest(passwordByteArray);
    for (int i = 0; i < IV_ITERATIONS; i++) {
      bytes = sha256.digest(bytes);
    }
    byte[] ivBuffer = new byte[16];
    System.arraycopy(bytes, 0, ivBuffer, 0, 16);
    return new IvParameterSpec(ivBuffer);
  }
}

Is this a secure way to do encryption/decryption, or does it have serious issues?

private static byte[] getSaltFromPassword(byte[] passwordByteArray) {
    byte[] bytes = sha256.digest(passwordByteArray);
    for (int i = 0; i < SALT_ITERATIONS; i++) {
      bytes = sha256.digest(bytes);
    }
    return bytes;
  }

this part is an NO NO. salt has to be random. please refer this
old question
The salt has to be independent and random else it is useless.


Simplify splitting a String into alpha and numeric parts Requirement:  Parse a String into chunks of numeric characters and alpha characters.  Alpha characters should be separated from the numeric, other characters should be ignored.

Example Data:

Input           Desired Output
1A              [1, A]
12              [12]
12G             [12, G]
12ABC-SFS513    [12, ABC, SFS, 513]
AGE+W#FE        [AGE, W, FE]
-12WE-          [12, WE]
-12- &%3WE-     [12, 3, WE]

Question:

The code below accomplishes this.  However, I am looking for any suggestions as to a better way to accomplish this (maybe a crazy regex using String.split()? ) or any changes that could make this code more readable/easy to follow.

Code:

private static String VALID_PATTERN = "[0-9]+|[A-Z]+";

private List<String> parse(String toParse){
    List<String> chunks = new LinkedList<String>();
    toParse = toParse + "$"; //Added invalid character to force the last chunk to be chopped off
    int beginIndex = 0;
    int endIndex = 0;
    while(endIndex < toParse.length()){
        while(toParse.substring(beginIndex, endIndex + 1).matches(VALID_PATTERN)){
            endIndex++;
        }
        if(beginIndex != endIndex){
            chunks.add(toParse.substring(beginIndex, endIndex));
        } else {
            endIndex++;
        }
        beginIndex = endIndex;
    }
    return chunks;
}

First of all, yes there is a crazy regex you can give to String.split:

"[^A-Z0-9]+|(?<=[A-Z])(?=[0-9])|(?<=[0-9])(?=[A-Z])"

What this means is to split on any sequence of characters which aren't digits or capital letters as well as between any occurrence of a capital letter followed by a digit or any digit followed by a capital letter. The trick here is to match the space between a capital letter and a digit (or vice-versa) without consuming the letter or the digit. For this we use look-behind to match the part before the split and look-ahead to match the part after the split.

However as you've probably noticed, the above regex is quite a bit more complicated than your VALID_PATTERN. This is because what you're really doing is trying to extract certain parts from the string, not to split it.

So finding all the parts of the string which match the pattern and putting them in a list is the more natural approach to the problem. This is what your code does, but it does so in a needlessly complicated way. You can greatly simplify your code, by simply using Pattern.matcher like this:

private static final Pattern VALID_PATTERN = Pattern.compile("[0-9]+|[A-Z]+");

private List<String> parse(String toParse) {
    List<String> chunks = new LinkedList<String>();
    Matcher matcher = VALID_PATTERN.matcher(toParse);
    while (matcher.find()) {
        chunks.add( matcher.group() );
    }
    return chunks;
}

If you do something like this more than once, you might want to refactor the body of this method into a method findAll which takes the string and the pattern as arguments, and then call it as findAll(toParse, VALID_PATTERN) in parse.


Determine whether there is a winner in tic-tac-toe game Below a function which answers: is there a winner in tic-tac-toe game?

Also, there is a test suite I wrote for it.

def is_win(field):
    # check horizontal
    N = len(field)
    for i in range(N):
        if field[i][0] != 0 and len(set(field[i])) == 1:
            return True
        vertical = [field[j][i] for j in range(N)]
        if vertical[0] != 0 and len(set(vertical)) == 1:
            return True
    # check diagonal
    diagonal = [field[i][i] for i in range(N)]
    if diagonal[0] != 0 and len(set(diagonal)) == 1:
        return True
    o_diagonal = [field[N-i-1][i] for i in range(N)]
    if o_diagonal[0] != 0 and len(set(o_diagonal)) == 1:
        return True
    return False

assert is_win(
        [
            [0, 0, 0],
            [2, 2, 2],
            [0, 0, 0],
            ]) == True

print(1)
assert is_win(
        [
            [0, 2, 0],
            [0, 2, 0],
            [0, 2, 0],
            ]) == True
print(2)
assert is_win(
        [
            [0, 0, 2],
            [2, 2, 1],
            [0, 0, 1],
            ]) == False
print(3)

assert is_win(
        [
            [2, 0, 0],
            [0, 2, 0],
            [0, 0, 2],
            ]) == True
print(4)
assert is_win(
        [
            [0, 0, 2],
            [0, 2, 0],
            [2, 0, 0],
            ]) == True
print(5)
```

Your code is WET.

var[0] != 0 and len(set(var)) == 1

Is repeated four times.
field[len(field)-i-1] can be simplified to field[~i].
I'd personally just use a couple of any and a couple of ors.

def is_unique_player(values):
    return values[0] != 0 and len(set(values)) == 1

def is_win(field):
    N = len(field)
    return (
        any(is_unique_player(row) for row in field)
        or any(is_unique_player(column) for column in zip(*field))
        or is_unique_player([field[i][i] for i in range(N)])
        or is_unique_player([field[~i][i] for i in range(N)])
    )


CRUD operation class This code is a chunk of big project, so I am sorry in advance for not adding everything as it's just not possible.

[DataContract]
public class Companionship
{
    [DataMember]
    public int ID;
    [DataMember]
    public string ClientID;
    [DataMember]
    public string UserSID;
    [DataMember]
    public string Role;
    [DataMember]
    public string Status;
    [DataMember]
    public string StartDate;
    [DataMember]
    public string EndDate;
    [DataMember]
    public bool AddedInBP;

    public int Add(Companionship pC)
    {
        int clientID = -1;
        var gda = new USDataAccess();
        if (string.IsNullOrEmpty(pC.ClientID))
            clientID = new gDataAccess().GetClientByURL(SPContext.Current.Web.Site.Url).ClientID;
        else
            clientID = Convert.ToInt32(pC.ClientID);

        pC.Status = "Active";
        DateTime? dtEnd = string.IsNullOrEmpty(pC.EndDate) ? (DateTime?)null : DateTime.Parse(pC.EndDate);
        DateTime dtStart = string.IsNullOrEmpty(pC.StartDate) ? DateTime.Now : DateTime.Parse(pC.StartDate);
        gda.AddCompanionship(clientID, pC.UserSID, pC.Role, pC.Status, dtStart, dtEnd);
        new DirectoryGroup().Add(clientID, pC.UserSID, "ChangeMe In Companionship");

        var cDetails = new Company().Get(clientID);
        if (cDetails != null && !string.IsNullOrEmpty(cDetails.CompanyID) && pC.AddedInBP)
        {
            SendApprovalRequest();
        }

        return 1;
    }

    public int AddCollection(Companionship[] pCs)
    {
        foreach (var c in pCs)
            this.Add(c);

        return 1;
    }

    private void SendApprovalRequest()
    {
        string url = "https://portal.gov.com/CompanyManagement/_vti_bin/sync/sync.svc";
        ICSync ws = Utils.GetProxy<ICSync>(url);

        var user = Utils.ADUserToObject(ADUser.LoadBySid(this.UserSID));
        user.Companionships[0] = this;
        var dd = Utils.UserToDirectoryGroupDetails(user);

        ws.AddDirectoryGroup(SPContext.Current.Web.Site.Url, SPContext.Current.Web.CurrentUser.Email, dd);
    }
}

This code is in a utilities project, it's called from WCF services to interact with Companionships, it adds them to the database with some business logic and sends a message to another web service.

Looking at the structure I think this can be improved.

I am calling it like this:

 new Companionship().AddCollection(pCPs);

I am more worried about the way I am interacting with Companionship class.

Naming

I won't list every bad name but you need to think more carefully about the names you choose.

ID should be Id Microsoft have very specific guidelines for this.

  The two abbreviations that can be used in identifiers are ID and OK. In Pascal-cased identifiers they should appear as Id, and Ok. If used as the first word in a camel-cased identifier, they should appear as id and ok, respectively.

Now on to some more of your names in general:

public int Add(Companionship pC) - What is pC? Full name please. I'd suggest one but I actually haven't been able to figure out what it might mean.

var gda = new USDataAccess(); Again - What is gda? I can't make a suggestion again...

new gDataAccess() - Types should be Pascal case and not start with a prefix.

dtEnd - Don't abbreviate in names - endDate is much more readable.

cDetails - wouldn't companyDetails be easier to read?

To be honest, 80% of your names are less than ideal - it's an easy thing to fix though!

Weirdness

This looks very odd to me:

new Company().Get(clientID)

I'd expect to be able to get a company by id without newing up a Company

Company.GetByClientId(clientId);

Note that would be even better on another class - e.g. CompanyRepository.

What on earth does this do:

new DirectoryGroup().Add(clientID, pC.UserSID, "ChangeMe In Companionship");

You don't use the result so I can only assume this persists something somewhere - that's not obvious at all from the code.

Parsing

This sort of code:

DateTime? dtEnd = string.IsNullOrEmpty(pC.EndDate) ? (DateTime?)null : DateTime.Parse(pC.EndDate);

Is fragile, you might have a string but that's no gauranteee that it's a valid date. You should also specify a culture or a pattern.

Other things

Why are you using public fields? Make them properties.

Why are you returning 1 from your methods? It's C# you don't need to return an exit code. Throw an exception if something goes wrong and if you don't need a return value, set the return type of the method as void.


Visualized bubble sort Recently I created a small app using JavaScript, and I would love for some JavaScript developers if they have the time of day, to tell me their opinions so I can grow and improve.

I'm mostly working on back-end tasks with PHP and Go, but I thought making something small in JS would be fun attempt.

Basically it's an app that sorts an image based on users choice of sorting algorithm. You first say how many rows and columns want, and past an image URL. The app then shuffles it and asks you what sort do you want.

It is then visual when sorting it back to original.

I know this is not a typical question, but I would love to get some opinions on code, and look of app. So, what do you think? Any recommendation ?

GitHub of project:

https://github.com/fvukojevic/Image-Sorting-Visualizer (if someone wants to star  ) Application: https://image-sorting-visualizer.herokuapp.com/

Example of one sorting algorithm where I'm saving every swap into an array of "animations":

function bubbleSort(arrayCopy, animations) {
    let sorted = false;
    let counter = 0;
    while(!sorted){
        sorted = true;
        for(let i=0;i<arrayCopy.length -1 - counter; i++) {
            if(arrayCopy[i].val > arrayCopy[i+1].val){
                swap(i, i+1, arrayCopy);
                sorted = false;
                animations.push([i, i+1]);
            }
        }
        counter++
    }
}

A short review

This code passes jshint perfectly, well done. Besides the obvious warning that you did not provide swap of course
The name arrayCopy is perhaps unfortunate, since this works on any array whether it is a copy or not
The name bubbleSort also does not perfectly describe what is going on, it not only bubble sorts the array, but it also tracks the swaps in an animations array
I keep thinking that providing the animations array in the function call is weird, I would return the animations instead, the caller can concat if needed
I see little point in providing an array with i and i+1 unless you animate with different kind of sorting algorithms, so you could just animations.push(i)
Still, very maintainable, correct code.


Enumerating new articles whose publication start date has passed (cn.PublishEnd == null || cn.PublishEnd < DateTime.Now)

vs

(cn.PublishEnd ?? DateTime.MinValue < DateTime.Now)

Which is more readable?
I'm inclined towards the second form, but something tells me I'm wrong.

Context:

namespace Damnation.Website.Main.Business.Extensions
{
    public static class CommunityNews
    {
        public static IEnumerable<DAL.CommunityNews> Published(this ObjectSet<DAL.CommunityNews> table)
        {
            return table.Where(cn => cn.PublishStart > DateTime.Now && DateTime.Now > (cn.PublishEnd ?? DateTime.MinValue)).OrderByDescending(cn => cn.PublishStart);
        }
    }
}

Types

DateTime PublishStart
DateTime? PublishEnd

As for me, the first form is more readable because it clearly describe the condition. It's easy to read and understand. The second form is a little confusing. The first thought I had was: What is the DateTime.Zero? Is this a default value for PublishEnd? If yes then why not to use the DateTime.MinValue? Maybe it's not a default value and I get it wrong somewhere? Of course these thoughts took only seconds but nevertheless.
In general I think that ?? should be used to return the default value with expected type. The default value of the  PublishEnd should be of DateTime type. But the second form distorts the meaning of ?? and returns bool type instead, and it’s confusing.

Edited:
If you want to read is like you said: "if now is greater than (end date or earliest date)"
then you should write it like:

(DateTime.Now > cn.PublishEnd ?? DateTime.MinValue)

And this, third form is better than first and second.


Project Euler #52 - Permuted multiples It can be seen that the number, 125874, and its double, 251748, contain exactly the same digits, but in a different order.
Find the smallest positive integer, x, such that 2x, 3x, 4x, 5x, and 6x, contain the same digits.

The following code works on SpiderMonkey 1.8.5. But didn't work on my nodejs.
It takes about two seconds, and gives the correct answer.
My two major concerns are my style. And my choice in functions.
Personally I think str_sorted is horrible, int -> str -> array -> str.
// Like Python's range.
function range(start, stop, step){
    if (typeof(stop)==='undefined'){
        stop = start;
        start = 0;
    }
    if (typeof(step)==='undefined') step = 1;
    if (stop === null){
        while (true){
            yield start;
            start += step;
        }
    }else{
        for (number = start; number < stop; number += step){
            yield number;
        }
    }
}

function str_sorted(num){
    num = num.toString();
    arr = Array();
    for (index in num){
        arr.push(num[index]);
    }
    return arr.sort().join('');
}

function permuted(num){
    tmp = str_sorted(num);
    return tmp == str_sorted(num * 2) &&
           tmp == str_sorted(num * 3) &&
           tmp == str_sorted(num * 4) &&
           tmp == str_sorted(num * 5) &&
           tmp == str_sorted(num * 6);
}

function find(iterable, fn){
    for (num in iterable){
        if (fn(num)){
            return num;
        }
    }
}

print(find(range(1, null), permuted));

The way that I run this code is via the command line.
$ js p52.js
142857
$

This runs the the SpiderMonkey 1.8.5 interpreter. I got it from the Arch repository at some point.

Overall, your code looks pretty decent. The indentation is spot-on, the variable names make sense, the code is clear and easy to read. But there are some big issues with your code.

The first thing that popped in my eyes was this:

if (typeof(stop)==='undefined'){

Please, don't use typeof to check if it is defined or not. You have a very interesting object called arguments.

Try this instead:

if (arguments.length < 3){

A few lines below, you have this:

if (stop === null){

What if I pass undefined or false?

You need to predict those, since they are valid values and, somewhat, in context. And yes, it is possible to pass undefined to a function, in one of these ways:

range(1, undefined, 3); //simply pass undefined
range.call({}, 1, undefined, 3); //mostly the same
range.apply({}, [1,,3]); //it may happen!
//... more similar variants ...

You need to take care of those.
This is one of a few examples.

You have the following function:

function str_sorted(num){
    num = num.toString();
    arr = Array();
    for (index in num){
        arr.push(num[index]);
    }
    return arr.sort().join('');
}

Let's focus on the following bit:

arr = Array();

There are 2 wrong things here:

This is a local variable without var
You are using the Array constructor as a function
You either use [] or use new Array()

The fix is simple:

function str_sorted(num){
    var arr = [];
    num = num.toString();
    for (index in num){
        arr.push(num[index]);
    }
    return arr.sort().join('');
}

But you still have something there that's bothering me:

    num = num.toString();
    for (index in num){
        arr.push(num[index]);
    }

Why are you converting it to a string and then iterating it? Is it to sort the numbers in the string? If that's the case, here's the whole function:

function str_sorted(num){
    return num.toString().split('').sort().join('');
}

Using string.split('') will split a string by each character, making an array of numbers.
Let's check your permuted() function.

It also suffer from a tiny flaw:

function permuted(num){
    tmp = str_sorted(num);
    return tmp == str_sorted(num * 2) &&
           tmp == str_sorted(num * 3) &&
           tmp == str_sorted(num * 4) &&
           tmp == str_sorted(num * 5) &&
           tmp == str_sorted(num * 6);
}

I have no idea what you are trying to do, but here is the mistake:

tmp = str_sorted(num);

There, you forgot a var.

Change it to this:

var tmp = str_sorted(num);

Lets take a look at the find function.

This one isn't too bad:

function find(iterable, fn){
    for (num in iterable){
        if (fn(num)){
            return num;
        }
    }
}

Just more of the same:

for (num in iterable){

You forgot a var there.

Change it to this:

for (var num in iterable){

I'm not sure if this is part of the code to be reviewed or not.

But let's analyze the last line:

print(find(range(1, null), permuted));

Please, don't use print. It isn't even a standard! It's just supported in some consoles (like Chrome). Use console.log() instead.

For now, I will leave the review as is. If I find any more issues, in the future, I will check on them.


Script to determine the outcome(s) of any number of dice rolls I think this code has far to many if else clauses and not enough abstraction via functions. I would like to break the code down into a series of small functions, or methods if i decide to go OOP, but I'm not sure of the best way to do so. If you brilliant Pythonista's could guide me in the write direction I would appreciate it.

Here is the GitHub repo if you would like to fork it repo

'''
script to determine the outcome(s) of any number of dice rolls
By Andrew Smith (2015)
'''

from random import randint

def dice_roller(dice, rolls, sides):
    '''
    Add doc string here.
    '''
    show_di = '|   |'
    show_di_1 = '| * |'
    show_di_2 = '|* *|'
    show_di_3 = '|***|'
    show_di_t = ' ___ '
    show_di_b = ' --- '
    roll_counter = 0
    di_counter = 0

    while roll_counter < rolls:
        roll_counter += 1
        di_counter = 0
        print " "

        while di_counter < dice:
            di_counter += 1
            num_side = randint(1, sides)
            print "Roll %r, Di %r: %r" % (roll_counter, di_counter, num_side)
            if num_side <= 6: # print di symbol if di is six-sided
                if num_side == 1:
                    print show_di_t
                    print show_di_1
                    print show_di
                    print show_di_b
                elif num_side == 2:
                    print show_di_t
                    print show_di_1
                    print show_di_1
                    print show_di_b
                elif num_side == 3:
                    print show_di_t
                    print show_di_1
                    print show_di_2
                    print show_di_b
                elif num_side == 4:
                    print show_di_t
                    print show_di_2
                    print show_di_2
                    print show_di_b
                elif num_side == 5:
                    print show_di_t
                    print show_di_2
                    print show_di_3
                    print show_di_b
                elif num_side == 6:
                    print show_di_t
                    print show_di_3
                    print show_di_3
                    print show_di_b
    print " "
#-------------------------------------------------------------------------------

TITLE = """
 __________________
| Dice Roller v1.1 |
| by Andrew Smith  |
|__________________|
"""

MENU = """
Enter the number of dice to roll, the number
of rolls, and the number of sides per di in the
following format: #dice #rolls #sides

--- or ---

Select from the following presets:

a. 3 man attack (Risk)
b. 2 man attack or defend (Risk)
c. 1 man attack or defend (Risk)

--- or ---

Type 'menu' to repeat this menu

--- or ---

Type 'quit' to quit
        """

def main():
    '''
    Explain this function.
    '''
    print TITLE
    print MENU
    again = 0

    while again != 1: # prompts the user until 'quit' is entered
        roll_choice = raw_input("> ")
        if roll_choice.isalpha(): # if input starts with a letter
            if roll_choice == 'a':
                dice_roller(3, 1, 6)
            elif roll_choice == 'b':
                dice_roller(2, 1, 6)
            elif roll_choice == 'c':
                dice_roller(1, 1, 6)
            elif roll_choice == 'menu':
                print MENU
            elif roll_choice == 'quit':
                again = 1
            else:
                print "Invalid choice.\n"

        elif roll_choice[0].isdigit(): # if input starts with a number
            choices = roll_choice.split()
            size = len(choices)
            if size == 3:
                one = int(choices[0])
                two = int(choices[1])
                three = int(choices[2])
                dice_roller(one, two, three)
            else:
                print "Invalid Choice.\n"
        else:
            print "Invalid choice.\n"

if __name__ == '__main__':
    main()

Use unpacking

        choices = roll_choice.split()
        size = len(choices)
        if size == 3:
            one = int(choices[0])
            two = int(choices[1])
            three = int(choices[2])
            dice_roller(one, two, three)
        else:
            print "Invalid Choice.\n"

Becomes:

dice_roller( * map(int, roll_choice.split()) )

Adding error handling it is:

 try:
     dice_roller( * map(int, roll_choice.split()) )
 except Exception as e:
     print("Invalid choice:" + e)

The * operator just goes from a list to free floating arguments and is called the unpacking operator. You use it to pass a list as argument to a function that takes separate arguments.

Naming in full

You should write the names of your variables in full to make them more understandable at first glance. show may be omitted. For example:

show_di_1 ➡ dice_1
show_di_b ➡ dice_bottom

A dictionary

A dictionary will allow you to avoid the first if elif chain easily. How is left as an exercise for the reader.


Exceptions to control data read flow I previously posted a Reservoir-Sampling program, which was basically a test version of this one. This is an assignment.  In the code below, I use RunTimeException to control when scanner finishes reading data. Reasons for this being:

I cannot use any other library than java.lang.
I cannot use Scanner, either.  We have been provided a StdIn class which has a static scanner.next() method only for reading strings.

Note: All the code is inside main() because that is how the API demands it.

Can someone review my code, and possibly tell if there's some way I can improve this?

import java.io.BufferedInputStream;
import java.util.Scanner;

public class Subset {

    public static void main(String[] args) {
        int k = Integer.parseInt(args[0]);
        String[] arr = new String[k];
        int i = 0;
        String str;
        while((str=stdin.readString())!=null && i < arr.length){
            arr[i++] = str;
            // System.out.println(str);
        }
        try{
            for(; (str=stdin.readString())!=null; i++){
                int r = (int)(Math.random()*(i + 1));
                if(r < k){
                    arr[r] = str;
                    //System.out.println(str);
                }
            }
        }catch(RuntimeException e){ // do nothing }
        for(String s : arr){
            System.out.print(s + " ");
        }
        System.out.println("\n");
    }
}
class stdin{
    private stdin(){}
    private static Scanner sc = new Scanner(new BufferedInputStream(System.in));
    public static String readString(){
        return sc.next();
    }
}

Command line input: echo A B C D E F G H I J | java Subset 3
Output: A F C

To clear out confusion, I am posting what the API and the instructor demands:

Subset client. Write a client program Subset.java that takes a command-line integer k,
reads in a sequence of N strings from standard input using StdIn.readString(),
and prints out exactly k of them, uniformly at random. Each item from the sequence can be printed out at most once.
You may assume that k = 0 and no greater than the number of string on standard input.

% echo A B C D E F G H I | java Subset 3       % echo AA BB BB BB BB BB CC CC | java Subset 8
C                                              BB
G                                              AA
A                                              BB
                                               CC
% echo A B C D E F G H I | java Subset 3       BB
E                                              BB
F                                              CC
G                                              BB
Your client should use only constant space plus one object either of type Deque or of type RandomizedQueue;
use generics properly to avoid casting and compiler warnings. It should also use time and space proportional to
at most N in the worst case, where N is the number of strings on standard input.
(For an extra challenge, use space proportional to k.) It should have the following API.

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

Finished in the comments, can you post the part of the assignment question pertinent to your limited library usage and main method only code? Anyway, if you have been told that a RTE will signify the end of the reading (rather than the null) then this seems marginally better as it makes it clear why you are doing what you are doing:

while (true) {
    try {
        str = stdin.readString();
        if (str == null) {
            break;
        }
        int r = (int)(Math.random()*(i++ + 1));
        if(r < k){
            arr[r] = str;
        }
    }
    catch(RuntimeException rte) {
        //Log the crazy break condition
        break;
    }
}

It is always a discussion point whether while(true) is acceptable and is normally held up against using a running loop variable. In this case you can intialise a boolean to true and use it as your while condition - setting it to false where the breaks are. It involves extra checks, and makes for harder to read code in my eyes, but hey!

Now, give your variables meaningful names.

EDIT: Post clarification in the comments

So importantly you do not know the length of the input but you want to randomly sample it, and a RumtimeException is unexpected and can make your code blow up? In which case why anymore complex than this?:

public class Subset {

    public static void main(String[] args) {
        int sampleSize = Integer.parseInt(args[0]);
        String[] samples = new String[sampleSize];
        int current = 0;
        String str;

        //make sure you have at least sampleSize number of samples
        while((str=stdin.readString())!=null && current < sampleSize){
            samples[current++] = str;
        }

        //this will read until the end of the input
        while((str=stdin.readString())!=null) {
            int randomIndex = (int)(Math.random()*(current++));
            if(randomIndex < sampleSize){
                samples[randomIndex] = str;
            }
        }

        for(String sample : samples){
            System.out.print(sample + " ");
        }
        System.out.println("\n");
    }
}

EDIT post comments
In your original question on Reservoir Sampling it was important to add +1 to you possible random number as you wanted to pick a value from an array of known size without excluding the final element . However in this version you are using the random number to dictate (with decreasing probability of a hit) where to write the next value read from Stdin into your array of samples.

If you know that the Stdin implementation and that it will through an Exception and that is the only thing you have been told or advised will signify an end to the data (no special characters, no nulls, no maximum length, etc) then the only thing that you can do is catch and handle it and that is perfectly acceptable - in this instance it is not really an Exception condition, it is the norm! Maybe he exercise is in working with bad APIs (which is very common)!

In the latest (very different) version of your code you have this:

for(; ; i++){
    int randomIndex = (int)(Math.random()*(i + 1));
    if(randomIndex < sampleSize){
        sample[randomIndex] = StdIn.readString();
    }
 }

You should replace this with code more like my first example where the exception state is handled within the loop. So:

Replace for loop with while loop
Because you are using the randomIndex to determine whether to read a value at all into the array you are no longer random sampling, you are just random inserting. Every value read from the pool will get written to the array. It is also important that this could run for a long time or more likely run to an error state when i > MAX_INT, which would be possible with a large dataset and a small sample.


Merge sort implementation in python 3 I have implemented a Merge sort in Python 3, and it works well. If anything needs to be improved, I would appreciate the criticism.

def merge_sort(nums):

    if len(nums) == 1:
        return

    middle_index = len(nums) // 2

    left_half = nums[:middle_index]
    right_half = nums[middle_index:]

    merge_sort(left_half)
    merge_sort(right_half)

    i = 0
    j = 0
    k = 0

    while i<len(left_half) and j<len(right_half):
        if left_half[i] < right_half[j]:
            nums[k] = left_half[i]
            i = i + 1
        else:
            nums[k] = right_half[j]
            j = j + 1

        k = k + 1

    while i<len(left_half):
        nums[k] = left_half[i]
        k = k + 1
        i = i + 1

if __name__ == "__main__":

   nums = [-3,-2,-1,1,2,1,0,-1,-2,-3]
   merge_sort(nums)
   print(nums)

Everything I'm going to talk about is in the merge_sort function

General

i = 0
j = 0
k = 0

Can be defined as

i = j = k = 0

You should always leave spaces between operators as per PEP 8 rules

i<len(left_half) should be i < len(left_half)

Use x += y instead of x = x + y

In my opinion, I think using short and concise names such as mid or middle instead of middle_index would be better. If you don't wish to, you can leave it as it!

Use type hints

Add docstrings

Bug

Your function only takes into account the left_half of the array, and ignores what's left in the right_half

For example, if nums array was [3, 9, 0], The array would be [0, 3, 0]

This would happen as

merge_sort([3]) which won't change the left_half
merge_sort([9, 0]) which would make the right_half as [0, 9]

Then,

left_half = [3]
right_half = [0, 9]

nums = [3, 9, 0]

i = 0
j = 0
k = 0

First, the else statement would be called as 3 > 0.

i = 0
j = 1
k = 1

nums = [0, 9, 0]

Next, the if statement would be called as 3 < 9

i = 1
j = 1
k = 2

nums = [0, 3, 0]

Now, the while loop will terminate as i = len(left_side)

Then, while i < len(left_side) would immediately terminate as i = len(left_side)

Did you notice? right_side still has one element 9 waiting to be traversed, but it never will be.

To fix that, add the following to the end of the function

while j < len(right_half):
    nums[k] = right_half[j]
    j += 1
    k += 1

Improvement

Now, instead of using a while loop at all, you can just use a[k:] = left_half[i:] + right_half[j:] to replace both the loops! This is true because one half must be empty and the other half must have the length of n - k.

Performance

If you are using this function in real time with an array of a really large size, this won't work efficiently.

len takes quite a bit of time. To make it even faster, use a parameter length which would be the length of the array

The final implementation of the function:

from typing import List, Any

def merge_sort(nums: List[Any], length: int) -> None:
    """ Uses Merge Sort to sort an array """

    # Base case
    if length == 1:
        return

    mid = length // 2

    left, right = mid, length - mid

    left_half, right_half = nums[:mid], nums[mid:]

    merge_sort(left_half, left)
    merge_sort(right_half, right)

    i = j = k = 0

    while i < left and j < right:
        if left_half[i] < right_half[j]:
            nums[k] = left_half[i]
            i += 1
        else:
            nums[k] = right_half[j]
            j += 1

        k += 1

    nums[k:] = left_half[i:] + right_half[j:]

Note: Any in typing means any datatype is allowed. The function can sort any datatype that is comparable with another element of the same datatype.


Refactoring a String Calculator I've refactored the string calculator kata as much as I could and tried to maintain single responsibility. Is there anything I could factor differently? The specs can be reviewed here.

module StringCalculator
  def self.add(string)
    string.end_with?("\n") ? fail('ends in newline') : solve(string)
  end

  def self.solve(string)
    verify(string)
    custom = delimiter(string)
    numerics = string.gsub(/\n/, custom).split(custom).map(&:to_i)
    numerics.reject { |n| n > 1000 }.reduce(0, :+)
  end

  def self.delimiter(string)
    string[0, 2] == '//' ? string[2, 1] : ','
  end

  def self.verify(string)
    find = string.scan(/-\d+/)
    fail("negatives not allowed: #{find.join(', ')}") if find.any?
  end

  private_class_method :verify, :delimiter, :solve
end

More importantly, I've thought of a case that involves the delimiter of a string that begins with // to be a number (which is considered invalid). I've made a quick way of solving that issue like so. Any suggestions on that?

Invalid case:

def self.delimiter(string)
  string[0, 2] == '//' ? check(string) || string[2, 1] : ','
end

def self.check(string)
  fail("invalid delimiter: #{string[2, 1]}") if string[2, 1] =~ /[0-9]/
end

I think your code is pretty good.  The main opportunity for improved clarity I saw was to consolidate your validation code into one place.  In your code, validations are sprinkled across various methods, so that you are mixing the responsibility of "validation" and "work".

Here's a refactoring (not tested) which puts all the validation in one place, and gets it out of the way up front.  That way, the actual "work" you are doing becomes simpler to express, because you know your inputs are clean and meet all your assumptions at that point.

Also, note that you don't need to specify 0 as the first argument of reduce.

module StringCalculator

  def self.add(string)
    verify(string)
    delim = delimiter(string)
    numerics = string.gsub(/\n/, delim).split(delim).map(&:to_i)
    numerics.reject { |n| n > 1000 }.reduce(:+)
  end

  #######################
  ## Verification Code
  #######################

  def self.verify(string)
    verify_doesnt_end_with_newline(string)
    verify_no_negatives(string)
    verify_custom_delim(string) if has_custom_delim?(string)
  end

  def self.verify_doesnt_end_with_newline(string)
    fail('ends in newline') if string.end_with?("\n")
  end

  def self.verify_no_negatives(string)
    find = string.scan(/-\d+/)
    fail("negatives not allowed: #{find.join(', ')}") if find.any?
  end

  def self.verify_custom_delim(string)
    custom_delim = get_custom_delim(string)
    fail("invalid delimiter: #{custom_delim}") if custom_delim =~ /[0-9]/
  end

  #######################
  ## Delimiter Utilities
  #######################

  def self.delimiter(string)
    return ',' unless has_custom_delim?(string)
    get_custom_delim(string)
  end

  def self.has_custom_delim?(string)
    string[0, 2] == '//'
  end

  def self.get_custom_delim(string)
    string[2, 1]
  end

  private_class_method :verify, :verify_doesnt_end_with_newline, :verify_no_negatives, :verify_custom_delim, :delimiter, :has_custom_delim?, :get_custom_delim
end