Discrete random variable generator There is my SSCCE to generate a value of discrete random variable.

values is set of value the RV can take and procents is equivalent to discrete pdf.

Can you anticipate any issue with this snippet?

import java.util.Random;

public class RandomTest {
    public static void main (String[] args) throws Exception {
        RandomTest rt = new RandomTest();

        int[] values = {0, 1, 2};
        int[] procents = {30, 60, 10};

        for (int i=0; i < 10; i++) {
            System.out.print(rt.discreteRV(values, procents) + " ");
        }
    }

    public int discreteRV (int[] values, int[] procents) throws Exception {
        if (values == null || procents == null) throw new Exception("Input parameters are null");
        if (values.length != procents.length) throw new Exception("Input parameters length mismatch");

        int sumProcents = 0;
        for (int i=0; i < procents.length; i++) {
            if (procents[i] < 0) throw new Exception("Negative procents are not allowed");
            sumProcents += procents[i];
        }

        if (sumProcents != 100) throw new Exception("Sum of procents is not 100");

        int rand = new Random().nextInt(100);

        int left = 0, right = 0;
        for (int i=0; i < procents.length; i++) {
            right += procents[i];
            if (rand >= left && rand < right)
                return values[i];
            left = right;
        }

        throw new Exception("");
    }
}

In general, this looks ok, i.e. it works.
I will put some notes on the lines of your method and will provide my suggestion after this.

Spelling: procent is not an English word. Well, the compiler does not care, but the next programmer probably will.

public int discreteRV(final int[] values, final int[] procents) throws Exception {

To throw a exception of type Exception does not provide any help. The purpose of exceptions is to communicate errors.
Noone knows the type of error from Exception and can do anything about it. They should be avoided in throw statements.

    if (values == null || procents == null)
        throw new Exception("Input parameters are null");

While checking your contract is a good idea, you do not gain anything here.
Java will throw a Nullpointer error with nearly the exact same message anyway if you try to access them.
As long as there are no plans to do any special things, I would not waste lines on this.

    if (values.length != procents.length)
        throw new Exception("Input parameters length mismatch");

I would throw a runtime exception. IllegalArgumentException looks suitable here.

    int sumProcents = 0;
    for (final int procent : procents) {
        if (procent < 0)
            throw new Exception("Negative procents are not allowed");
        sumProcents += procent;
    }

For exception: same as above.
For check: This is not valid for all input.
Consider (and/or try:) final int[] procents = { 1234567890, 1234567890, 1825831616 };
If you want to check the input, check for both sides.

    if (sumProcents != 100)
        throw new Exception("Sum of procents is not 100");

For exception: same as above.

    final int rand = new Random().nextInt(100);

If you use this method frequently, make it static (and/or even ThreadLocalRandom)

    int left = 0, right = 0;
    for (int i = 0; i < procents.length; i++) {
        right += procents[i];
        if (rand >= left && rand < right)
            return values[i];
        left = right;
    }

The left check is not needed.
If you check for something in between, support your readers:
Instead of rand >= left && rand < right try to read left <= rand && rand < right

    throw new Exception("");

For exception: same as above. A IllegalStateException looks suitable here.

}

Suggestion:

public int discreteRV(final int[] values, final int[] percentages) {
    if (values.length != percentages.length)
        throw new IllegalArgumentException("values.length != percentages.length");

    int sumPercentages = 0;
    for (int i = 0; i < percentages.length; ++i) {
        if (percentages[i] < 0)
            throw new IllegalArgumentException("Negative percentages are not allowed: percentages[" + i + "] = " + percentages[i]);
        sumPercentages += percentages[i];
        if (sumPercentages > 100)
            throw new IllegalArgumentException("Sum > 100");
    }

    if (sumPercentages != 100)
        throw new IllegalArgumentException("Sum of percentages is not 100");

    final int randomIntUpTo100 = random.nextInt(100);
    int threshold = 0;
    for (int i = 0; i < percentages.length; i++) {
        threshold += percentages[i];
        if (randomIntUpTo100 < threshold)
            return values[i];
    }
    throw new IllegalStateException("No value found. rand:" + randomIntUpTo100);
}

More ideas:
The arguments (arrays) could be replaced by an Object. This could make the handling easier. Depends on the use case.
The two loops could be combined into one loop. You will loose the check for sum == 100 then.


RC4 implementation in Go I'm new to Go, and as a learning project I've been implementing RC4, attempting to follow pseudo-code in the Wikipedia links (and trying not to look at the far-superior version in the crypto package).

Some questions:

Wikipedia suggests computing new array indexes using mod 256 at several points throughout the implementation.  However, mod 256 results in index out of bound errors or "constant 256 overflows byte".  What am I missing?
The crypto package returns a custom "Cipher" type, which has the rc4 function implemented on it.  Is this idiomatic Go (returning an objecty type) or something specific for the rc4 implementation?
any suggestions to improve clarity/speed/accuracy?

Source:

package main
import "fmt"

func ksa(keystring string) ([256]byte) {
  var key = []byte(keystring)
  var s [256]byte
  var j byte

  for i := 0; i < 255; i++ {
    s[i] = byte(i)
  }
  for i := 0; i < 255; i++ {
    j = (j + s[byte(i)] + key[i % len(key)]) % 255
    s[j], s[i] = s[i], s[j]
  }
  return s
}

func rc4(bufferstring string, key [256]byte) ([]byte) {
  buffer := []byte(bufferstring)
  res := make([]byte, len(buffer))
  var x, y, k, r byte = 0, 0, 0, 0

  for i, b := range(buffer) {
    x = (x + 1) % 255
    y = (y + key[x] % 255)
    y, x = x, y
    k = key[(key[x] + key[y]) % 255]
    r = b ^ k

    fmt.Printf("i: %d, b: %c, x: %#X, y: %#X, k: %#X, r: %#X  \n", i, b, x, y, k, r)
    res[i] = r
  }
  return res
}

func main() {
  keystring := "Secret"
  plaintext := "Attack at dawn"
  key := ksa(keystring)

  encrypted := rc4(plaintext, key)

  unencrypted := rc4(string(encrypted), key)

  fmt.Println("Encrypted")
  fmt.Printf("hex: %#X\n", string(encrypted))
  fmt.Printf("string: %s\n", string(encrypted))

  fmt.Println("Un-Encrypted")
  fmt.Printf("hex: %#X\n", string(unencrypted))
  fmt.Printf("string: %s\n", string(unencrypted))
}

Here is a simple translation of the Wikipedia RC4 key-scheduling algorithm (KSA) and pseudo-random generation algorithm (PRGA) into Go.

package main

import "fmt"

// Key-scheduling algorithm (KSA)
func ksa(key []byte) []byte {
    s := make([]byte, 256)
    for i := range s {
        s[i] = byte(i)
    }
    j := byte(0)
    for i := range s {
        j += s[i] + key[i%len(key)]
        s[i], s[j] = s[j], s[i]
    }
    return s
}

// Pseudo-random generation algorithm (PRGA)
func prga(message, s []byte) []byte {
    text := make([]byte, len(message))
    i, j := byte(0), byte(0)
    for m := range message {
        i++
        j += s[i]
        s[i], s[j] = s[j], s[i]
        k := s[s[i]+s[j]]
        text[m] = message[m] ^ k
    }
    return text
}

// RC4 stream cipher
// http://en.wikipedia.org/wiki/RC4
func rc4(message, key []byte) []byte {
    return prga(message, ksa(key))
}

func main() {
    testRC4 := []struct {
        key, plain []byte
    }{
        {[]byte("Key"), []byte("Plaintext")},
        {[]byte("Wiki"), []byte("pedia")},
        {[]byte("Secret"), []byte("Attack at dawn")},
    }
    for _, test := range testRC4 {
        cipher := rc4(test.plain, test.key)
        fmt.Printf("%s\n", string(test.key))
        fmt.Printf("%s\n", string(test.plain))
        fmt.Printf("%X\n", cipher)
        fmt.Println(string(rc4(cipher, test.key)) == string(test.plain))
    }
}

Output:

Key
Plaintext
BBF316E8D940AF0AD3
true
Wiki
pedia
1021BF0420
true
Secret
Attack at dawn
45A01F645FC35B383552544B9BF5
true

References:

The Go Programming Language Specification

Integer overflow

Conversions

Mod 256 error:

  Constants

  There are boolean constants, rune constants, integer constants,
  floating-point constants, complex constants, and string constants.
  Character, integer, floating-point, and complex constants are
  collectively called numeric constants.

  A constant value is represented by a rune, integer, floating-point,
  imaginary, or string literal, an identifier denoting a constant, a
  constant expression, a conversion with a result that is a constant, or
  the result value of some built-in functions such as unsafe. The
  boolean truth values are represented by the predeclared constants true
  and false. The predeclared identifier iota denotes an integer
  constant.

  Numeric constants represent values of arbitrary precision and do not
  overflow.

  Constants may be typed or untyped. Literal constants, true, false,
  iota, and certain constant expressions containing only untyped
  constant operands are untyped.

  A constant may be given a type explicitly by a constant declaration or
  conversion, or implicitly when used in a variable declaration or an
  assignment or as an operand in an expression. It is an error if the
  constant value cannot be represented as a value of the respective
  type.

  Operators

  Operators combine operands into expressions.

Expression = UnaryExpr | Expression binary_op UnaryExpr .
UnaryExpr  = PrimaryExpr | unary_op UnaryExpr .

binary_op  = "||" | "&&" | rel_op | add_op | mul_op .
rel_op     = "==" | "!=" | "<" | "<=" | ">" | ">=" .
add_op     = "+" | "-" | "|" | "^" .
mul_op     = "*" | "/" | "%" | "<<" | ">>" | "&" | "&^" .

unary_op   = "+" | "-" | "!" | "^" | "*" | "&" | "<-" .

  Comparisons are discussed elsewhere. For other binary operators, the
  operand types must be identical unless the operation involves shifts
  or untyped constants. For operations involving constants only, see the
  section on constant expressions.

  Except for shift operations, if one operand is an untyped constant and
  the other operand is not, the constant is converted to the type of the
  other operand.

For example, the compile error "constant 256 overflows byte" can occur as follows:

b := byte(42)
// constant 256 overflows byte
m := b % 256

The integer literal 256 is an untyped numeric constant. In the expression b % 256, it takes the type (byte) of the other operand b. A byte is an eight-bit unsigned integer that can represent the integers 0 to 255. Therefore, it's an error that the constant value 256 cannot be represented as a value of type byte: "constant 256 overflows byte."


Angular 2 window service that listens for 'click away' This is the angular service I wrote window.service.ts.

import { Injectable } from '@angular/core';

import { Subject } from 'rxjs/subject';
import { Observable } from 'rxjs/observable';

declare let $: any;

@Injectable()
export class WindowService {

  private _subscriptions: Array<Subscription> = [];

  constructor() {
    $(window).bind('click', this._onWindowClick.bind(this));
  }

  ngOnDestroy() {
    $(window).unbind('click');
  }

  // Handle any click event on the document
  private _onWindowClick(event): void {
    let $target = $(event.target);
    // Check every subscription you to see if the $target element
    // that was clicked is contained inside the selector they subscribed with
    this._subscriptions.forEach((sub: Subscription) => {
      // IF you do find that selector continue - the click was inside this subscribers
      // element ELSE fire off the subject to let the subscriber know a click away from them occurred
      if ($target.closest(sub.selector).length == 0)
        sub.subject.next(event);
    });
  }

  // Sign up for clickAway event - called by components
  public clickAway(selector: string): Observable<Event> {
    let subject = new Subject();
    this._subscriptions.push(new Subscription(selector, subject));
    return subject.asObservable();
  }

}

class Subscription {
  constructor(
    public selector: string,
    public subject: Subject<Event>
  ) { }
}

The problem I was trying to solve with it was that I have custom components that open and close. Because of the strict component scope that Angular enforces I had no way to know if someone had clicked away from an open component.

Think of a native HTML select element. You click it to open it, then you click away from it and expect it to close. I needed that functionality.

So I created this service that takes a selector that should represent the most parent element of the component. When a click on the window occurs the service checks to see if the click occurred inside the element passed during subscription. If it was not on that element, it executes next() so the component can know that a click occurred away from itself and likely close itself.

Example use in a component:

ngOnInit() {
  this._windowService.clickAway('#customSelect')
    .subscribe(() => {
      console.log('clicked away');
      this.state = ViewState.Closed;
    });
}

I'm looking for a review on the code in specific to good Angular and javascript practices. I know using $ is considered a bad practice but I already have it in the project and it makes the check to see if the event occurred an easily understandable inside another element a one liner. Also, best practices on how Angular Services, Component, and Directives should interact and communicate is confusing to me so if there is changes that could make this better, looking for those.

Also of course if anyone sees any red flags, please point out.

To answer your first question: yes it works.

Have you explore solutions with the HostListener decorator?

https://stackoverflow.com/questions/40107008/hostlistener-documentclick-angular-2-same-component

Subject and Observable imports are capitalized.

import { Subject } from 'rxjs/Subject';
import { Observable } from 'rxjs/Observable';

you can use const instead of let for $target and subject variables since they are not reassigned.

Use triple-equals.

if ($target.closest(sub.selector).length === 0) {
  sub.subject.next(event);
}

You can spot the previous points with the ng lint command.

Lifecycle hooks only work in components.

You can drop the JS convention of prefixing private members with _.

Members of the Subscription class can be marked as readonly. Moreover, Angular is a rxjs-everywhere-framework so Subscription is a confusing name since it's a common type of rxjs.

JQuery is a dependency of your service, maybe it can be treated like every other dependencies, with injection. You can also type the jQuery objects if you install the @types/jquery package.

import { Inject, Injectable, InjectionToken, ValueProvider } from '@angular/core';
import { Subject } from 'rxjs/Subject';
import { Observable } from 'rxjs/Observable';

import * as $ from 'jquery';

export const DOM_HELPER: InjectionToken<JQueryStatic> = new InjectionToken<JQueryStatic>('DomHelper');
// use the provider in your app.module
export const domHelperProvider: ValueProvider = {provide: DOM_HELPER, useValue: $};

class Subscription {
  constructor(public readonly selector: JQuery.Selector,
              public readonly subject: Subject<JQuery.Event>) {
  }
}

@Injectable()
export class WindowService {
  private subscriptions: Array<Subscription> = [];

  constructor(@Inject(DOM_HELPER) private $: JQueryStatic) {
    this.$(window).click((event: JQuery.Event): void => this.onWindowClick(event));
  }

  public clickAway(selector: JQuery.Selector): Observable<JQuery.Event> {
    const subject = new Subject();
    this.subscriptions.push(new Subscription(selector, subject));
    return subject.asObservable();
  }

  private onWindowClick(event: JQuery.Event): void {
    const $target = this.$(event.target);
    this.subscriptions.forEach((sub: Subscription): void => {
      if ($target.closest(sub.selector).length === 0) {
        sub.subject.next(event);
      }
    });
  }
}

Your code works but it's buggy. The subscriptions array will never stop growing and may cause performance issue. You can add an "unsubscribing" method in your service, components will call it in their ngOnDestroy method; but I think it's a bad design, consumers of your service will forget to call the method for sure because they will assume that they only have to unsubscribe from the observable you provide.

You have to get rid of this array. I suggest creating an Observable directly from the click events and then filtering it in clickAway:

import { Inject, Injectable, InjectionToken, ValueProvider } from '@angular/core';
import { Observable } from 'rxjs/Observable';
import 'rxjs/add/observable/fromEvent';
import 'rxjs/add/operator/filter';

import * as $ from 'jquery';

export const DOM_HELPER: InjectionToken<JQueryStatic> = new InjectionToken<JQueryStatic>('DomHelper');
export const domHelperProvider: ValueProvider = {provide: DOM_HELPER, useValue: $};

@Injectable()
export class WindowService {
  private observable: Observable<Event>;

  constructor(@Inject(DOM_HELPER) private $: JQueryStatic) {
    this.observable = Observable.fromEvent(window, 'click');
  }

  public clickAway(selector: JQuery.Selector): Observable<Event> {
    return this.observable
      .filter((event: Event): boolean => this.$(event.target).closest(selector).length === 0);
  }
}

Finally you don't need jQuery to achieve the functionality and you can use Angular and native types in your API:

import { ElementRef, Injectable } from '@angular/core';
import { Observable } from 'rxjs/Observable';
import 'rxjs/add/observable/fromEvent';
import 'rxjs/add/operator/filter';

@Injectable()
export class WindowService {
  private observable: Observable<Event>;

  constructor() {
    this.observable = Observable.fromEvent(window, 'click');
  }

  public clickAway(el: ElementRef): Observable<Event> {
    return this.observable.filter((event: Event): boolean => !el.nativeElement.contains(event.target));
  }
}

Example of a component using the service:

import { Component, ElementRef, OnDestroy, OnInit, ViewChild } from '@angular/core';
import { WindowService } from '../window.service';
import { Subscription } from 'rxjs/Subscription';

@Component({
  selector: 'app-closable',
  templateUrl: './closable.component.html',
  styleUrls: ['./closable.component.css']
})
export class ClosableComponent implements OnInit, OnDestroy {

  @ViewChild('outside')
  private outside: ElementRef;

  private subscription: Subscription;

  constructor(private window: WindowService) {
  }

  ngOnInit() {
    this.subscription = this.window.clickAway(this.outside).subscribe(() => console.log('clicked away!'));
  }

  ngOnDestroy() {
    this.subscription.unsubscribe();
  }
}

and its template:

<div>
  closable works!
  <div #outside>
    <button>click outside of me</button>
  </div>
</div>


Remove first occurrence of element from list As explained here, I'm learning a limited subset of Scheme.

My task has been to sort a numerical list. To implement a simple insertion sort, I need to remove a single element from a list.

I've done the following (in the spirit of The Little Schemer):

(define remove-elem
  (lambda (ll elem)
    (cond
      ((null? ll) '())
      ((eq? (car ll) elem) (cdr ll))
       (else
        (cons (car ll) (remove-elem (cdr ll) elem))))
    ))

What are other ways of implementing this function?

That looks mostly right to me. I'll suggest two things.

First, there is nice syntatic sugar for defining functions:

(define (remove-elem xs elem)
    (cond ...)
)

Save yourself the lambda.

Second, you are using eq?. That is the wrong checker for this problem. That function returns true if the two objects are the exact same object in memory. It could be true for primitives like ints, but it's not necessarily true. What you want to use is eqv? which will guaranteed work for numbers.


Designing a public API for a JavaScript module I am writing a module that can access localStorage through a public API which exposes methods that allow string or array returns.

It doesn't feel as though I'm doing this in the most effective way. For example, I feel as though using:

var tudCache = getTud(); to grab the object from getTud() doesn't feel like the most correct implementation.

I'm inclined to use an if statement i.e. if(getTud().isSuccessful) to check the conditional but then I would have to re-execute getTud().result to return the result inside that conditional, causing the getTud() function to be called twice.

Does anyone have any thoughts?

_self.getTudCache = function () {

    // returns an object. {'isSuccessful': boolean, 'result': String}
    function getTud() {

        return localstorage.getItem(tudStorageKey);

    }

    function getString() {

        var tudCache = getTud();

        if (tudCache.isSuccessful)
            return tudCache.result;
        return "";

    }

    function getArr() {

        var tudCache = getTud();

        if (tudCache.isSuccessful)
            return JSON.parse(tudCache.result)[tudCacheKey];
        return [];
    }

     return {
         getString: getString,
         getArr: getArr
     }
};

It seems to me that it'd be simpler to always use JSON, rather than limit yourself to strings and arrays, specifically.

E.g.

function retrieve(key) {
  var json = localStorage.getItem(key);
  try {
    return JSON.parse(json).value;
  } catch(e) {
    return json; // either null or a non-comforming value
  }
}

function store(key, value) {
  var json = JSON.stringify({ value: value });
  localStorage.setItem(key, json);
}

This will let you store and retrieve any JSON-serializable value including strings and arrays. You can wrap/namespace it however you like.


Table-Driven Finite State Machine I'm writing a finite state machine based on a transition table in C++ 14.
My code so far looks like:

#include <array>
#include <functional>

template < typename _State, typename _Event >
class FSM {
  static constexpr size_t state_count = static_cast<size_t>(_Event::_);
  static constexpr size_t event_count = static_cast<size_t>(_State::_);

  using _FuncPtr = typename std::function<_State(_State)>;
  using _Table   = typename std::array< std::array< _FuncPtr, state_count >, event_count >;

  _State _current;
  _Table _transitions;

public:
  FSM(_State initial, _Table transitions) : _current(initial), _transitions(transitions) {};

  void operator()(const _Event& event) {
    _current = _transitions[static_cast<size_t>(event)][static_cast<size_t>(_current)](_current);
  }

  _State state() const { return _current; }
};

The usage looks like:

/*            States & Events            */
enum class State : char { Locked, Unlocked,    _ };
enum class Event : char { Pass, Coin,          _ };

/*              Transitions              */
State alarm(State current) {
  cout << "Alarm" << endl;
  return current;
}

State unlock(State current) {
  cout << "Unlock" << endl;
  return State::Unlocked;
}

State lock(State current) {
  cout << "Lock" << endl;
  return State::Locked;
}

State thankyou(State current) {
  cout << "Thankyou" << endl;
  return current;
}

void main() {
  auto fsm = FSM< State, Event >(State::Locked, {{
    // Locked    Unlocked
    {  alarm,    lock      },  // Pass
    {  unlock,   thankyou  }   // Coin
  }});

  // To switch states
  fsm(Event::Coin);
  fsm(Event::Pass);
}

Is there some way I can further simplify it?

I see a number of things that may help you improve your code.

Don't use leading underscores in names

Anything with a leading underscore is a reserved name in C++ (and in C).  See this question for details.

Don't use void main()

The only function signatures allowed by the C++ standard both require an int return type. See this question for details.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.  This sample doesn't explicitly have that, but it's implied by the way the usage code is written.

Fix the bug

Right now, the way the arrays are constructed and the way that they're used are reversed.  Try implementing a state machine in which the number of states does not equal the number of events and you'll quickly see what I mean.

Rethink the interface

Right now, there is very little benefit in using the FSM class.  All of the actual code to implement the finite state machine is done with freestanding functions rather than member functions and requires a pair of enum classes that each must have _ as the last named item.  Further, the event that caused the transition isn't passed to the function -- it's implicit.  All of this makes for a fragile design.  Take the classic example of a vending machine that dispenses items for fifteen cents and that accepts only nickels (5 cents) and dimes (10 cents).  We'd have something like this:

enum class State : char { Empty, Five, Ten, _ };
enum class Event : char { Nickel, Dime,   _ };

// Then the state machine
auto fsm = FSM< State, Event >(State::Empty, {{
    // Nickel       Dime
    { zeroToFive,  zeroToTen },    // Empty
    { fiveToTen,   fiveToFifteen},  // Five
    { tenToFifteen, tenToTwenty}   // Ten
}});

Now we still have to write six standalone functions to implement this!  That's not very convenient.  Worse, the argument each function gets isn't even used, so each function looks something like this:

State fiveToFifteen(State ) {
  std::cout << "got a dime, now have 15 cents\nHere's your item!\n";
  return State::Empty;
}


Iterating through specific elements of an array with filter() I have the following code which iterates through some divs of an array to show() them. I was wondering if there is any way to rewrite the for loop using filter(). Will this have a better performance over my initial code?

var selectEnabledGenerators = function(generators) {
    //List of generator indexes to show
    var list = generators;
    var allGenerators = $(".generatorContainer");
    //Hide all generators
    allGenerators.hide();
    //maybe use filter here?
    for (var i = 0, max = list.length; i < max; i++) {
        $(".generatorContainer[data-generator=" + list[i] + "]").show();
    }
};

This is kinda pointless:

var list = generators;

The generators argument is already a local variable. I get you might want to call it by a different name, but in that case the parameter should be called generatorIndices or something more descriptive.
The allGenerators variable isn't necessary. It could be, but you're not reusing it. You could just write $(".generatorContainer").hide();.

And yes, you can use jQuery's filter. You've already found all .generatorContainer elements once, so that's the collection you want to filter.

I'd probably just do:

var selectEnabledGenerators = function (generatorIndices) {
    var containers = $(".generatorContainer").hide();
    generatorIndices.forEach(i => containers.filter(`[data-generator=${i}]`).show() );
};


Console Application Customizer - GetBootstrap v2.7.0 GetBootstrap v2.7.0 Update from GetBootstrap v2.5

I add a progress bar feature in my console application customizer and update the #region grouping and convert the switch statement to Dictionary that suggested in the last version.

For now I need help in reviewing my codes in ProgressBar.cs. You can also review my source code and contribute in my GitHub if you want. DOWNLOAD GetBootstrap v2.7.0

My objective is to create a progress bar that can be modify to 4 different size (small, normal, medium, and large) and change color.

This is the best code I can make. Any Suggestion?

ProgressBar.cs

public class ProgressBar
{
    public int Max { get; set; }
    public int Value { get; set; }
    public ConsoleColor Color { get; set; }
    public ProgressBarSize Size { get; set; }
    float _Calculation { get; set; }

    public ProgressBar()
    {
        Max = 100;
        Value = 0;
        Color = ConsoleColor.Blue;
        Size = ProgressBarSize.Normal;
    }

    public void Increment(int value)
    {
        Value += value;

        if (Value <= Max)
        {
            SetUpProgressBar();

            WriteProgressBar();
        }

    }

    private void WriteProgressBar()
    {
        ProgressBarSize size = Size;
        object valueSize = Convert.ChangeType(size, size.GetTypeCode());

        int position = 1;
        for (int i = 0; i <= _Calculation * Value; i++)
        {
            Console.BackgroundColor = Color;
            Console.CursorLeft = position++;
            Console.Write(" ");
        }

        for (int i = Value; i <= (int)valueSize + 1; i++)
        {
            Console.BackgroundColor = ConsoleColor.Gray;
            Console.CursorLeft = position++;
            Console.Write(" ");
        }

        WriteProgressBarStatus((int)valueSize);
    }

    private void WriteProgressBarStatus(int p)
    {
        Console.CursorLeft = p + 5;
        Console.ResetColor();
        Console.Write(Value + " of " + Max);
    }

    private void SetUpProgressBar()
    {
        ProgressBarSize size = Size;
        object valueSize = Convert.ChangeType(size , size.GetTypeCode());

        Console.CursorLeft = 0;
        Console.Write("|");
        Console.CursorLeft = (int)valueSize + 2;
        Console.Write("|");
        Console.CursorLeft = 1;
        _Calculation = ((int)valueSize + 0.0f) / Max;
    }
}

public enum ProgressBarSize
{
    Small = 20,
    Normal = 30,
    Medium = 40,
    Large = 60
}

It is normally a good idea to separate data from behavior as this way we follow SOLID guidelines.

I recommend creating a ProgressBar object and maybe a ProgressBarOperationService or the like with the behavior.

The ProgressBarOperationService would receive the ProgressBar that needs the operation in all its methods.


Filtering DbContext data dynamically by user input in WPF applications I need to filter database data based on filters available to the end user in the form of search term text box, select boxes etc.

I have put together this code and need feedback if this is a good way to do it or if there are any better solutions.

using Multi.Model;
using System;
using System.Linq;
using System.Windows.Controls;

namespace Multi.Pages
{
    /// <summary>
    /// Interaction logic for Page1.xaml
    /// </summary>
    public partial class Page1 : Page
    {

        public Page1()
        {
            InitializeComponent();
            textBoxName.TextChanged += TextBoxName_TextChanged;
            PopulateDataGrid();
        }

        private void PopulateDataGrid()
        {
            using (var db = new optisysEntities())
            {
                var items = db.clients.AsQueryable();
                items = FilterClients(db, items);
                dataGrid.ItemsSource = items.ToList();
            }
        }

        private System.Linq.IQueryable<Multi.Model.clients> FilterClients(optisysEntities db, System.Linq.IQueryable<Multi.Model.clients> clients)
        {

            if (!String.IsNullOrWhiteSpace(textBoxName.Text)) clients = db.clients.Where(c => c.name.Contains(textBoxName.Text)
                                                                                || c.phone.Contains(textBoxName.Text));
            // if (!String.IsNullOrWhiteSpace(search.Email)) clients = clients.Where(u => u.Email.Contains(search.Email));
            // if (search.UsertypeId.HasValue) clients = clients.Where(u => u.UsertypeId == search.UsertypeId.Value);
            return clients;
        }

        private void TextBoxName_TextChanged(object sender, TextChangedEventArgs e)
        {
            PopulateDataGrid();
        }

    }
}

Reducing your code to what I think is the core of your question:

private System.Linq.IQueryable<Multi.Model.clients> FilterClients(optisysEntities db, System.Linq.IQueryable<Multi.Model.clients> clients)
{

    if (!String.IsNullOrWhiteSpace(textBoxName.Text))
        clients = db.clients.Where(c => c.name.Contains(textBoxName.Text) || c.phone.Contains(textBoxName.Text));

    if (!String.IsNullOrWhiteSpace(search.Email))
        clients = clients.Where(u => u.Email.Contains(search.Email));

    if (search.UsertypeId.HasValue)
        clients = clients.Where(u => u.UsertypeId == search.UsertypeId.Value);

    return clients;
}

Yes, this is one of the better ways to do dynamic filtering.

I would suggest abstracting the data retrieval into a separate layer. You don't want your form logic handling your underlying ORM directly.

This is exacerbated by the fact that you've put the FilterClients() method by itself. Your UI form therefore contains a method whose reponsibility has nothing to do with the UI.

However, I get the feeling that this application is either tiny, or has only just been developed. So I can understand that this abstraction is something for a later stage. I would suggest doing it immediately to make it less painful in the future, but that's your choice.


Activator with simple dependency injection In one of my projects I needed a simple dependency incjection activator so I've built one. It performs only type/inheritance matching because I'm not creating a second Autofac and I think I won't need anything more complex any time soon (aka YAGNI).

To find the dependencies I use a Join with a custom comparer. It runs over constructor parameters and tries to match a dependency and maintain the order of the parameters. If the parameters and dependencies counts don't match I use Except to find the missing ones and put their names in the exception message.

I don't use a DI container because I use it for creating commands based on the command line.

public interface IDependencyInjectionActivator
{
    object CreateInstance(Type type, params object[] candidates);
}

public class DependencyInjectionActivator : IDependencyInjectionActivator
{
    private static readonly IEqualityComparer<Type> TypeInheritanceComparer =
        AdHocEqualityComparer<Type>
            .CreateWithoutHashCode((candidate, parameter) => parameter.IsAssignableFrom(candidate));

    public object CreateInstance(Type type, params object[] candidates)
    {
        var constructor = type.GetConstructors().Single();

        var parameters =
            constructor
                .GetParameters()
                .Join(
                    candidates,
                    parameter => parameter.ParameterType,
                    candidate => candidate.GetType(),
                    (parameter, candidate) => candidate,
                    TypeInheritanceComparer
                ).ToArray();

        var dependenciesResolved = (parameters.Length == constructor.GetParameters().Length);
        if (dependenciesResolved)
        {
            return Activator.CreateInstance(type, parameters);
        }

        var missingDependencies =
            constructor
                .GetParameters()
                .Select(p => p.ParameterType)
                .Except(parameters.Select(p => p.GetType()), TypeInheritanceComparer)
                .ToList();

        var message =
            $"Some dependecies for {type.ToPrettyString().QuoteWith("'")} could not be resolved: " +
            $"{missingDependencies.Select(t => t.ToPrettyString()).Join(", ").EncloseWith("[]")}";
        throw DynamicException.Factory.CreateDynamicException($"MissingDependency{nameof(Exception)}", message, null);
    }
}

The compare I use here takes two Funcs for each interface method. Factory methods make the creation easier.

public class AdHocEqualityComparer<T> : IEqualityComparer<T>
{
    private readonly Func<T, T, bool> _equals;
    private readonly Func<T, int> _getHashCode;

    private AdHocEqualityComparer(Func<T, T, bool> equals, Func<T, int> getHashCode)
    {
        _equals = equals;
        _getHashCode = getHashCode;
    }

    public static IEqualityComparer<T> CreateWithoutHashCode([NotNull] Func<T, T, bool> equals)
    {
        if (equals == null) throw new ArgumentNullException(nameof(@equals));

        return Create(equals, _ => 0);
    }

    public static IEqualityComparer<T> Create([NotNull] Func<T, T, bool> equals, [NotNull] Func<T, int> getHashCode)
    {
        if (equals == null) throw new ArgumentNullException(nameof(equals));
        if (getHashCode == null) throw new ArgumentNullException(nameof(getHashCode));

        return new AdHocEqualityComparer<T>(equals, getHashCode);
    }

    public bool Equals(T x, T y)
    {
        if (ReferenceEquals(null, x)) return false;
        if (ReferenceEquals(null, y)) return false;
        if (ReferenceEquals(x, y)) return true;
        return _equals(x, y);
    }

    public int GetHashCode(T obj) => _getHashCode(obj);
}

This time without examples because I think the usage is strightforward. You create and instance of the activator, specify the type you want to create and pass an array of dependency caditates.

Do you think this DI is smart enough or does it need any improvements?

You are putting pretty strong limitations on classes you can activate this way - a single .ctor, no default .ctor(which can make serialization troubles later) and all .ctor arguments should have different types, otherwise .Join will go crazy. No nested dependencies support, too. No named registrations.

So, all in all, your DI is rather stupid, and if you try to make it smarter, you'll unavoidably end up with "another Autofac".

I went this way once and two months later I wished I hadn't... unless you need to save every byte of RAM, home-made DI surrogate simply doesn't pay off.


Parsing airline reservation using JS regular expressions I'm searching a set text for multiple values, which will later be stored in a database.

I am using RegExp and worked with the official documentation and help to get the code to be two separate while loops. The question is if it's possible combine those into one, adding more and more regular expressions, or if they each need a for-loop of their own.

const regex = /PREPARED FOR([^]*?)RESERVATION/g;
const regex2 = /AIRLINE RESERVATION CODE (.*)/g;

const str = `30 OCT 2017  04 NOV 2017
Distance (in Miles):500
Stop(s): 0
Distance (in Miles):500
Stop(s):0
TRIP TO KRAKOW, POLAND
PREPARED FOR
DOE/JANE MRS
APPLESEED/JOHN MR
RESERVATION CODE   UVWXYZ
AIRLINE RESERVATION CODE DING67 (OS)
AIRLINE RESERVATION CODE HDY75S (OS)`;
let m;
let x;

while ((m = regex.exec(str)) !== null) {
    // This is necessary to avoid infinite loops with zero-width matches
    if (m.index === regex.lastIndex) {
        regex.lastIndex++;
    }
    console.log(m[1])

}

while ((x = regex2.exec(str)) !== null) {
    // This is necessary to avoid infinite loops with zero-width matches
    if (x.index === regex2.lastIndex) {
        regex2.lastIndex++;
    }
    console.log(x[1])

}
console.log("We're done here")

Now, this works perfectly fine as is, but I will be adding more filters and searches to it, which means I may get to a total of 8-9 while loops, which may slow the process down, or be inefficient.

You don't need a loop, you can use the string's match method, but in order to get only the matched group you have to use a lookbehind, which javascript's implementation of regex doesn't support, but, it does support a lookahead, so basically, you could reverse it, reverse all the matches and filter out empty strings to get the reservation codes. Since you only have a single match for the names you don't need a loop for that either, assuming there will always be a match, so you can just use exec and pop the result off the end..

const str = `30 OCT 2017  04 NOV 2017
    Distance (in Miles):500
    Stop(s): 0
    Distance (in Miles):500
    Stop(s):0
    TRIP TO KRAKOW, POLAND
    PREPARED FOR
    DOE/JANE MRS
    APPLESEED/JOHN MR
    RESERVATION CODE   UVWXYZ
    AIRLINE RESERVATION CODE DING67 (OS)
    AIRLINE RESERVATION CODE HDY75S (OS)`;

var r1 = / (.*)(?= EDOC NOITAVRESER ENILRIA)/g,
    r2 = /PREPARED FOR([^]*?)RESERVATION/g;

const rev = s=>s.split('').reverse().join('').trim();

let x = r2.exec(str).pop();
let m = rev(str).match(r1).map(rev).filter(w=>w.length);

console.log(x);
console.log(m);


CSS attribute Selector: responsive grid performance I am looking to create a simple column based responsive framework, and would like to call my classes something like:

where the div would display as 4 columns on a desktop browser, 2 columns on a tablet and one column on a phone (these breakpoints are hypothetical).

or

where the div would display as 8 columns on a desktop browser, 4 columns on a tablet and 2 columns on a phone.

The easiest way would be to use attribute selectors, but I am not sure if this would be a huge performance hit. Is this a good idea, or should I write out all the permutations of the column structure (720 classes)? Or just do what Bootstrap does, with col-sm-12 col-md-6 col-lg-3

// columns
.col[class|="c-6"] { width: 16.666%; }
.col[class|="c-5"] { width: 20%; }
.col[class|="c-4"] { width: 25%; }
.col[class|="c-3"] { width: 33.333%; }
.col[class|="c-2"] { width: 50%; }
.col[class|="c-1"] { width: 100%; }

@media only screen and (max-width:800px) {
  .col[class*="-6-"]  { width: 16.666%; }
  .col[class*="-5-"]  { width: 20%; }
  .col[class*="-4-"]  { width: 25%; }
  .col[class*="-3-"]  { width: 33.333%; }
  .col[class*="-2-"]  { width: 50%; }
  .col[class*="-1-"]  { width: 100%; }
}

@media only screen and (max-width:600px) {
  .col[class$="-6"] { width: 16.666%; }
  .col[class$="-5"] { width: 20%; }
  .col[class$="-4"] { width: 25%; }
  .col[class$="-3"] { width: 33.333%; }
  .col[class$="-2"] { width: 50%; }
  .col[class$="-1"] { width: 100%; }
}

You should do neither.  Not for performance reasons, but for semantic and maintenance reasons.  Having class names that reflect what the element looks like is an extremely popular anti-pattern.  It's convenient in the here and now, but every single instance of these classes in your markup will need to be changed when your design changes.

Related reading:  http://www.kapowaz.net/articles/cargo-cult-css

Furthermore, the concept of phone/tablet/desktop styles is extremely narrow thinking and is rarely appropriate for any design.

Related reading: http://bradfrost.com/blog/post/7-habits-of-highly-effective-media-queries/


4x4 Matrix Keypad on Arduino UNO I recently bought a 4x4 keypad.
It is one like this:
http://www.learningaboutelectronics.com/images/4x4-matrix-keypad-pinout.png

Now every code I found on the net was either using a highly overloaded library or was poorly hacked by a guy in the arduino forums.
I realized that almost all blogs about electronics just seem to copypaste the same code and changing the pictures so I felt like I had to work out something by myself.

Now, the common explained "tactic" was to iterate through every column and give it a LOW signal [for button pressed DOWN] and then check if anything came in at the row sides.Then use a technique from video game programming to make sure the button would not stay in a pin-down state [so one press would count at a time and not a press occurred every whatsomany nanoseconds].

I was highly uncomfortable with this solution, so I came up with this code:

/* Reverse Keypad Handler
 * 2nd version
 * by clockw0rk @ 26.02.2017
 */

//define rows and columns
byte row [4] = {9,8,7,6};
byte col [4] = {5,4,3,2};

byte counter = 0;
byte colCount;

//16 fields keymapping 4x4 matrix
byte keys [ 0x4 ] [ 0x4 ] =
{
   {0x1,0x2,0x3,0xA},
   {0x4,0x5,0x6,0xB},
   {0x7,0x8,0x9,0xC},
   {0xE,0x0,0xF,0xD}
};

void setup() {
  for(int c = 0; c != 4; c++)
  {
    pinMode (row[c], INPUT_PULLUP);
    pinMode (col[c], OUTPUT);
    digitalWrite(col[c], LOW);
  }

   Serial.begin (9600);  //9600 BAUD for Uno Board Serial Communication
   Serial.println("INITIALISATION FINISHED\nEntering work loop...\n");

}

void loop() {

  // cycle through input pins and check for a signal
  if(digitalRead(row[counter]) == 0)
  {
    checkRoutine();
  }

  //reset counter for each iteration cycle
  counter <3 ? counter++ : counter = 0;

}

void checkRoutine(){

  colCount = 0;

  //debugging: give me the linecount
  Serial.print("[ LOW ] on line ");
  Serial.print(counter);
  Serial.print("\n");

  //now find which number is pressed
  while(colCount < 4 && digitalRead(row[counter]) == 0)
  {
     digitalWrite(col[colCount], HIGH);
     colCount++;
  }

  //remember we incremented in while loop
  Serial.print("Button pressed is [ ");
  Serial.print(keys[counter][colCount -1]);
  Serial.print(" ]\n");

  //reinitialise output
  for(int c = 0; c != 4; c++)
  {
        digitalWrite(col[c], LOW);
  }

  //wait until user releases button
  while(digitalRead(row[counter])==0)
  {;}
}

As you can see, I reversed the idea (kind of) by giving every output a permanent LOW and made sure the program flow is stopped until the user releases the button.

I would like to hear your thougths on this solution.
Is it wasting memory or CPU time?
Arduino programming is absolutely fun but I just recently started with it and want to get better so I ask all of you "old veterans" to look over the lines and give me a feedback.

Your general idea is correct - looping over columns to find out if one of them is active, and then only looping over the rows to find out which button was pressed will be somewhat faster (as in: more responsive) than checking each possible button separately.

If I were to suggest any improvements in the method - you could look into using pin change interrupts instead of busy looping. This would provide you with hardware support to detect when a pin goes LOW.
This would also allow you to put your arduino to sleep to save power while waiting for a button to be pressed.

You might also need to debounce the keypress since you can get momentary false readings from a switch. This would basically require a small delay (usually less than 100ms) after you detect a keypress, and then rechecking after the delay if that switch is still active.

the comments below are going beyond your question but I decided to add them for some perspective on coding style

As far as your code goes I would suggest putting the entire key-checking logic in a single function that uses local variables for your column and row counters. This primarily helps with reasoning about the code but also has a small positive effect on memory usage overall (memory on the stack will be reclaimed after a return from the function).

Instead of using magic numbers throughout your code you should use constants/macros. This has the benefit of giving the value a name, and having a single place that would require changing instead of multiple values that are not easily ctrl+f'able. Calculating array length can be automated. This eases adding changes further on, and helps enforce correctness by the compiler.

You should also declare your pin arrays as const if possible. This will make optimizing your code easier for the compiler.

A sample of how this would look together:

//define rows and columns
const byte row[] = {9,8,7,6};
const byte rowCount = sizeof(row) / sizeof(row[0]); //old C trick for calculating static array length
const byte col[] = {5,4,3,2};
const byte colCount = sizeof(col) / sizeof(col[0]);

const byte keys [ rowCount ] [ colCount ] =
{
    {0x1,0x2,0x3,0xA},
    {0x4,0x5,0x6,0xB},
    {0x7,0x8,0x9,0xC},
    {0xE,0x0,0xF,0xD}
};


Reverse complement of a DNA string I have a function in python where I want the reverse complement of a DNA string.

This is what I have now:

def inversComplement(input):
    output = ''
    for letter in input:
        letter = letter.upper()

        if letter == 'A':
            output += 'T'
        elif letter == 'T':
            output += 'A'
        elif letter == 'G':
            output += 'C'
        else:
            output += 'G'

    return(output[::-1])

I was thinking of using the string.replace() method. But the problem was that if I use this the T-> A would be changed afterwards with the A -> T replacement

    string.replace('A','T').replace('T','A').replace('G','C').replace('C','G')

You can do this with str.translate, which was changed slightly in Python  3. All you need to do is pass it a translation table, and it'll do what your above code is doing. However to build the translation table is hard in Python 2 and requires the helper function string.maketrans, which was changed to str.maketrans in Python 3.

But here is how you'd use it in Python 2:

>>> import string
>>> trans = string.maketrans('ATGC', 'TACG')
>>> 'ATTAGCC'.translate(trans)
'TAATCGG'

And here is how you can use it in Python 3:

>>> trans = str.maketrans('ATGC', 'TACG')
>>> 'ATTAGCC'.translate(trans)
'TAATCGG'
>>> trans = {ord('A'): 'T', ord('T'): 'A', ord('G'): 'C', ord('C'): 'G'}
>>> 'ATTAGCC'.translate(trans)
'TAATCGG'

You can re-implement this yourself, but the above is written in C, and so should out perform any Python code.


Calculate number of words and characters (/statistics) from an HTML Textarea var textarea = document.getElementById("textarea"),
  statusBar = document.getElementById("status-bar");
    function updateStatusBar() {
      var text = textarea.value,
        chars = text.length,
        words = text.split(/\b\S+\b/g).length - 1,
        lines = text.split("\n").length;
      statusBar.value = lines + " lines, " + words + " words, " + chars + " characters";
    }

I will give it a try:

These days I would recommend using document.querySelector, to write generic code which is well amenable to code re-factoring without introducing bugs.

So either:

var  textarea = document.querySelector("#my-textarea")
    ,statusBar = document.querySelector("#status-bar");

or in later JavaScript versions, I believe >=1.6 (i.e. OK Server-side, when a specific JS-runtime can be guaranteed):

var [eltxtarea, elstatusbar] = document.querySelectorAll("#my-textarea,#status-bar");

Do not assign elements to Id's that are also keywords/attributes in the HTML specification (as such not textarea but e.g. my-textarea )
Bind the invocation of your function to an event or directly to underlying data-changes(requiring additional frameworks such as backbone.js). Do not use the respective exposed handler attribute but rather the attachEventHandler.

elstatusbar.attachEventHandler( "change", changeHandlerStatusbarUpdate);

The Handler:

    var changeHandlerStatusbarUpdate = function(evt){
        if(evt instanceof Event && evt.target instanceof HTMLTextAreaElement){
        //you can be more specific. Event is just the Baseclass.
        //for instance you may test for "KeyboardEvent" only

        //the following logic should be improved: See threads of how to count words in a language/culture-specific fashion
            var text = textarea.value,
            chars = text.length,
            words = text.split(/\b\S+\b/g).length - 1,
            lines = text.split("\n").length,
            stat = lines + " lines, " + words + " words, " + chars + " characters";
            evt.target.value = stat;
        }
    }

Do not put the updateStatusBar function in the global or window scope when the function relies upon textarea in the outside-scope, but use scope-encapsulation via closures (example), or a retrieve the target element within the function or through the event-target in a handler-function.

As pointed out in the source-comment, the word-counting is much to trivial and not "culture-invariant" as is the case right now.


LINQ Group query - simplifying Is there a way of writing this correctly working query more efficiently? I'm asking only to learn LINQ better:

    var cpuInfo = edgeLPs
        .GroupBy(k => k.CPU.ID, e => e.CPU)
        .ToDictionary(k => k.Key, v => new
        {
            LogicalProcs = v.First().LogicalProcessorsCount,
            Cores = v.First().Cores.Count,
            SMT = v.First().SMT
        });

First, the usual disclaimer about performance: most of the time, you shouldn't worry much about it. If you find that your code is too slow, you should measure to find out which part is causing the slowdown and improve that. Trying to make code that's not performance critical more efficient is often a waste of your time.

In your specific case, I don't think the calls to First() would hurt your performance. But I would understand if you wanted to fix that to make your code more DRY. To do that, you could use Select():

var cpuInfo = edgeLPs
    .GroupBy(e => e.CPU.ID, e => e.CPU)
    .Select(g => new { id = g.Key, cpu = g.First() })
    .ToDictionary(g => g.id, g => new
    {
        LogicalProcs = g.cpu.LogicalProcessorsCount,
        Cores = g.cpu.Cores.Count,
        SMT = g.cpu.SMT
    });

Though the change from .First() to something like .cpu isn't actually much of an improvement.

Also, I think that using First() like this can be a sign of faulty logic. If you know that there will be always at most one CPU with a given ID, then you should use Single() instead, or, in your case, simply use ToDictionary() without the preceding GroupBy(). If there can be multiple CPUs with the same ID, i think you should figure out what to do with the rest of them, and not simply ignore all but the first one.


Managing documents with manager class So I have a program that visualizes data from files (csv,txt,etc.). These files are saved to database as individual tables. In my application I have DocumentExplorer which is just a module with a list of available documents(files). You can open and close any document you want, and you can have multiple documents opened at the same time.

This program also uses MEF to separate modules and exposes API module for users to create plugins. What I'm working on is some kind of manager to manage all opened documents in current session. Basically I need something that will be responsible for closing, opening, switching documents. I have 2 ideas how to deal with it, but I'm not sure whether they are any good or not.
This is what I came up with so far:

1. DocumentManager passed by IoC container with interface exposed inside API module.

   public class DocumentManager : IDocumentManager
    {
        private List<IDocument> ActiveDocuments { get; set; }

        public void Create(IDocument document)
        {
            if(!ActiveDocuments.Contains(document))
                ActiveDocuments.Add(document);
        }

        public void Close(IDocument document)
        {
            if (ActiveDocuments.Contains(document))
                ActiveDocuments.Remove(document);
        }

        public IDocument Get(Expression<Func<IDocument, bool>> predicate)
        {
            return ActiveDocuments.AsQueryable().FirstOrDefault(predicate);
        }
    }

2. DocumentManager as static class kept inside API module, so others can access it. (without IDocumentManager interface, obviously)

What are your thoughts? Are these good approaches? Do you know any good tricks with this kind of problem?

Manager - Repository or Factory or both?

What is a Manager? It's usually a class that does too much. In this case it tries to be a repository and a factory a the same time. This should be separated.

Why would we do this? To make the usage more obvious and to be able to add functionality to either class without affecting or breaking the others and of course to test each module separately.

DocumentRepository

When implementing the repository consider switching from the List<T> to the HashSet<T>. This would improve not only the performance (which is probably neglectable here - with a dozen of documents no one will notice the change) but it would greaty simplify your code and both methods would be single calls:

public bool Add(IDocument document)
{
    return ActiveDocuments.Add(document);
}

public bool Remove(IDocument document)
{
    remove ActiveDocuments.Remove(document);
}

What about the naming? You call your methods Create and Close but I don't see anything but adding and removing documents to/from a collection. I suggest naming them simply Add and Remove respectively.

If you store the documents in a database then you may add other methods to it. However even better would be to implement two repositories. The first one only knows how to talk to the database and if you need to cache the documents then the decorator repository can do the rest.

Example:

interface IDocumentRepository
{
    IDocument Get(int id);
    bool Add(IDocument document);
}

class DocumentRepository : IDocumentRepository
{
    public IDocument Get(int id)
    {
        // get from database
    }
    bool Add(IDocument document)
    {
        // add to database
    }
}

class CachedDocumentRepository : IDocumentRepository
{
    private readonly DocumentRepository _documentRepository;

    private readonly HashSet<IDocument> _cache = new HashSet<IDocument>();

    public CachedDocumentRepository(DocumentRepository documentRepository)
    {
        _documentRepository = documentRepository;
    }

    public IDocument Get(int id)
    {
        // check the cache and get a document from there or
        // get it from _documentRepository.Get(id);
        // and add to the cache
    }

    bool Add(IDocument document)
    {
        // add to the cache and
        // add to database
    }
}

IEnumerable

With your current implemetation you could remove the Get method and further simplify the repository by implementing the IEnumerable<IDocument> interface for this class.

DocumentFactory

This class should only know how to create documents. It can have methods for creating many different types but nothing else.


String literals as char arrays Is it considered bad practice to pass string literals as char*, when an array and not a c-string is expected by the function? From my code:

char ConsoleIO::GetNextChar(string prompt, char* choices, int numChoices) {
    // Loop until valid input, or 10 times.
    for (int i = 0; i < 10; i++) {
        Write(prompt);

        char* buffer = new char[200];
        cin.getline(buffer, 200);
        char choice = tolower(buffer[0]);

        if (in(choice, choices, numChoices)) {
            return choice;
        }

        WriteLine("Invalid choice.");
    }
    // Just return the first choice as the default.
    return choices[0];
}

// In main:
char userChoice = ConsoleIO::GetNextChar("Make a selection: ", "234", 3);

It compiles, but is it bad practice?

When a C or C-related programmer sees char* they're going to naturally think of passing in a C-string. In my opinion, it's absolutely fine to do that. If you want to invoke an array of characters in C, I'd do it like this:

char ConsoleIO::GetNextChar(string prompt, char[] choices, int numChoices)

But, you're in C++ so you should use a std::vector<char> and then don't pass in numChoices, like this:

char ConsoleIO::GetNextChar(string prompt, std::vector<char> const& choices)

Also, you have a bug. You aren't ever deleteing buffer, which means it's leaking.


Using Comparator with 3 conditions at the same time I am trying to create a class which implements the Comparator class.

The thing is that I have a list with some objects. This object is composed of 3 fields (Strings). My purpose is to sort the list based first on the first field: if the first field is the same on the two objects that I am comparing at the moment, then compare the second item of the 2 objects and finally if both fields are the same, compare the third field of the objects.

This is what I have done but I do not know if this is the correct answer:

public class MyComparator implements Comparator<Object>{

    @Override
    public int compare(final Dto address1, final Dto address2) {
        int result = 0;

        if (dto instanceof Dto) {
            final Dto dto = (Dto) address;
            final Dto dto2 = (Dto) address2;
            if(dto.getName() < dto2.getName()) {
                 result = -1;
            } else if(dto.getName() > dto2.getName()) {
                result = 1;
            } else {
                result = 0;
            }
        }
//if the Names are the same then compare the Number
            if (result == 0) {
                if(dto.Number() < dto2.Number()) {
                    result = -1;
                } else if(dto.Number() > dto2.Number()) {
                    result = 1;
                } else {
                    result = 0;
                }
            }
        //if the Numbers are the same then compare the Other Field

        if (result == 0) {
            if(dto.Other() < dto2.Other()) {
                result = -1;
            } else if(dto.Other() > dto2.Other()) {
                result = 1;
            } else {
                result = 0;
            }
        }

return result

}

Then I will use the following way in my service:

Collections.sort(out,new MyComparator());

The shortest way to do this is using the Java 8 Comparator building api :

Comparator<Dto> myComparator = Comparator.comparing(Dto::getName)
    .thenComparing(Dto::Number)
    .thenComparing(Dto::Other);

And this is even a lot more readable.

Btw : your method names do not follow Java coding conventions, and the posted code doesn't actually compile.


All upper and lowercase permutations of a string A couple of questions:

Is the algorithm wasting CPU time or memory unnecessarily?
If there is something in the code that is not idiomatic Python, how to improve on that?

def permute_first( c ):
    retval = []
    retval.append( c.upper() )
    retval.append( c.lower() )
    return retval

def permute( str ):
    leads = permute_first( str[0] )
    remain = str[1:]
    if not remain:
        return leads
    permutations = permute( remain )
    new_permutations = []
    for perm in permutations :
        for lead in leads:
            new_permutations.append( lead + perm )
    return new_permutations

og = "Question"
print permute( og )

1. Review

"Permutations" is not the right word for what this code does. The permutations of a sequence are the different ways the items can be ordered: for example, the permutations of ABC are ABC, ACB, BAC, BCA, CAB and CBA. These can be computed using the built-in itertools.permutations:

>>> from itertools import permutations
>>> list(map(''.join, permutations('ABC')))
['ABC', 'ACB', 'BAC', 'BCA', 'CAB', 'CBA']

What you have here are the different ways to capitalize the letters in a word, so the name ought to be something like capitalizations.
There are no docstrings. What do these functions do and how to I call them?
The Python style guide (PEP8) recommends:

  Avoid extraneous whitespace … immediately inside parentheses, brackets or braces.

The body of permute_first could be written more simply:

return [c.upper(), c.lower()]

In fact this is so simple that there's no real need for this to be a separate function at all.
If str contains non-letters then we get duplicates in the output:

>>> permute('<>')
['<>', '<>', '<>', '<>']

Use sorted(set((c.upper(), c.lower()))) to de-duplicate the output.
If str is the empty string, permute raises IndexError. It should return a list containing the empty string.

2. Revised code

This is a one-liner using itertools.product:

from itertools import product

def capitalizations(s):
    """Return a list of the different ways of capitalizing the letters in
    the string s.

    >>> capitalizations('abc')
    ['ABC', 'ABc', 'AbC', 'Abc', 'aBC', 'aBc', 'abC', 'abc']
    >>> capitalizations('')
    ['']
    >>> capitalizations('X*Y')
    ['X*Y', 'X*y', 'x*Y', 'x*y']

    """
    return list(map(''.join, product(*(sorted(set((c.upper(), c.lower())))
                                       for c in s))))


Selecting site to browse in python This code is for part of my first major contribution to open-source, for the socli project. The purpose is to allow the user to select a site from the stack exchange network to browse and fill two variables with the appropriate urls (used through the rest of the program). Since the stack exchange network is adding new sites all the time, I wanted to make it decently easy to update the stored list. Also, I'm lazy, and didn't want to manually create a dictionary =) So I use a text file with the list of sites copy-pasted into it and the code takes care of the rest.
Approach
The purpose of the code is to go through a text file named sites.txt filled with urls in a format like
https://codereview.stackexchange.com
https://physics.stackexchange.com

Taking the above, it would then produce a dictionary looking like
{'codereview':'https://codereview.stackexchange.com', 'physics':'https://physics.stackexchange.com'}

It would also print
Sites: codereview, physics

and allow the user to select one of the above, storing the url and a modified version of the url in a variable. I'd like to know if I've done this in the best possible way, and also get general feedback.
Code
import re
siteslst = {}
with open('sites.txt', 'r') as s:
    data = s.read()
    data = data.splitlines()
    for i in data:
        keyVal = re.search(r'(\w+)\.',str(i)).group(1)
        siteslst[str(keyVal)] = str(i)
testInput = siteslst.keys()
print('Sites: ', ', '.join(testInput), '\n')
validInput = False
while validInput == False:
    searchSite = input("which site would you like to search? ")
    if searchSite in testInput:
        url = siteslst[searchSite]
        urlSearch = siteslst[searchSite]+'/search?q='
        validInput = True
    else:
        validInput = False

Here is a list of things I would improve:

variable naming - PEP8 recommends to use lower_case_with_underscores style of variable names
instead of a regular expression, you can use urlparse to parse the domain name
you can use a dictionary comprehension to construct a dictionary of sites
you don't actually need a boolean flag for your while loop - instead initialize an endless loop and break it once the input is valid
instead of calling .keys() and passing the "viewkeys" result to str.join() - you can pass your dictionary directly

Improved code:

from urllib.parse import urlparse

with open('sites.txt') as input_file:
    sites = {
        urlparse(url).netloc.split('.')[0]: url
        for url in input_file
    }

print('Sites: ', ', '.join(sites), '\n')

while True:
    search_site = input("Which site would you like to search?\n")
    if search_site in sites:
        url_search = sites[search_site]+'/search?q='
        print(url_search)
        break

Further thoughts:

make the code more modular and extract parts of the code into separate functions - at this moment, it is clear that you have 2 separate high-level things you do in the code - getting the dictionary of sites and getting the user's input
put the execution code to under the if __name__ == '__main__': to avoid the code to be executed on import
you don't actually need to handle the list of sites manually - StackExchange provides an API endpoint with that information


Optimizing solution for Project Euler Problem #23 (non-abundant sums) Project Euler Problem 23 asks:

  A perfect number is a number for which the sum of its proper divisors
  is exactly equal to the number. For example, the sum of the proper
  divisors of 28 would be 1 + 2 + 4 + 7 + 14 = 28, which means that 28
  is a perfect number.

  A number n is called deficient if the sum of its proper divisors is
  less than n and it is called abundant if this sum exceeds n.

  As 12 is the smallest abundant number, 1 + 2 + 3 + 4 + 6 = 16, the
  smallest number that can be written as the sum of two abundant numbers
  is 24. By mathematical analysis, it can be shown that all integers
  greater than 28123 can be written as the sum of two abundant numbers.
  However, this upper limit cannot be reduced any further by analysis
  even though it is known that the greatest number that cannot be
  expressed as the sum of two abundant numbers is less than this limit.

  Find the sum of all the positive integers which cannot be written as
  the sum of two abundant numbers.

I have been trying to optimize my solution for almost a day now but my program is not ready to get small and optimized. Can anyone please tell me how I can do that?

def isabundant(n): return sum(list(x for x in range(1, int(n/2)+1) if n % x == 0)) > n
abundants = list(x for x in range(1, 28123) if isabundant(x) == True)
sums = 0
for i in range(12, 28123):
    for abundant in abundants:
        if abundant >= i and isabundant(i+abundant) == True: sums += i
print(sums)

Your first problem is that you're trying to cram too much information onto one line. As a result, you loose the overview. Here is a simple refactoring:

def is_abundant(n):
    max_divisor = int(n / 2) + 1
    sum = 0
    for x in range(1, max_divisor):
        if n % x == 0:
            sum += x
    return sum > n

abundants = list(x for x in range(1, 28123) if is_abundant(x))

sums = 0
for i in range(12, 28123):
    for abundant in abundants:
        if abundant >= i and is_abundant(i + abundant):
            sums += i
print(sums)

The == True tests are unecessary and were removed.
Naming was improved: isabundant → is_abundant.
The long statement in is_abundant was split up.

Now we can think about how this could be optimized.

One subproblem is calculating all divisors of a number. We could put the relevant code into its own function. We can furthermore exploit that if n % i == 0, then there must be another integer k so that n == i * k. This means that we only have to look at a lot less numbers: only the range(2, 1 + int(sqrt(n))) is interesting.

def divisors(n):
    """
    Returns all nontrivial divisors of an integer, but makes no guarantees on the order.
    """
    # "1" is always a divisor (at least for our purposes)
    yield 1

    largest = int(math.sqrt(n))

    # special-case square numbers to avoid yielding the same divisor twice
    if largest * largest == n:
        yield largest
    else:
        largest += 1

    # all other divisors
    for i in range(2, largest):
        if n % i == 0:
            yield i
            yield n / i

We can now rewrite our is_abundant to the simpler:

def is_abundant(n):
    if n < 12:
        return False
    return sum(divisors(n)) > n

Later, in your main loop, you are doing a rather weird calculation. What were we supposed to do?

  Find the sum of all the positive integers which cannot be written as the sum of two abundant numbers.

We furthermore know that all integers above 28123 can be written as such a sum. Thus we have to look at the range(1, 28123 + 1)! How can we decide if a number n can be written as a sum of abundant numbers i and k? There exists any abundant number i with the constraint i < n, and another abundant number with the constraints k < n and n - i == k. Here is one clever way to write this:

def is_abundant_sum(n):
   for i in abundants_smaller_than_n:
       if (n - i) in abundants_smaller_than_n:
           return True
   return False

Because we don't want to calculate the abundants_smaller_than_n each time, we just take all possible abundants and bail out if we get larger than n:

def is_abundant_sum(n):
   for i in abundants:
       if i > n:  # assume "abundants" is ordered
         return False
       if (n - i) in abundants:
           return True
   return False

where abundants = [x for x in range(1, 28123 + 1) if is_abundant(x)].

Now, all that is left to do is to sum those numbers where this condition does not hold true:

sum_of_non_abundants = sum(x for x in range(1, 28123 + 1) if not is_abundant_sum(x))

We could perform one optimization: abundants is a list, which is an ordered data structure. If we search for an element that is not contained in the list, all elements would have to be searched. The set() data structure is faster, so:

abundants_set = set(abundants)
def is_abundant_sum(n):
   for i in abundants:
       if i > n:  # assume "abundants" is ordered
         return False
       if (n - i) in abundants_set:
           return True
   return False


Yet Another Regex-Based MarkDown to HTML converter I wrote a simple script to translate (a subset of) StackExchange markdown to HTML. I used Regex for this task, but luckily I only had to use simple patterns.

Modularization

I also wrote a Regexp-generating function as I noticed that in Markdown you are always looking for things inside such and such symbols, I feel like it simplified the code.

What I do not like

The massive and mindless use of .tr to remove surrounding mark-up before applying HTML tags.

Tests

The code has 9 testcases, one for each feature.

Code / Tests ratio.

The code is pretty compact, totalling 16 lines of logic supported by 50 blank/end-only/comments/tests lines.

Testing bug

I used my own testing framework, but sadly it has a bug that does not allow to use # inside tests, so I replaced it with 35.chr

Links trickiness

The code is a bit longer when handling HTML links as they are a bit more complex than tags like bold or italics

Limitations

The code does not escape tags contained in:

``

It also has no support for lists of any kind and gives results visually more ugly (still correct) than the markdown implemented on this site.

It works on my basic example.

(On-topic, as I acknowledge that this code is not perfect but I do not ask you to fix it.)

The code

require "arrow_test"

# Returns a Regex that matches anything inside the delimiter.
# The given delimiter is escaped to allow the use of special characters.
#
# anything_inside("**") #=> /\*\*.*\*\*/
def anything_inside(start, ending=nil)
  start = "\\" + start.chars.join("\\")
  ending = start if ending == nil
  Regexp.new(start + ".*" + ending)
end

# This function translates MarkDown to HTML.
#
# markdown_to_html("plaintext") #=> "plaintext"
# markdown_to_html("**bold only**") #=> "bold only"
# markdown_to_html("_italics only_") #=> "italics only"
# markdown_to_html("`code only`") #=> "code only"
# markdown_to_html("    code by indentation") #=> "code by indentation"
# markdown_to_html("> To be or not to be\n\n") #=> "<q> To be or not to be</q>"
# markdown_to_html(35.chr + "Title\n") #=> "
Title
"
# markdown_to_html("\npara\n") #=> "\n
para
\n"
# markdown_to_html("[foo](http://foo.com)") #=> "foo"
def markdown_to_html(text)
  text
    .gsub(anything_inside(">", "\n\n"))  {|txt| "<q>#{txt.sub(/^>*/,"").tr("\n\n", "")}</q>" }
    .gsub(anything_inside("**")) {|txt| "#{txt.tr("**","")}"}
    .gsub(anything_inside("_")) {|txt| "#{txt.tr("_","")}"}
    .gsub(anything_inside("`"))  {|txt| "<code>#{txt.tr("`","")}"}
    .gsub(anything_inside("#", "\n")) {|txt| "
#{txt.tr("\n","").sub(/^\#*/,"")}
"}
    .gsub(anything_inside("\n")) {|txt| "\n
#{txt.tr("\n","")}
\n"}
    .gsub(/\ \ \ \ .*/) {|txt| "#{txt.strip}"}
    .gsub(/\[.*\]\(.*\)/) do |txt|
       to_show = txt.match(/(\[.*\])/)[1].tr("[","").tr("]","")
       url = txt.match(/(\(.*\))/)[1].tr("(","").tr(")","")
       "#{to_show}"
    end
end

if __FILE__ == $0
  arrow_test($0, true)
  File.write("example.html", markdown_to_html("""
# A nice example.

All the time, the site [example](www.example.com) amuses me with its beauty.

I felt a **strong attachment** to it, regardless of its simplicity.

The site is an  _example_ as few things can be, as Lapalisse said:

> What is an example, is an example.

Such beatiful a site may be used in examples such as:

**Python**

    import webbrowser
    webbrowser.open(\"www.example.com\")

And now, for something completely different, `inline code`.

> The great man may not have ideas, but will always write examples.

_And that is all,_ I hope you enjoyed reading this example, good day.

"""))
end

The whole approach of using regexes to parse Markdown and convert it into HTML is broken.

You have no way of knowing whether any html you generate with one call to gsub is violating the constraints of the HTML containing it.
After the first gsub call, you iteratively apply it to text which now contains some html.  You may mistakenly replace parts of an html element with an html element.

Some of the individual gsub calls are fragile in themselves but none of that compares to the basic flaws of the approach.

Having said that, the way you are using regexes is duplicating code.  You have typed each matched pattern twice: first in the regex, then in the closure.  This would not be necessary if you used capture groups and back-references.


Fastest way to find and replace inside a large amount of files Currently I am looping through dependencies of objects, and I am trying to replace GUID (unity object ID's) inside the files.
Right now I am using the following code, any suggestions for improvement ?

public class SendToThread
{
    public int localID = 0;
    public string oldID;
    public string newID;
    public List<string> dependencyAssetPath = new List<string>();
    public string applicationPath;
}

public void Charlie()
{
    //toUseData is a List<SendToThread>
    for (int i = 0 ; i < toUseData.Count; i++)
    {
        while (toUseData[i].dependencyAssetPath.Count >= 1)
        {
            string[] lines = File.ReadAllLines(toUseData[i].applicationPath + toUseData[i].dependencyAssetPath[0]);

            for (int e = 0; e < lines.Length; e++)
            {
                DebugVoid(lines.Length.ToString());
                lines[e].Replace(toUseData[i].oldID.ToString(), toUseData[i].newID.ToString());

                if (e == lines.Length - 1)
                {
                    DebugVoid(toUseData[i].oldID.ToString() + "  " + toUseData[i].newID.ToString());
                    File.WriteAllLines(toUseData[i].applicationPath + toUseData[i].dependencyAssetPath[i], lines);
                }
            }
            toUseData[i].dependencyAssetPath.RemoveAt(0);
        }
    }
}

Building on what Heslacher and Magus have said:

You should not make publically accessible non-readonly collections. So I've converted those to readonly collections. It didn't appear to change how you were accessing them anyway.

I've also swapped your outermost for loop for a foreach, which makes the code slightly cleaner and I've swapped your while loop for a foreach, because it appears to just be iterating over the collection. However this assumes that

File.WriteAllLines(toUseData[i].applicationPath + toUseData[i].dependencyAssetPath[i], lines);

is a bug and should instead be

File.WriteAllLines(toUseData[i].applicationPath + toUseData[i].dependencyAssetPath[0], lines);

If not, then the while loop should be used.

Resulting version:

public class SendToThread
{
    private int localId = 0;
    public int LocalID
    {
        get{ return localId;}
        set{ localId = value;}
    }
    public string OldID {get; set;}
    public string NewID {get; set;}
    private readonly List<string> dependencyAssetPath = new List<string>();
    public List<string> DependencyAssetPath
    {
        get{ return dependencyAssetPath;}
    }
    public string ApplicationPath {get; set;}
}

public void ReadAssets()
{
    //toUseData is a List<SendToThread>
    foreach(var toUse in toUseData)
    {
        foreach(var assetPath in toUse.DependencyAssetPath)
        {
            string[] lines = File.ReadAllLines(toUse.ApplicationPath + assetPath);

            for (int e = 0; e < lines.Length-1; e++)
            {
                DebugVoid(lines.Length.ToString());
                lines[e].Replace(toUse.OldID, toUse.NewID);
            }

            DebugVoid(toUse.OldID + "  " + toUse.NewID);
            File.WriteAllLines(toUse.ApplicationPath + assetPath, lines);
        }
        toUse.dependencyAssetPath.Clear();
    }
}


Queue that connects multiple producers and multiple consumers The scenario is about processing 'Message' objects. Producer creates them and Consumer does the consumption.

class Message{...}

class MessageBatch{

    public MessageBatch(Collection<Message> messages){...}

    public List<Message> getMessages(){...};

    //more utility methods like getMessageIds(), etc.
}

The following is a pipeline object (queue) that connects multiple producer threads and multiple consumer threads. Both my producers & consumers are I/O bound (database) and that justifies this attempt.

API requirements

[A] Ability to add & get batch of Message objects.
[B] Consumer has a way to realize that no more elements are expected.

public class MessagePipeline {

    //[1]; to support API requirement [B]
    public static final MessageBatch SENTINEL_BATCH = new MessageBatch(Collections.<Message>emptyList());

    private BlockingQueue<Message> messageQueue = new LinkedBlockingQueue<>();

    // to support API requirement [B]
    private CountDownLatch producersWorkCompletionLatch;

    public MessagePipeline(int numOfProducerThreads) {
        this.producersWorkCompletionLatch = new CountDownLatch(numOfProducerThreads);
    }

    public boolean add(MessageBatch batch) {
        isWriteAllowed(); // UPDATE: this step is misleading and thinking of removing this.
        return messageQueue.addAll(batch.getMessages());
    }

    // returns a batch containing up to maxSize elements.
    public MessageBatch takeBatch(int maxSize) {
        List<Message> messages = drainIfMoreElementsExpected(new ArrayList<Message>(), maxSize);

        return messages.isEmpty() ? SENTINEL_BATCH : new MessageBatch(messages);
    }

    //[2]: Producer should invoke this at the end.
    public void notifyCompletion() {
        producersWorkCompletionLatch.countDown();
    }

    public boolean isMoreElementsExpected() {
        boolean isExpected = !(producersWorkCompletionLatch.getCount() == 0 && messageQueue.size() == 0);
        return isExpected;
    }
    //[3] : This is CPU bound waiting. TODO: Make it I/O bound.
    private List<Message> drainIfMoreElementsExpected(List<Message> messages, int maxSize) {
        while (messages.isEmpty() && isMoreElementsExpected()) {
            messageQueue.drainTo(messages, maxSize);
        }
        return messages;
    }

    /**
     * Assures write-operations 'happen-before' producer-threads-completion.
     */
    private void isWriteAllowed(){
        if(producersWorkCompletionLatch.getCount() == 0){
            throw new UnsupportedOperationException("New elements are not accepted after all producer threads notify of completion");
        }
    }
}

Questions

I'm looking forward to code review in general.
Do approaches [1] and [2] seem right?
[3]: Am I right that it is CPU bound? How do I make it I/O bound waiting? Introducing locks seem to make it more complicated.

I'm in the middle of writing unit tests. It's good to know if I missed anything because unit-testing multi-threaded code tells you that the code is broken only one in million times and doesn't tell you why!

Since you say that your whole system is based on message passing you should take a serious look at akka.   You get a rock solid message passing system, and you can even spread it on multiple machines in the future.

I have not checked in detail, but you have some very serious concurrency bugs.  For example, add checks if writing is allowed, then writes to the queue.  The problem is that you did not synchronize add, so the check might pass, but before calling the next line, some other thread might bring the count down latch down to zero.  BlockingQueue is thread-safe, but it is far from enough to make your code thread-safe.

I don't like the MessagePipeline api: it is too complex.  I really don't see the advantage of writing or taking "batches" of messages.  If the reader or writers want to process many messages, just let them read or write many messages themselves by processing their many messages in a loop.  The api itself should only read/write one message at a time.  In that case, your whole MessagePipeline is superfluous since a simple BlockingQueue is exactly what you want.  You will also avoid many concurrency bugs.  You would also need to keep the CountDownLatch or something similar to let the consumer know when it's over.


Where best to declare a counter variable to track number of guesses? An exercise asks that we extend the below random number generator app to print to the console how many times the user guessed before guessing the correct random number.

I first declared what I call a counter variable numberOfGuesses inside the keepPlaying while loop and then inside the continueGuessing while loop I increment this number by 1 before I check the user's guess against the random number generated. This works, but then I tried declaring numberOfGuesses at the top of the program and commented out the former placement, as you can see below, and this also worked.

My question is, which is the better way of these two to solve this, and is there another even better way to solve this problem by using another loop perhaps or would that be silly/overkill/not workable?

var randomNumber = 1
var numberOfGuesses = 0
var continueGuessing = true
var keepPlaying = true
var input = ""

while keepPlaying {
    //get a random number between 0 - 100
    randomNumber = Int(arc4random_uniform(101))
    print("The random number to guess is: \(randomNumber)")

    //var numberOfGuesses = 0 //can go here, or in global variables up top!

    while continueGuessing { //
        print("Pick a number between 0 and 100.")
        //get keyboard input, and trim the new line
        input = String(bytes: FileHandle.standardInput.availableData, encoding: .utf8)!
        input = input.trimmingCharacters(in: .whitespacesAndNewlines)

        if let userGuess = Int(input) {
            numberOfGuesses += 1
            if userGuess == randomNumber {
                continueGuessing = false //terminator for this while loop
                print("Correct number!")
                print("You have made this number of attempts to get it right: \(numberOfGuesses)")
            } else if userGuess > randomNumber {
                //user guessed too high
                print("Your guess is too high!")
            } else {
                //no reason to check if userGuess < randomNumber. It has to be.
                print("Your guess is too low!")
            }
        } else {
            print("Invalid guess, please try again.")
        }
}

print("Play again? Y or N")
    input = String(bytes: FileHandle.standardInput.availableData,encoding: .utf8)!
    input = input.trimmingCharacters(in: .whitespacesAndNewlines)

    if input == "N" || input == "n" {
        keepPlaying = false //a way to exit while loop
        print("You have played the game this number of times: \(numberOfGuesses)")

    }
    continueGuessing = true
}

As a general rule, you want to use the narrowest possible scope for all variables. We do this so that:

The declaration is close to where we use it, making it easier to reason about why the variable exists.
It minimizes the chance of unintended consequences where the variable is accidentally updated somewhere else.

In this case, it’s so trivial that it doesn’t matter that much, but as your code increases in complexity, this becomes increasingly important. This practice of restricting variables to the narrowest possible scope becomes a central precept in writing safe code: It’s much harder to have unintended consequences or accidentally state changes if variables have narrow scopes.

So, if numberOfGuesses represents how many guesses it took you to guess a  particular random number, then it should be inside the keepPlaying loop. But if you also wanted to keep track of the total number of guesses for all the times you played, then that (probably best kept track in another variable, perhaps, totalNumberOfGuesses) would be outside this outer loop.

By the way, while you’re at it, the continueGuessing and randomNumber, belong inside this keepPlaying loop, too.

By the way,

If you define variables only used within the scope of a loop, it saves you from having to reset that variable for the next iteration. For example, right now you are resetting continueGuessing back to true as the last step in the keepPlaying loop; if you define continueGuessing within the loop, then you wouldn’t have to reset it again at the end of the loop, resulting in simpler code.

In your code, you make reference to “globals”. As a general rule, we try to minimize the use of globals because as as apps scale, globals can make it hard to test, harder to reason about state changes, etc. See Why is Global State so Evil or google “globals are evil”.

Some unrelated observations:

Rather than

let randomNumber = Int(arc4random_uniform(101))

I’d instead recommend:

let randomNumber = Int.random(in: 0...100)

It is equivalent, but arguably is more intuitive. (This would be especially true if you wanted to go from 1 to 100 instead, the arc4random_uniform code would become even less intuitive, whereas random(in: 1...100) is perfectly natural and trivial to reason about.
When you have a loop that you are always going to do at least once, rather than a while loop, we’d often consider repeat-while. E.g. rather than

var shouldPlayAgain = true
while shouldPlayAgain {
    ...

    shouldPlayAgain = ...
}

You might do:

var shouldPlayAgain: Bool
repeat {
    ...

    shouldPlayAgain = ...
} while shouldPlayAgain

Technically the latter is a tad more efficient, but the main benefit is that it makes your intent (to check at the end) absolutely clear.
At the end, you appear to want to be printing number of times the game was played, but you’re printing the number of guesses.

E.g. if you played twice, the first time requiring 3 guesses and the second time requiring 4 guesses, do you want to say “you played 2 times” or do you want to say “you guessed a total of 7 times”?


Batch file command looper Just a fun batch file I have made. I wanted an easy way to loop any arbitrary command several times: loopme.bat

@echo off
SETLOCAL
echo LoopMe v1.0

IF "%~1"=="/?" (GOTO HELP)
IF "%~1"=="" (GOTO HELP)
IF "%2"=="" (SET TIMEOUT=1) ELSE (SET TIMEOUT=%2)
SET COMMAND=%~1
SET COUNTER=0

:RUN
SET /A COUNTER=%COUNTER% + 1
CMD /C "%COMMAND%"
echo.
echo Command  : "%COMMAND%"
echo Times run: %COUNTER%
CHOICE /C QC /N /T %TIMEOUT% /D C /M "Press Q to quit, otherwise wait %TIMEOUT% second(s) to run again."
IF "%ERRORLEVEL%"=="1" (GOTO EOF) ELSE (GOTO RUN)

:HELP
echo Perform the same command in a loop.
echo Commands containing ^& or ^| must be escaped with ^^
echo Commands containing variables must be escaped with ^^ in the varible name
echo Optionally specify loop time. Default is 1 second.
echo.
echo Usage: loopme dir 2
echo               [run dir in current, loop 2 seconds]
echo.
echo        loopme "cls ^& netstat -an ^| find ".109""
echo               [clear screen then run netstat with find filter, loop 1 second]
echo.
echo        loopme "echo %%^time%%"
echo               [display the time, loop 1 second]
echo
echo.

:EOF

Any improvements or suggestions?

p.s this is my new way to annoy colleagues who leave their workstation unlocked:

loopme "color %^time:~9,2% ^&echo. ^& echo. ^&echo. ^& echo YOU HAVE BEEN HACKED!! ^& echo. ^&echo."

If you want an easy way to loop an arbitrary command several times...

set /a loop=100
for /L %%P in (1,1,%loop%) do (
    echo Looping for one hundred times
)

The first line is obvious.

The second line is a bit harder - for /L specifies that for every number in a certain set, the batch file will do something. %%P is a parameter that holds what number the for loop is at currently - it can be any letter you like (and some other characters but they are dangerous so stick with letters). in (1,1,%loop%) specifies that this is for every number from 1 to %loop%, increasing by 1 every time. do ( ... ) obviously tells the batch file what it needs to do.

So, in every number from 1 to 100, it will echo Looping for one hundred times. This will obviously scroll down the page seeing as we never clear the screen with cls.

Once a for loop has reached its final number, the batch file continues on executing the commands following the final ) as you would expect.

I can expand this answer if you want to know what you're doing wrong and right in your own code example, but I pretty much just did the same thing in 4 lines which should suffice.

And here comes the expansion. Please note that your code is pretty close to perfect, and these tips only encourage good scripting habits.

To begin with, you spelt variable as varible in line 3 of the :HELP label.

First line of the :RUN label:

set /a counter=%counter% + 1

Instead of this, one can do

set /a counter+=1

This isn't incredibly helpful at the moment, but when you want to add a variable to another variable...

set /a varA+=%varB%

This also works for other operators...

set /a varA*=%varB%

And slightly more complex equations...

set /a varA-=%varB% * %varC%

Which will evaluate %varB% * %varC%, then subtract the result from varA

The command CMD /C "%COMMAND%" is not entirely necessary to my knowledge - simply writing %COMMAND% on a new line will cause the batch parser to expand it, so if %COMMAND% is set as exit, the line will read, at run time, exit. This enables you to use this method rather than the entire CMD /C method.

Another concern that could become more than just a concern is your method for checking the errorlevel - there's a problem. Checking with quotes and == denotes a string comparison - never good when you're dealing with two numbers. So you'll need a fix for this problem, and luckily it's quite simple.

if %errorlevel% EQU 1 ( ... )

This tests for a mathematical equivalence - always good when you're dealing with two numbers. All the operators for mathematical equivalencies are...

EQU - 'equal to'
NEQ - 'not equal to'
GTR - 'greater than'
LSS - 'less than'
GEQ - 'greater than or equal to'
LEQ - 'less than or equal to'

And finally, using if varA==varB (goto label) is not necessary - it works just as well as if varA==varB goto label. The only time you need brackets is when you plan on conditionally executing multiple lines or if you need to use else (just to be safe). When using brackets, always remember to balance them. Indenting isn't required, but it gets really confusing when you start nesting if statements and for loops without it. An example of executing multiple lines...

if %varA% GTR %varB% (
    echo varB is less than varA
    if %varA% GTR %varC% (
        echo varC is less than varA
    ) else (
        echo varC is greater than varA
)

To conclude, your code obviously works fine, but some habits die hard in weird circumstances later on, so it's best to get used to using common conventions (they exist for a reason!). As a side note, remember that batch scripting is not case sensitive. Anything I wrote here, along with anything you wrote, can be written in the opposite case - it has no difference.


Handle NullValueInNestedPathException I am looking for a elegant solution to this. I am new to Spring framework. I have a method:

public String getStringProperty(String property) {
        Object value = SomeWrapper.getPropertyValue(property);
        return (value != null) ? String.valueOf(value) : "";
    }

The immediate upstream method is:

public String getValueForHeader(String header) {
        return getStringProperty(LOOKUP_TABLE.get(header));
    }

Where LOOKUP_TABLE is an ImmutableMap (google guava). An example representation of it:

private final ImmutableMap<String, String> LOOKUP_TABLE = ImmutableMap.<String, String>builder()
            .put(LogHeaders.Phone, "Person.phone")
            .put(LogHeaders.Address, "Person.personal.address")
            .put(LogHeaders.Email, "Person.billing.contact.primary.email")
            .put(LogHeaders.Ip, "Person.ip")
            .build();

So, when I call getStringProperty method with property "Person.billing.contact.primary.email" I get a "" String. My aim here is to return empty String when some property does not exist. For example if "Person.billing.contact.primary.email" does not have "email" set in address an empty String is returned. Now if "contact" or "primary" is not set I would get a NullValueInNestedPathException but I should return an empty String.

One way to deal with this is to introduce the try catch lock in this method and have the catch block return empty string.

public String getStringProperty(String property) {
    try {
        Object value = transactionWrapper.getPropertyValue(property);
        return (value != null) ? String.valueOf(value) : "";
    } catch (NullValueInNestedPathException e) {
        return "";
    } catch (NotReadablePropertyException nr) {
        LOGGER.error("Invalid property {} ", property);
        return "!invalid property!";
    }
}

The NotReadablePropertyException is to warn me when I am trying to get some property which does not exist (or getter for that property is not readable).

The other solution could be to throw same exception and let methods upstream worry.

Is there some alternative, elegant way to handle this inside my getStringProperty() method ?

Returning "" is reasonable, since that is what you want to use as the default value.

Returning the special value "!invalid property!" to indicate an error might be allowable only in the following limited circumstance:

This is view-layer code, not your model, not middleware, not an API, and
You actually want that special string to appear in the UI as a placeholder, and
The placeholder text is to be presented in the normal font and color.

If any of those conditions is not applicable, then the special string is absolutely inappropriate. That is precisely what exceptions are for — exceptional cases. If you present exceptional cases as if they were legitimate data, then you end up with bugs like this one, where a person whose name is Null causes grief.