Connection String Decorating I found myself in a situation where it is necessary to often decorate connection strings with some kind of metadata, like an extra DB Provider attribute, read it back, and remove the decoration without damaging original content.

Here is how it looks like:

        var cs = new MsSqlConnectionStringBuilder
        { User = "dmitry", Password = "Password!" }
        .Build();

        Console.WriteLine(cs); // Password=Password!;User=dmitry;

        cs = cs.With("Provider", "MSSQL");
        Console.WriteLine(cs); // Provider=MSSQL;Password=Password!;User=dmitry;
        Console.WriteLine(cs["Provider"]); // MSSQL

        cs = cs.Excerpt("Provider"); // Password=Password!;User=dmitry;
        Console.WriteLine(cs);

Where custom MsSqlConnectionStringBuilder is:

public class MsSqlConnectionStringBuilder
{
    public string User { get; set; } = "sa";
    public string Password { get; set; }

    public ConnectionString Build() =>
        ConnectionString.Empty
            .With("User", User ?? throw new InvalidOperationException())
            .With("Password", Password ?? throw new InvalidOperationException());
}

And library code is:

public sealed class ConnectionString
{
    public static explicit operator ConnectionString(string text) => Parse(text);
    public static implicit operator string(ConnectionString connectionString) => connectionString.ToString();

    public static readonly ConnectionString Empty = new ConnectionString(ConnectionParameter.None);

    public static ConnectionString Parse(string text, string regex = "(?<name>[^=]+)=(?<value>[^;]);?") =>
        new ConnectionString(
            new Regex(regex ?? throw new ArgumentNullException(nameof(regex)))
                .Matches(text ?? throw new ArgumentNullException(nameof(text)))
                .Select(m => new ConnectionParameter(m.Groups["name"].Value, m.Groups["value"].Value)));

    ConnectionString(IEnumerable<ConnectionParameter> parameters)
    {
        Parameters = parameters.ToArray();
    }

    IEnumerable<ConnectionParameter> Parameters { get; }

    public string this[string name] => Parameters
        .FirstOrDefault(p => p.Name.ToUpper() == name.ToUpper())?.Value;

    public ConnectionString With(string name, object value) =>
        new ConnectionString(Parameters
            .Prepend(new ConnectionParameter(name, $"{value}")));

    public ConnectionString Excerpt(string name) =>
        new ConnectionString(Enumerable.Concat(
            Parameters.TakeWhile(p => p.Name.ToUpper() != name.ToUpper()),
            Parameters.SkipWhile(p => p.Name.ToUpper() != name.ToUpper()).Skip(1)));

    public override string ToString() => ToString("{0}={1};");

    public string ToString(string format) =>
        string.Join("", from p in Parameters
                        select string.Format(format, p.Name, p.Value));
}

And:

sealed class ConnectionParameter
{
    public static readonly ConnectionParameter[] None = new ConnectionParameter[0];

    internal ConnectionParameter(string name, string value)
    {
        Name = name ?? throw new ArgumentNullException(nameof(name));
        Value = value ?? throw new ArgumentNullException(nameof(value));
    }

    public string Name { get; }
    public string Value { get; }
}

What struck me most are these lines:

p.Name.ToUpper() != name.ToUpper()

I find it's always better to use the Equals overload that accepts a StringComparer and set it as a readonly field for the entire class rather then hoping you don't forget ToUpper everywhere.

This is a minor thing but I find that Excerpt should be called Without because it does not actually create an excerpt where you can say what you want to have but rather the other way around. The parameter says what should be excluded from the string. I also think it should take a params string[] argument.

new Regex(regex ?? throw new ArgumentNullException(nameof(regex)))

I find newing a Regex should be reserved for initializing a field. In a situation like here the static Regex.Matches would be more suitable - unless there is a reason why you chose new over the static method.

public string ToString(string format) =>

This one I don't like. It's not a standard .net practice. If you want to use a formattable ToString then the class should implement the IFormattable interface correctly... or you should make this one private.

Besides having this helper method to just do

ToString("{0}={1};")

is a little bit overengineered. How many of such formats do you have? A single ToString() would do too.

ConnectionParameter should be ConnectionStringField (or Property) to clearly communicate it belongs to a connection string.

I'm not sure if you know that there is already a SqlConnectionStringBuider and most providers have their own implementations.

I prefer to write extensions against this type rather than parsing everything myself. It also validates the keys you use so you cannot use anything that isn't supported... well, unless it was your intention and a reason for the custom builder.


WebAPI global exception handler There is a global exception handler in our WebAPI application that looks like the following:

public class ApiExceptionHandler : IExceptionHandler
{
    public async Task HandleAsync(ExceptionHandlerContext context, CancellationToken cancellationToken)
    {

        var exceptionType = context.Exception.GetType();

        if (exceptionType == typeof(ResourceNotFoundException))
        {
            context.Result = new ResponseMessageResult(
                context.Request.CreateResponse(HttpStatusCode.NotFound, context.Exception.Message));
        }

        else if (exceptionType == typeof(UserNotFoundException))
        {
            context.Result = new ResponseMessageResult(
                context.Request.CreateResponse(HttpStatusCode.Unauthorized, context.Exception.Message));
        }

        else if (exceptionType == typeof(UserAlreadyExistsExeption))
        {
            context.Result = new ResponseMessageResult(
                context.Request.CreateResponse(HttpStatusCode.Conflict, context.Exception.Message));
        }

        ...

        else
        {
            context.Result = new ResponseMessageResult(
                context.Request.CreateResponse(HttpStatusCode.InternalServerError, "An unexpected error occured"));
        }
    }
}

And each exception has the following form:

public class UserAlreadyExistsExeption: Exception
{
    public UserAlreadyExistsExeption()
    {
    }

    public UserAlreadyExistsExeption(string message) : base(message)
    {
    }

    public UserAlreadyExistsExeption(string message, Exception inner) : base(message, inner)
    {

    }
}

And can be thrown in the code like the following:

//If user exists
throw new UserAlreadyExistsExeption("User already exists");

I was thinking of making all our custom exceptions implement a IHasHttpErrorCode so each one has its own HttpErrorCode - usage like the following:

Interface:

public interface IHasHttpErrorCode
{
    HttpStatusCode GetHttpStatusCode();
}

Exception:

public class UserAlreadyExistsExeption: Exception, IHasHttpErrorCode
{
    public UserAlreadyExistsExeption()
    {
    }

    public UserAlreadyExistsExeption(string message) : base(message)
    {
    }

    public UserAlreadyExistsExeption(string message, Exception inner) : base(message, inner)
    {

    }

    public HttpStatusCode GetHttpStatusCode() {
        return HttpStatusCode.Conflict;
    }
}

Global Error Handler:

public class ApiExceptionHandler : IExceptionHandler
{
    public async Task HandleAsync(ExceptionHandlerContext context, CancellationToken cancellationToken)
    {

        var exceptionType = context.Exception as IHasHttpErrorCode;

        if (customException != null) {
            context.Result = new ResponseMessageResult(
                context.Request.CreateResponse(customException.GetHttpStatusCode(), context.Exception.Message));
        }
        else
        {
            context.Result = new ResponseMessageResult(
                context.Request.CreateResponse(HttpStatusCode.InternalServerError, "An unexpected error occured"));
        }
    }
}

Is this a valid approach to improve the existing code? Is there anything else I should look to improve?

Using the interface is definitely an improvement, but what I lack in your code is common base exception, something like ServerException that could be used to distinguish other exceptions (e.g. IndexOutOfRangeException or whatever) from your exceptions (useful for catching).

That common base could be made abstract to force you to define HttpStatusCode

public abstract class ServerException: Exception {
//  standard constructors...
    public abstract HttpStatusCode GetHttpStatusCode();
}

Now, if you derive any custom exception from this (as you should, maybe creating some other abstract derived exceptions for some groups of problems), then you have to override (define) the code (or it would have to remain abstract and you won't be able to throw it).

public class UserAlreadyExistsExeption: ServerException {
//  again standard .ctors
    override public HttpStatusCode GetHttpStatusCode() => HttpStatusCode.Conflict;
}

Alternative would be to use public get-only property and initialize it in the constructor, maybe even having one exception type (and use try-catch-when), but I am not sure that can be used in your code (would require perfect 1-to-1 match between exception types and status codes).

Make message default

public class UserAlreadyExistsExeption: Exception, IHasHttpErrorCode
{
    public UserAlreadyExistsExeption(): base("User already exists") {}
}

and

throw new UserAlreadyExistsExeption();

instead of

throw new UserAlreadyExistsExeption("User already exists");

Maybe even handling localization in the default constructor, if needed.


PHP, would this class pass the single responsibility principle? I have a user class comprised of the code below. It has functions to insert a user, update a user, delete a user, look up a user, and search for users. What I'm wondering is if I this class would pass the Single Responsibility Principle as stated here (http://en.wikipedia.org/wiki/Single_responsibility_principle).

class user{
    $first_name = "";
    $last_name = "";
    $physical_address1 = "";
    $physical_address2 = "";
    $physical_city = "";
    $physical_state = "";
    $physical_zip = "";
    $mailing_address1 = "";
    $mailing_address2 = "";
    $mailing_city = "";
    $mailing_state = "";
    $mailing_zip = "";
    function __construct($array){
        if(!isset($array['first_name']) && !empty($array['first_name'])){
            $this->first_name = "";
        }
        if(!isset($array['last_name']) && !empty($array['last_name'])){
            $this->last_name = "";
        }
        if(!isset($array['physical_address1']) && !empty($array['physical_address1'])){
            $this->set_physicalAddress(array(
                "address_1"=>$array['physical_address1'],
                "address_2=>$array['physical_address2']",
                "city"=>$array['physical_city'],
                "state"=>$array['physical_state'],
                "zip"=>$array['physical_zip']
            ));
        }
        if(!isset($array['mailing_address1']) && !empty($array['mailing_address1'])){
            $this->set_mailingAddress(array(
                "address_1"=>$array['mailing_address1'],
                "address_2=>$array['mailing_address2']",
                "city"=>$array['mailing_city'],
                "state"=>$array['mailing_state'],
                "zip"=>$array['mailing_zip']
            ));
        }
    }
    function set_physicalAddress($array){}
    function get_physicalAddress(){}
    function set_mailingAddress($array){}
    function get_mailingAddress(){}
    static function insert($array){}
    static function update($array){}
    static function delete($int){}
    static function lookup($array){}
    static function search($array){}
}

$obj = new user(array(
"first_name"=>"John",
"last_name"=>"Smith"
));

The purpose of having the constructor parameter value be an array is that I feed it only the values I want without having to deal with errors caused by not setting a parameter. Instead of setting the parameters to null it's easier to just not put them in the array.

I don't think the user class you provided satisfies the Single Responsibility Principle.

I can see a few situations that may require changes to the user class.

You want to change what information about the user is stored. This could happen if you need to allow a title (Mr., Mrs., etc.) and/or a middle name. Doing so would require a change to the user class.
You want to create a new class that also contains address information. The new class would likely duplicate code that is already present in the user class.
You want include the user's country with the physical and mailing addresses. Doing so would require a change to the user class. It would also require changes to the class in #2.
You want to change where the information is stored. You may decide you want to be able to test your class without needing to connect to the database (assuming you're connecting to a database). Doing so would require a change to the CRUD methods (insert, update, delete, lookup, and search) of your class.

(1) is related to the user class and changing the class is expected. No problem here.

(2), (3), and (4) are really not the responsibility of the user class, yet they would probably require changes to the user class.

You should consider creating an additional "address" class that can be reused for the physical and mailing addresses for the user. The user class could contain instances of the address class, and would not require changes if the address functionality changed. Any new classes that need address information could reuse this class.

You should also consider creating a new class (let's call it userProvider) that is responsible for the CRUD functions. This class could have the CRUD methods with a user object as a parameter. Your user class could still define the CRUD methods if you want, but would simply pass the user instance to the userProvider class, which would be responsible for storing/retrieving the user information.


Sorting and update 2D Dimension Question:
Compare and update the inventory stored in a 2D array against a second 2D array of a fresh delivery. Update the current existing inventory item quantities (in arr1). If an item cannot be found, add the new item and quantity into the inventory array. The returned inventory array should be in alphabetical order by item.

And my code is:

function updateInventory(arr1, arr2) {

  var newArr = arr1.concat(arr2);

  for(var i = 0; i<newArr.length; i++){
    for(var j = i+1; j<newArr.length; j++){
      if(newArr[i][1] === newArr[j][1]){
          newArr[i][0] += newArr[j][0];
          var index = newArr.indexOf(newArr[j]);
          newArr.splice(index,1);
      }
    }
  }

  return newArr.sort(function(a,b){
    return (a[1] < b[1] ? -1 : (a[1] > b[1] ? 1 : 0));
  });

}

var curInv = [
  [21, "Bowling Ball"],
  [2, "Dirty Sock"],
  [1, "Hair Pin"],
  [5, "Microphone"]
];

var newInv = [
  [2, "Hair Pin"],
  [3, "Half-Eaten Apple"],
  [67, "Bowling Ball"],
  [7, "Toothpaste"]
];

updateInventory(curInv, newInv);

Output =  [
  [ 88, 'Bowling Ball' ],
  [ 2, 'Dirty Sock' ],
  [ 3, 'Hair Pin' ],
  [ 3, 'Half-Eaten Apple' ],
  [ 5, 'Microphone' ],
  [ 7, 'Toothpaste' ]
];

Can this function be improved?

You could do better on your time complexity. Your current method runs in O(n^2), where n is the length of the concatenated newArr. However, if you sorted arr1 and arr2 first and then merged them, you could reduce the time complexity to O(n*log(n)), where n is the length of the longer of the two arrays:

function compareInventoryItems(a, b) {
  return (a[1] < b[1] ? -1 : (a[1] > b[1] ? 1 : 0));
}

function updateInventory(arr1, arr2) {
  arr1.sort(compareInventoryItems); // O(n*log(n))
  arr2.sort(compareInventoryItems); // O(m*log(m))

  var newArr = [],
    i1 = 0,
    i2 = 0;

  while (i1 < arr1.length && i2 < arr2.length) { // O(n+m)

    switch (compareInventoryItems(arr1[i1], arr2[i2])) {
      case -1:
        newArr.push(arr1[i1++]);
        break;
      case 1:
        newArr.push(arr2[i2++]);
        break;
      case 0:
        arr1[i1][0] += arr2[i2][0];
        newArr.push(arr1[i1]);
        i1++;
        i2++;
        break;
    }
  }
  while (i1 < arr1.length) {
    newArr.push(arr1[i1++]);
  }
  while (i2 < arr2.length) {
    newArr.push(arr2[i2++]);
  }

  return newArr;
}

// Overall complexity = O(n*log(n)) + O(m*log(m)) + O(n+m) = O(n*log(n))

One problem is that the passed in arrays are mutated. You have this problem in your current solution too when you add the amounts of a matched item. Another issue is that the new array share pointers to the original inventory items. This could result in someone inadvertently mutating the updated inventory when working with the original arrays passed into updateInventory. If any of that's a concern to your project, then you could create copies of the items to avoid them all:

function updateInventory(arr1, arr2) {
  arr1.map((x) => x.slice(0)).sort(compareInventoryItems); // O(n*log(n))
  arr2.map((x) => x.slice(0)).sort(compareInventoryItems); // O(m*log(m))
  // ...


PHP-Mysqli example secure? I'm just writing my first PHP-Mysqli sample (think about a Wiki 0.0.1) and I would like to ask you if this example is secure or not or if there are any other problems/suggestions you might recommend?

I would like to use prepared statements and not care about sanitizing the input from $_GET, but I don't know if this code is considered secure and OK?

Also, any comments regarding how this functionality (reading a latest revision from a database) should be done is welcome.

require_once 'connect.php';

$id = $_GET['id'];

if ( $stmt = $mysqli->prepare("SELECT text, rev FROM wiki WHERE id = ? ORDER BY rev DESC LIMIT 0,1") ) {

    $stmt->bind_param( "s", $id );
    $stmt->execute();
    $stmt->bind_result( $text, $rev );
    $stmt->fetch();

    echo "rev: " . $rev . ": " . $text;
    echo '<br>';

    $stmt->close();
}

$mysqli->close();

connect.php

$mysqli = new mysqli("localhost", "xxx", "yyy", "zzz");

if (mysqli_connect_errno()) {
    printf("Connect failed: %s\n", mysqli_connect_error());
    exit();
}

Is it safe to keep connect.php at the same folder as the other php files?

Is there any chance someone could read the plain password in connect.php file?

Yes, that looks good.

Minor comment:  Normally an id is an integer.  Should it have been "i" in your bind_param?

Connection Details

First, I am not a security expert.  However, I would recommend storing the connection details in a location that is not served by your web-server.  It should be somewhere in your PHP include_path or accessible from your autoloader.

Having the file in a directory accessible from your web-server could open up the connection details if you had an incorrectly configured web-server.


Making a preselected drop-down selection box in HTML I'm a PHP newbie and I have a question. Is there a better way to do this? A nice solution?

$list = isset($_GET['list']) ? htmlspecialchars($_GET['list']) : "experience";

<select name="type" id="type">
<option value="experience">Experience</option>
<option value="magic"<?=$list == "magic" ? 'selected' : '';?>>Magic Points</option>
<option value="shielding"<?=$list == "shielding" ? 'selected' : '';?>>Shielding</option>
<option value="distance"<?=$list == "distance" ? 'selected' : '';?>>Distance</option>
<option value="melee"<?=$list == "sword" ? 'selected' : '';?>>Melee</option>
<option value="fishing"<?=$list == "fishing" ? 'selected' : '';?>>Fishing</option>
</select>

You can use a lookup table for your values like this:

$values = array("magic" => "Magic points", ...);

and then loop over that to reduce the duplication:

foreach ($values as $key => $value) {
    ...


Binary search for inserting in array I have written a method that uses binary search to insert a value into an array.

It is working, but i would like to get a second opinion to see if i didn't write too much code for it. aka doing same thing twice for example.

The code looks for the right index for insertion and is called by an insert method that uses that index value to insert.

Here is the code:

public class OrdArray {

    final private long[] a;                      // ref to array
    int nElems;                                  // number of dataitems
    int curIn;
    //----------------------------------------------------------------------

    public OrdArray(int max) {                   // constructor
        a = new long[max];                       // create array
        nElems = 0;
    }

    public int binaryInsert(long insertKey) {
        int lowerBound = 0;
        int upperBound = nElems - 1;

        while (true) {
            curIn = (upperBound + lowerBound) / 2;
            if (nElems == 0) {
                return curIn = 0;
            }
            if (lowerBound == curIn) {
                if (a[curIn] > insertKey) {
                    return curIn;
                }
            }
            if (a[curIn] < insertKey) {
                lowerBound = curIn + 1;          // its in the upper
                if (lowerBound > upperBound) {
                    return curIn += 1;
                }
            } else if (lowerBound > upperBound) {
                return curIn;
            } else {
                upperBound = curIn - 1;          // its in the lower
            }
        }
    }

    public void display() {                      // display array contents
        for (int j = 0; j < nElems; j++) {       // for each element,
            System.out.print(a[j] + " ");        // display it
        }
        System.out.println("");
    }

    public void insert(long value) {             // put element into array
        binaryInsert(value);
        int j = curIn;
        int k;
        for (k = nElems; k > j; k--) {           // move bigger ones one up.
            a[k] = a[k - 1];
        }
        a[j] = value;                            // insert value
        nElems++;                                // increment size.
     }
}

public static void main(String[] args) {
    // TODO code application logic here
    int maxSize = 100;                                  // array size
    OrdArray arr;                                       // reference to array

    arr = new OrdArray(maxSize);                        // create array
    arr.insert(77);                                     // insert 10 items
    arr.insert(99);
    arr.insert(44);
    arr.insert(55);
    arr.insert(22);
    arr.insert(88);
    arr.insert(11);
    arr.insert(00);
    arr.insert(66);
    arr.insert(33);

    arr.display();
}

Feedback appreciated.

Here is my version of your code.

You never use "max" so I used it by throwing an exception if you are trying to insert too many elements.
You should make everything that shouldn't be public; private.
In your binaryInsert() you should move your base case to the begining.
Your binaryInsert() is a bit wonky but it works. I think this would do but I haven't checked it. Looks a bit neater and has one unnecessary if removed.

public int binaryInsert(long insertKey) {
   if (nElems == 0)
     return 0;
   int lowerBound = 0;
   int upperBound = nElems - 1;
   int curIn = 0;
   while (true) {
     curIn = (upperBound + lowerBound) / 2;
     if (a[curIn] == insertKey) {
       return curIn;
     } else if (a[curIn] < insertKey) {
       lowerBound = curIn + 1; // its in the upper
       if (lowerBound > upperBound)
         return curIn + 1;
     } else {
       upperBound = curIn - 1; // its in the lower
       if (lowerBound > upperBound)
         return curIn;
     }
   }
 }

Just use methods that return things directly. Don't need to store them in a temporary variable first. Talking about curIn here in your insert function.
If you want to return something (like an object) as a String or print something out. You should override the toString() method as I have done. Then you can just call System.out.println(arr.toString()) whenever you want to print the Object.
The whole point of doing a binary insert would be to quickly find out where to insert an element. Your implementation does this, however your implementation isn't super useful because you have to move each and every element foward by one. A double linked list (as usually taught in C++ classes) is ideal for your implementation of this better version of insertion sort. The java equivalent of a doubly linked list is a LinkedList. Which will give you much better performance as you will not need to move elements forward by one.

.

public class OrdArray {

  final private long[] a; // ref to array
  private int nElems; // number of dataitems
  private final int MAX;

  // ----------------------------------------------------------------------

  public OrdArray(int max) { // constructor
    this.MAX = max;
    a = new long[MAX]; // create array
    nElems = 0;
  }

  private int binaryInsert(long insertKey) {
    if (nElems == 0) {
      return 0;
    }

    int lowerBound = 0;
    int upperBound = nElems - 1;

    while (true) {
      int curIn = (upperBound + lowerBound) / 2;
      if (lowerBound == curIn) {
        if (a[curIn] > insertKey) {
          return curIn;
        }
      }
      if (a[curIn] < insertKey) {
        lowerBound = curIn + 1; // its in the upper
        if (lowerBound > upperBound) {
          return curIn += 1;
        }
      } else if (lowerBound > upperBound) {
        return curIn;
      } else {
        upperBound = curIn - 1; // its in the lower
      }
    }
  }

  @Override
  public String toString() { // display array contents
    StringBuffer sb = new StringBuffer();
    for (int j = 0; j < nElems; j++) { // for each element,
      sb.append(a[j] + " "); // display it
    }
    sb.append(System.lineSeparator());
    return sb.toString();
  }

  public void insert(long value) throws Exception { // put element into array
    if (nElems == MAX)
      throw new Exception("Can not add more elements.");
    int j = binaryInsert(value);
    int k;
    for (k = nElems; k > j; k--) { // move bigger ones one up.
      a[k] = a[k - 1];
    }
    a[j] = value; // insert value
    nElems++; // increment size.
  }
}

I'm sure I didn't get everything you need to improve on but hopefully that is atleast one step forward in the right direction. :)


Creating simpler fluent interface I have the following:

public interface IBehaviour
{
    event EventHandler Completed;
    void Execute();
}

public interface IBehaviourA : IBehaviour
{
    // Some specific stuff here
    object A { get; set; }
}

public interface IBehaviourB : IBehaviour
{
    // Some specific stuff here
    object B {get;set;}
}

public interface IBehaviourQueue
{
    void Run();
    BehaviourQueueItem<IBehaviour> AddBehaviour<T>() where T : IBehaviour;
}

public class BehaviourQueue : Queue<BehaviourQueueItem<IBehaviour>>,  IBehaviourQueue
{
    private IBehaviourFactory factory;
    public BehaviourQueue(IBehaviourFactory factory)
    {
        this.factory = factory;
    }

    public BehaviourQueueItem<IBehaviour> AddBehaviour<T>() where T:IBehaviour
    {
        T behaviour = factory.GetNew<T>();
        var queueItem = new BehaviourQueueItem<IBehaviour>(behaviour);
        Enqueue(queueItem);
        return queueItem;
    }

    public void Run()
    {
        //Run each queue item
    }
}

public class BehaviourQueueItem<T>
{
    public IBehaviour behaviour;

    public BehaviourQueueItem(IBehaviour behaviour)
    {
        this.behaviour = behaviour;
    }

    public void WhenComplete(Func<T, bool> action)
    {
        CompletedAction = action;
    }

    public BehaviourQueueItem<T> ConfigureFor<Z>(Action<Z> dow) where Z : IBehaviour
    {
        dow((Z)behaviour);
        return this;
    }
}

This is how it is used:

var q =new BehaviourQueue(new BehaviourFactory()); // Queue here is IBehaviour
q.AddBehaviour<IBehaviourA>()
 .ConfigureFor<IBehaviourA>(x => x.A = "someValueA")
 .WhenComplete(x => DoStuffWithSomeProperty(((IBehaviourA)x).A));

q.AddBehaviour<IBehaviourB >()
 .ConfigureFor<IBehaviourB >(x => x.B = "someValueB")
 .WhenComplete(x => DoStuffWithSomeProperty(((IBehaviourB)x).B));

What I don't really like is that I have to specify which type of IBehaviour I am referring to every time. I would like to be able to write:

var q =new BehaviourQueue(new BehaviourFactory()); // Queue here is IBehaviour
q.AddBehaviour<IBehaviourA>()
 .Configure(x => x.A = "someValueA")
 .WhenComplete(x => DoStuffWithSomeProperty(x.A));

q.AddBehaviour<IBehaviourB>()
 .Configure(x => x.B = "someValueB")
 .WhenComplete(x => DoStuffWithSomeProperty(x.B));

I know that I have to modify this part:

public BehaviourQueueItem<IBehaviour> AddBehaviour<T>() where T:IBehaviour
{
    T behaviour = factory.GetNew<T>();
    var queueItem = new BehaviourQueueItem<IBehaviour>(behaviour);
    Enqueue(queueItem);
    return queueItem;
}

to something like

public BehaviourQueueItem<T> AddBehaviour<T>() where T:IBehaviour
{
    T behaviour = factory.GetNew<T>();
    var queueItem = new BehaviourQueueItem<T>(behaviour);
    Enqueue(queueItem); // It won't let me know do it because of Covariance/Contravariance thingy
    return queueItem;
}

Do you have any idea what I should write to be able to create a base typed list and add specific items and configuring it fluently?

Keep in mind that I should be able to add any type of IBehaviour to BehaviourQueue so the queue can't be a derived type.

If I understand it right, you would want IBehaviourA associated with property A, and IBehaviourB with property B.

If the above is right, could you try something like:

BehaviourQueueItem<T> AddBehaviour() where T : IBehaviour;

instead of

BehaviourQueueItem<IBehaviour> AddBehaviour<T>() where T : IBehaviour;

By this we can determine the object type at BehaviourQueueItem class level itself.


Merging records from one spreadsheet into another I have created the code below that compare two spreadsheets . It initially compares the range of value from sheet1 ("Status") to sheet2 ("Interface") . Whenever a range matches with the ranges present in other sheet , it does nothing .When the range does not any matches in the other sheet , then the entire record is copied from sheet2 to sheet1 . I have around 1500 rows of data in both sheets with 15 columns . It takes around 30 minutes .
. I look forward to see If I could get any help.

 Option Explicit

Function UpdateNEW() As Long
'   This Sub will do the Following Update
'   Run through all records in NEW
'      if found in Steps ---> Do nothing
'      if not found in Steps ----> Add it to Steps
'

Dim WSO As Worksheet
Dim WSN As Worksheet
Dim MaxRowO As Long, MaxRowN As Long, I As Long, J As Long, lAdd As Long
Dim sJob As String, sOps As String, sFirstAddress As String
Dim cCell As Range
Dim bNotFound As Boolean

'---> Disable Events
With Application
    .EnableEvents = False
    .DisplayAlerts = False
    .ScreenUpdating = False
End With

'---> Set Variables
Set WSO = Sheets("Steps")
Set WSN = Sheets("Interface")
MaxRowO = WSO.Range("A" & WSO.Rows.Count).End(xlUp).Row
MaxRowN = WSN.Range("C" & WSN.Rows.Count).End(xlUp).Row
WSN.Range("P6:P" & MaxRowN).ClearContents

'---> Loop thruough all rows in sheet New
For I = 6 To MaxRowN
    bNotFound = False
    sJob = WSN.Cells(I, "D")
    sOps = WSN.Cells(I, "E")
    Set cCell = WSO.Range("B2:B" & MaxRowO).Find(what:=sJob, LookIn:=xlValues, lookat:=xlWhole, MatchCase:=False)
    If Not cCell Is Nothing Then
        bNotFound = True
        sFirstAddress = cCell.Address
        Do
            '---> Check to See if Ops if found for that Job
            If WSO.Cells(cCell.Row, "C") = sOps Then
                bNotFound = False
                Exit Do
            End If
            Set cCell = WSO.Range("B2:C" & MaxRowO).FindNext(cCell)
        Loop While Not cCell Is Nothing And cCell.Address <> sFirstAddress
    Else
        bNotFound = True
    End If

    '---> Add Record to OLD if Not Found
    If bNotFound Then
        WSN.Range("C" & I & ":O" & I).Copy WSO.Range("A" & MaxRowO + 1)
        'WSN.Range("P" & I) = "Copied to OLD"
        'WSO.Range("N" & MaxRowO + 1) = sJob & " " & sOps & " Copied from New row " & I
        MaxRowO = MaxRowO + 1
        lAdd = lAdd + 1
    End If

Next I

'---> Enable Events
With Application
    .EnableEvents = True
    .DisplayAlerts = True
    .ScreenUpdating = True
End With

UpdateNEW = lAdd

End Function

The unique data could be appended, using a Scripting Dictionary and arrays in less than a second.  Alternatively, you could use an ADO Query to append records from sheet1, not in sheet2.

The easiest method would be to add all the records to the second worksheet then record a macro that:

Convert the range to a table
Select a cell in the range then click Ribbon >  Table Tools > Remove Duplicates

All that is left to do is refactor the Macro to make the code dynamic.

Watch:
 - Excel VBA Introduction Part 25 - Arrays
 - Excel VBA Introduction Part 39 - Dictionaries
 - Excel VBA Introduction Part 32 - ADO (ActiveX Data Objects) SQL Statements (INSERT, UPDATE, DELETE)

Here is how I would rewrite the OP's code using a Scripting Dictionary and arrays.

Function UpdateNEW2() As Long
    Const Delimiter As String = "|"
    Dim dic As Object
    Set dic = CreateObject("Scripting.Dictionary")
    Dim newRows As Range
    Dim vSteps, key
    With Sheets("Steps")
        vSteps = .Range("A2:O2", .Cells(.Rows.Count, 2).End(xlUp)).Value
    End With

    Dim r As Long, c As Long, n As Long
    For r = 1 To UBound(vSteps)
        key = vSteps(r, 3) & Delimiter & vSteps(r, 4)
        Debug.Print key
        If Not dic.Exists(key) Then dic.Add key, 0
    Next

    Dim vInterface, results
    With Sheets("Interface")
        vInterface = .Range("A7:O7", .Cells(.Rows.Count, "C").End(xlUp)).Value
    End With

    ReDim results(1 To UBound(vInterface), 1 To 15)
    For r = 1 To UBound(vInterface)
        key = vInterface(r, 4) & Delimiter & vInterface(r, 5)
        Debug.Print key
        If Not dic.Exists(key) Then
            n = n + 1
            For c = 3 To 15
                results(n, c - 2) = vInterface(r, c)
            Next
        End If
    Next
    If n > 0 Then
        With Sheets("Steps")
            With .Cells(.Rows.Count, 2).End(xlUp).Offset(1)
               .Resize(n, 15).Value = results
            End With
        End With
    End If
    UpdateNEW2 = n
End Function

(Sample Data)


Getting all the UTC "day stamps" between 2 dates I need to get every UTC day stamp in yyyy-MM-dd format (e.g. "2015-07-02") between any two arbitrary Date fields.

My first attempt is this:

public static Set<String> getUTCDayStringsBetween(Date startDt, Date endDt) {
    if (!startDt.before(endDt)) {
        throw new IllegalArgumentException("Start date (" + startDt + ") must be before end date (" + endDt + ")");
    }
    final TimeZone UTC = TimeZone.getTimeZone("UTC");
    final Calendar c = Calendar.getInstance();
    final SimpleDateFormat sdf = new SimpleDateFormat("yyyy-MM-dd");
    sdf.setTimeZone(UTC);
    final Set<String> dayStrings = new LinkedHashSet<>();

    c.setTime(startDt);
    while (true) {
        dayStrings.add(sdf.format(c.getTime()));
        c.add(Calendar.DATE, 1);//add 1 day
        if (c.getTime().after(endDt)) {
            //reached the end of our range. set time to the endDt and get the final day string
            c.setTime(endDt);
            dayStrings.add(sdf.format(c.getTime()));
            break;
        }
    }
    return dayStrings;
}

It seems to work, but I'm wondering if there is a more efficient way to do it (aside from using external time libraries like Joda.).
Also, is there anything I'm glossing over in terms of handling time zones and day-light-savings changes appropriately.

Instead of an infinite while loop + an if to break out, it would be more natural to convert this to a do-while loop.

The comment "//reached the end of our range. set time to the endDt and get the final day string" is pointless, it just says the same thing with words that the code already tells us perfectly clearly.

c is not a great name for a Calendar. How about cal, or even calendar ? Instead of startDt and endDt, I would just also them out.

It's a bit smelly about the interface that the method requires the date parameters in a specific order, but this is not obvious from the name. And if the caller uses the wrong order, the runtime exception is a heavy penalty, crashing the application. It's not great to have rules that cannot be enforced at compile time, but I don't have a good counter proposal for you.


A simple queue implementation in Haskell I'm a member of a discord channel and sometimes people post simple coding exercises for fun. This time the exercise was:

Implement the following operations of a queue using stacks.

push(x) -- Push element x to the back of queue.
pop() -- Removes the element from in front of queue.
peek() -- Get the front element.
empty() -- Return whether the queue is empty

Example:

MyQueue queue = new MyQueue();

queue.push(1);
queue.push(2);
queue.peek(); // returns 1
queue.pop(); // returns 1
queue.empty(); // returns false

I didn't implement it using stacks because I don't know of any that are in the standard Haskell library but
I came up with this Solution:

data Queue a = Empty | Value a (Queue a) deriving (Show, Eq, Read)

push :: a -> Queue a -> Queue a
push x Empty             = Value x Empty
push x (Value a pointer) = Value a (push x pointer)

pop :: Queue a -> Maybe (a, Queue a)
pop Empty             = Nothing
pop (Value a pointer) = Just (a, pointer)

peek :: Queue a -> Maybe a
peek Empty       = Nothing
peek (Value a _) = Just a

empty :: Queue a -> Bool
empty Empty = True
empty _     = False

I'm currently trying to understand Haskell better and was wondering if there was a more idiomatic Solution.

While this is some pretty idiomatic haskell, you definitely didn't implement the challenge the way it was intended to be solved. Either way: here's some minor comments I'd want to put out anyways:

The type-signature of pop implies, that the queue returned can basically be Nothing.
If it can be, what queue did you call this on in the first place? I'd prefer the following signature:

pop :: Queue a -> (Maybe a, Queue a)

The implementation should be pretty straightforward :)
I'd not use a for both the type-variable and the variable in all your functions. Consider explicitly differentiating between types and values:

push x (Value value tail') = Value value (push x tail')
pop (Value value tail') = Just (value, tail')
peek (Value value _) = Just value

This makes the differentiation more clear. Note that I've renamed pointer to tail'. One could also argue for a rename of Value to Element to make the distinction more clear as well.

The naming changes are mostly nitpicks, though.

Now let's examine how to "correctly" solve this challenge. As cole outlines in their comment:

  This queue takes \$O(n)\$ time to enqueue, unlike the common queue implementation which uses a doubly linked list. You can get an amortized \$O(1)\$ time for enqueues if you implement it with two stacks (which is probably what you were getting at with the stack comment). A stack can be implemented using a singly linked list, which is what lists in haskell are. You can either make an abstract data type for a stack and use that in your queue ADT or just use lists
  minor formatting by me

So how would we implement a queue using two stacks, or rather two haskell lists that take the function of stacks? The basic idea of this challenge is to realize that packing two LIFO data structures can make for a FIFO datastructure. Take an "input" stack and an "output" stack. push (or more semantically correctly push_back) will just push a value to the "input" stack.

pop (and peek) are a little more complex. They need to handle the case of an empty output stack. If the output stack is empty, the input stack is moved to the output stack using the basic LIFO operations. Note that this "reverses the order" in the stack, which explains how two LIFO datastructures turn into a FIFO datastructure.

Unfortunately this means something for the interface you've outlined there. We can't assume that peek does not modify the internal state of the Queue anymore. It's simply speaking "not idempotent" anymore, since an empty output stack requires moving the input stack to it.

Now let's look at some type definitions

type Queue a = ([a], [a])

push :: a -> Queue a -> Queue a
pop :: Queue a -> (Maybe a, Queue a)
peek :: Queue a -> (Maybe a, Queue a)
empty :: Queue a -> Bool

Now to implement this:

push x (input, output) = (x:input, output)

pop ([], []) = (Nothing, ([], []))
pop (input, []) = (Just val, ([], new_out))
  where (val, new_out) = fromJust $ uncons $ reverse input
pop (input, (o:os)) = (Just o, (input, os))

peek ([], []) = (Nothing, ([], []))
peek (input, []) = (Just val, ([], new_out))
  where
    new_out = reverse input
    val = head new_out
peek (input, output@(o:_)) = (Just o, (input, output))

empty ([], []) = True
empty _        = False

For whatever it's worth: I liked your implementation better :)


Constructing objects for external libraries using a model in Laravel I want to know if it's more appropriate to use a separate class (such as a factory?) to construct an object. While the code works, I am concerned about bloating of the model and the tight coupling.

To construct the said object I need:

environment variables like api tokens
some of the user's attributes from the model
classes from the library

Here's the relevant snippet within my user model that creates an AdWordsSession object for use in the third-party library:

class User extends Authenticatable {
...
   public function adWordsSession($customer_id = null){
        if(!$this->google_auth_token) {
            return null;
        }

        if( $this->adWordsSesssion &&
            (
                (isset($customer_id) && $this->adWordsSesssion->getClientCustomerId() == $customer_id) ||
                (!isset($customer_id) && $this->adWordsSesssion->getClientCustomerId() == $this->adwords_customer_id)
            )
        ){
            return $this->adWordsSesssion;
        }

        $oAuth2Credential = (new OAuth2TokenBuilder)
            ->withClientId(env('ADWORDS_CLIENT_ID'))
            ->withClientSecret(env('ADWORDS_CLIENT_SECRET'))
            ->withRefreshToken($this->google_auth_token)
            ->build();

        $session = (new AdWordsSessionBuilder())
            ->withDeveloperToken(env('ADWORDS_DEVELOPER_TOKEN'))
            ->withUserAgent(env('ADWORDS_USER_AGENT'))
            ->withClientCustomerId($customer_id ?: $this->adwords_customer_id)
            ->withOAuth2Credential($oAuth2Credential)
            ->build();

        $this->adWordsSesssion = $session;

        return $session;
    }
...
}

I am also interested to know if it applies to the reverse, using a class returned from the library (web service) to construct a model object. Should this be a static method of the model or a separate factory class. Code follows:

class AdWordsCampaign {
    public static function fromAdWordsClass(Campaign $campaign, AdWordsCampaign $adWordsCampaign = null): AdWordsCampaign {

        $self = $adWordsCampaign;

        if(is_null($adWordsCampaign)) {
            $self = new self;
            $self->criterions = $self->defaultCriterions();
            $self->extensions = $self->defaultExtensions();
        }

        if($campaign->getId()) {
            $self->fieldsHelper()->id = $campaign->getId();
            $self->setPublished();
        }

        $self->fieldsHelper()->name = $campaign->getName();
        $self->fieldsHelper()->status = $campaign->getStatus();
        $self->fieldsHelper()->advertisingChannelType = $campaign->getAdvertisingChannelType();
        $self->fieldsHelper()->advertisingChannelSubType = $campaign->getAdvertisingChannelSubType();

        $self->fieldsHelper()->budget = arrayify_object(
            $campaign->getBudget() ?
                $campaign->getBudget() :
                CampaignFields::defaultBudget()
        );

        return $self;
    }
}

I would implement this as a service in Laravel, and use IOC to resolve my dependencies.

I would make a service, called AdWordsCampaign. This would resolve its dependencies in constructor, namely OAuth2TokenBuilder and AdWordsSessionBuilder, although these two classes are good candidates for services themselves (a factory pattern is most of times is a sign you should abstract creation of class into a service provider).

Then I would implement a method in this service, adWordsSession which takes two parameters: a User instance and $customer_id. Then it has everything it needs to to its job.

I would also implement the reverse case using a service. Semantics are the same.

As for static method, I would avoid them. If you like them, use a Facade. They enable you to use a service anywhere you want.

I strongly suggest to read Laravel docs about services and providers and IOC and Facades. It will give you enough insight about how they work, and where to use them.

As a rule of thumb, whenever you see yourself using new * in your codes, it seems your could use a service/provider and inject it.


Add two signed integers, given in stdin(space separated) in i386 ASM Linux ABI .global _start
_start:
    mov $0x1000,%edx
    mov %edx,%eax
    neg %eax
    and %eax,%esp
    mov %esp,%ecx
    xor %ebx,%ebx
    mov $3,%eax
    int $0x80
    add %edx,%esp
    mov %ecx,%edi
    mov $label00,%ebp
    jmp parse_eax_load_constants
label00:
    push %eax
    inc %edi
    mov $label01,%ebp
    jmp parse_eax
label01:
    mov %edi,%ebp
    mov %esi,(%edi)
    dec %edi
    pop %edx
    add %edx,%eax # finally
print_eax: # %edi: where to print %eax
    xor %edx,%edx
    div %esi
    or %cl,%dl
    mov %dl,(%edi)
    dec %edi
    test %eax,%eax
    jnz print_eax

    mov %ebp,%edx
    sub %edi,%edx
    inc %edi
    mov %edi,%ecx
    xor %ebx,%ebx
    inc %ebx
    mov $4,%eax
    int $0x80
    mov %ebx,%eax
    int $0x80

parse_eax_load_constants:
    mov $10,%esi
    mov $0x30,%ecx
parse_eax: # %edi: the string to parse
    xor %eax,%eax
    movb (%edi),%al
    mov %edi,%ebx
    inc %ebx
    cmpb $0x2d,%al
    cmovz %ebx,%edi
    setz %bl
    push %ebx
    mov (%edi),%al
    xor %ebx,%ebx
    xor %cl,%al
parse_eax_loop:
    inc %edi
    movb (%edi),%bl
    xor %cl,%bl
    cmp %esi,%ebx
    jae parse_eax_end
    mul %esi
    add %ebx,%eax
    jmp parse_eax_loop
parse_eax_end:
    pop %ebx
    mov %eax,%edx
    neg %edx
    test %bl,%bl
    cmovnz %edx,%eax
    jmp *%ebp

You are asking for a code review, and my first bit of feedback is: COMMENTS.

Please, reading raw assembler is a challenge.  Start by a comment block at the top that describes the purpose of the code, then consider line-by-line comments, something like this: https://codereview.stackexchange.com/a/156947/110050


Simple array-based Stack class in Java I have created a simple array-based Stack class with some methods like the actual Stack in Java.

I am testing this for mistakes, but since I am learning Java, my tests may not be as comprehensive as they should.

import java.util.*;

public class SJUStack<E> {
    // Data Fields
    private E[] theData;
    private int topOfStack = -1;
    private static final int INITIAL_CAPACITY = 10;
    private int size = 0;
    private int capacity = 0;

    // Constructors
    public SJUStack(int initCapacity) {
        capacity = initCapacity;
        theData = (E[]) new Object[capacity];
    }

    public SJUStack() {
        this(INITIAL_CAPACITY);
    }

    // Methods
    public E push(E e) {
        if(size == capacity) {
            reallocate();
        }
        theData[size] = e;
        size++;
        topOfStack++;

        return e;
    } // End push(E e) method

    public E peek() {
        if(empty()) {
            throw new EmptyStackException();
        }
        return theData[topOfStack];
    } // End peek() method

    public E pop() {
        E result = peek();
        theData[topOfStack] = null;
        size--;
        topOfStack--;
        if(size <= (capacity/4) && capacity >= INITIAL_CAPACITY) {
            shrink();
        }
        return result;
    } // End pop() method

    public boolean empty() {
        return size == 0;
    } // End empty() method

    private void reallocate() {
        capacity *= 2;
        theData = Arrays.copyOf(theData, capacity);
    } // End reallocate() method

    private void shrink() {
        capacity /= 2;
        theData = Arrays.copyOf(theData, capacity);
    } // End shrink() method

    public String toString() {
        return Arrays.toString(theData);
    } // End toString() method

    public int size() {
        return size;
    } // End size() method
}

I suggest to use following method name: isEmpty() instead of empty()
If your Java version > 1.5 you should use @Override for your toString() method.
Do you really need the the article in class field name like theData?
You don't need both size and topOfStack variables. theData.size is enough.


Better Alternative For Storing Multiple Booleans? I have a class, which stores an enum to track if my object is modified, newly created, or no change. If the object is modified, I have additional booleans for each field that is modified.

For instance,

public enum status
{
    NoChange,
    Created,
    Modified
}

private bool? name;
private bool? address;
...
private bool? numberOfDonkeysPurchased;

So the idea is that if the status is modified, then the nullable bools will be either true or false, depending on if these fields are changed.

If the status is not modified, then the nullable bools will be null.

The problem with this is that I have a couple of these fields that I want to check (say 10). Is it valid to create a nullable bool for each field that I am checking, knowing that I maybe be storing a lot of nulls? Or is that not a concern?

Is there a better way to store this?

Thanks!

I would suggest that you use a Dictionary, with the property as the key and the bool as the value. e.g.

Dictionary<string, bool?> myProperties = new Dictionary<string, bool?>();

Use reflection to get all the properties from your object. Then you dont have any work when properties gets added/removed.

You can initialise your dictionary with reflection like this:

foreach (PropertyInfo propertyInfo in myObject.GetType().GetProperties()) {
    myProperties.Add(propertyInfo.Name, false);
}

Or you can even only add those that are not null.


Repository implementation I have a repository called PostsRepository:

public class PostsRepository : IPostsRepository
{
    private readonly DatabaseContext context = new DatabaseContext();

    public IEnumerable<Post> All()
    {
        return
            context.Posts
                   .OrderBy(post => post.PublishedAt);
    }

    public IEnumerable<Post> AllPublishedPosts()
    {
        return
            context.Posts
                   .OrderBy(post => post.PublishedAt)
                   .Where(post => !post.Draft);
    }

    public Post Find(string slug)
    {
        return context.Posts.Find(slug);
    }

    public void Create(Post post)
    {
        post.Slug = SlugConverter.Convert(post.Slug);
        post.Summary = Summarize(post.Content);
        post.PublishedAt = DateTime.Now;
        AttachTags(post);

        if (context.Posts.Find(post.Slug) != null)
        {
            throw new Exception("tag already exists. choose another.");
        }

        context.Posts.Add(post);
        context.SaveChanges();
    }

    public void Update(Post post)
    {
        post.Slug = SlugConverter.Convert(post.Slug);
        post.Summary = Summarize(post.Content);
        AttachTags(post);

        if (context.Posts.Find(post.Slug) != null)
        {
            throw new Exception("tag already exists. choose another.");
        }

        context.Posts.Add(post);
        context.SaveChanges();
    }

    public void Delete(Post post)
    {
        context.Posts.Remove(post);
        context.SaveChanges();
    }

    private void AttachTags(Post post)
    {
        foreach (var tag in post.Tags)
        {
            if (context.Tags.Any(x => x.Name == tag.Name))
            {
                context.Tags.Attach(tag);
            }
        }
    }

    private static string Summarize(string content)
    {
        // contrived.
        return content;
    }
}

I am worried that I might have wound up with a design that is not very testable as it is not apparent to me how to test this code.

I am going to ask another question on SO as to how to unit test this class soon but before I peruse this implementation I would like to ask that you please review my repository implementation.

Particular areas of concern:

Testability
I have read countless opinions about what a repository should do. Are there any pragmatic reasons why my implementation might be bad?
The PostsRepository needs to access the Tags database set. Am I allowing this in the correct way? Know that I plan to implement a TagsRepository in the future.
I throw an exception when the slug (which must be unique) is occupied. Should I return a bool to indicate failure instead? Would this not violate the command-query segregation principle?
I am aware that the Update method is hard to reason about and I am working on that. It is for this reason that my code does not currently adhere to DRY.

Testability

Your repository implements an interface which will allow it to get stubbed out easily, so that's a very testable thing. However if you want to also (unit-test) your repositories themselves then you are stuck because you have a hardcoded dependency on DatabaseContext.

You should move that up one layer by abstracting it out as well and providing a custom DbContext for testing purposes or by mocking it out. More information on that here.

  I have read countless opinions about what a repository should do. Are there any pragmatic reasons why my implementation might be bad?

One thing that comes to mind is the long-lived DbContext object. Some say it isn't an exact necessity, others say you definitely should avoid it but generally accepted is the notion that you should dispose of your DbContext. A unit of work corresponds to one method inside your repository to wrap a using statement around it.

Note that this would interfere with testing the repository itself so it's worth considering using integration-tests to test your repository in particular.

  The PostsRepository needs to access the Tags database set. Am I allowing this in the correct way? Know that I plan to implement a TagsRepository in the future.

I am not familiar enough with EF to answer that.

  I throw an exception when the slug (which must be unique) is occupied. Should I return a bool to indicate failure instead? Would this not violate the command-query segregation principle?

It does not violate it because it is just the result of your command, it is not the result of a query. It holds exactly the same value as an exception.

There is a more interesting way though: create a "result"-object. This can be as simple as

class CallResult {
    bool Success { get; set; }
    string Message { get; set; }
}

Which will be more descriptive than bool since you can also send a message along. Whether you choose this or an exception depends on your own preference: using an exception will force you to have a try-catch around it somewhere which is rather ugly but then again it will prevent you from accidentally leaving out validation on the return type (either a try-catch or if(result.Success)) which is not enforced with a custom type.

  I am aware that the Edit method is hard to reason about and I am working on that. It is for this reason that my code does not currently adhere to DRY.

Since I don't see an Edit method I'll assume you haven't written it yet. DRY is nice and all but for some niche situations (CRUD actions, tests, ..) I would argue that readability is more important than having a few lines similar to eachother.

I suppose you could create a situation like this, but that depends on how different each action is.

void Process(Post post) {
    post.Slug = SlugConverter.Convert(post.Slug);

    if (context.Posts.Find(post.Slug) != null)
    {
        throw new Exception("tag already exists. choose another.");
    }

    post.Summary = Summarize(post.Content);
    AttachTags(post);
}

void Create(Post post) {
    Process(post);
    post.PublishedAt = DateTime.Now;
    context.Posts.Add(post);
    context.SaveChanges();
}

void Update(Post post) {
    Process(post);
    context.Posts.Add(post);
    context.SaveChanges();
}

Note how I performed the validation before doing the other work.

A method should be written as [action][context]. AllPublishedPosts makes me think it's a property, not a method. I would change this to GetAllPublishedPosts. I'm not speaking out about All since that's a special situation, I suppose.


Converting JPG images to PNG I had a task to convert some JPG images into PNG format using Python. I successfully did it but I was wondering if somehow I could make my code better than my current one.

Code -

#Python Code

import os
from PIL import Image

path1 = sys.argv[1]
path2 = sys.argv[2]

if not os.path.exists(path2):
    os.makedirs(path2)

for filename in os.listdir(path1):
    if filename.endswith(".jpg"):
        # print(filename.split(".")[0])
        im = Image.open(f"{path1}{filename}")
        # print(Image.open(f"{path1}{filename}"))
        im.save(f"{path2}/{filename.split('.')[0]}.png", "png")
        print("Lets go")

Not much to improve upon, just some minor housekeeping.

Help the end user!

If I was using this script, and forgot to put one or both of the filepaths when calling the script, there would be this error: IndexError: list index out of range. This error wouldn't help the user in a significant way. Consider this:

try:
    path1 = sys.argv[1]
    path2 = sys.argv[2]
except IndexError:
    raise Exception("Usage: python3 jpg_to_png.py <path to input folder> <path to output folder>")

This gives the user a detailed message on how the use your script.

Remove commented code.

Code you don't need anymore adds useless clutter, and can make your code harder to read.


Simplifying the array of objects with comma separated values Looking a way to reduce the below code to get comma separated values from the list of arrays
By using ECMA or plain old vanilla JS. How should we achieve this?
Actual Array Data:
[{
    ansBook: "046H",
    ansPersonId: "2044000102",
    bookNbr: "046H",
    dns: "0",
    examCode: "20717010",
    examShortTitle: "QA-ATAG-Director of Applications",
    nar: "0",
    personId: "2044000102",
    pull: "0",
    wn: "0",
    corrCd: 1
  },
  {
    ansBook: "046I",
    ansPersonId: "2044000102",
    bookNbr: "046I",
    dns: "0",
    examCode: "20721010",
    examShortTitle: "QA-ATAG-Plans Examinier",
    nar: "0",
    personId: "2044000102",
    pull: "0",
    wn: "0",
    corrCd: 1
  }
];

Expected Result:
{
  "PersonIds": "2044000102, 2044000102",
  "BookLetCodes": "046H, 046I",
  "ExamNumbers": "20717010, 20721010",
  "ExamTitles": "QA-ATAG-Director of Applications, QA-ATAG-Plans Examinier"
}

As I am getting the expected result. But I need to reduce and improvise the performance performed on it..

let seriesDataSet = [{
    ansBook: "046H",
    ansPersonId: "2044000102",
    bookNbr: "046H",
    dns: "0",
    examCode: "20717010",
    examShortTitle: "QA-ATAG-Director of Applications",
    nar: "0",
    personId: "2044000102",
    pull: "0",
    wn: "0",
    corrCd: 1
  },
  {
    ansBook: "046I",
    ansPersonId: "2044000102",
    bookNbr: "046I",
    dns: "0",
    examCode: "20721010",
    examShortTitle: "QA-ATAG-Plans Examinier",
    nar: "0",
    personId: "2044000102",
    pull: "0",
    wn: "0",
    corrCd: 1
  }
];

let auditingInfo = {};
auditingInfo = (seriesDataSet).filter(w => (w.dns == '0' && w.pull == '0') && w.corrCd == 1)
  .map(e => ({
    PersonIds: e.personId,
    BookLetCodes: e.ansBook,
    ExamNumbers: e.examCode,
    ExamTitles: e.examShortTitle
  }))
console.log(auditingInfo)

let auditingDetails = {
  PersonIds: auditingInfo.map(x => x.PersonIds).join(', '),
  BookLetCodes: auditingInfo.map(x => x.BookLetCodes).join(', '),
  ExamNumbers: auditingInfo.map(x => x.ExamNumbers).join(', '),
  ExamTitles: auditingInfo.map(x => x.ExamTitles).join(', ')
}
console.log('______________________________________________')
console.log('Expected Result:')
console.log(auditingDetails)
.as-console-wrapper {
  max-height: 100% !important;
  top: 0;
}

OR
let ScoringApprove: Array<number> = [];
ScoringApprove = new List<SeriesReportsDataset>(this.tempSeriesDataSet)
                .Where(w => (w.dns == '0' && w.pull == '0') && w.corrCd == 1)
                .Select(s => s.scannedResponseID).ToArray();

Honestly, structure-wise, your code seems pretty good. I've just got a handful of minor code-cleanup suggestions:

Use === instead of ==, == does some magical type coercion, and can create unexpected results.
You've got some unenesary parentheses here: auditingInfo = (seriesDataSet).filter(w => (w.dns === '0' && w.pull === '0') && w.corrCd === 1) - you can take out the parentheses around (seriesDataSet) and (w.dns === '0' && w.pull === '0')
What's the reason for initializing auditingInfo to {}, then immediately setting it to a different value? Just do it in one go. For example:

// instead of
let auditingInfo = {};
auditingInfo = ...
// just do
let auditingInfo = ...;

You're using a .map() to extract and rename values from your data set, then the next thing you do is some .map()s again to extract out specific values. Why not take out the first .map() entirely?
After applying these suggestions, you'll be left with this:
let auditingInfo = seriesDataSet.filter(w => w.dns === "0" && w.pull === "0" && w.corrCd === 1);

let auditingDetails = {
  PersonIds: auditingInfo.map(x => x.personId).join(", "),
  BookLetCodes: auditingInfo.map(x => x.ansBook).join(", "),
  ExamNumbers: auditingInfo.map(x => x.examCode).join(", "),
  ExamTitles: auditingInfo.map(x => x.examShortTitle).join(", ")
};

Finally, you had also asked about improving performance. You're not doing anything slow algorithm-wise, so there's not a whole lot of performance gains you can squeeze out of this. I wouldn't bother unless you really need it, in which case, you can try out different micro-optimizations and run performance tests on your target platforms to see which optimizations help the most. (some optimizations could speed things up on one platform and slow them down on another). Any of these types of optimizations will reduce the readability of your code, so only apply them if they make a significant enough impact.
A couple of optimization ideas you can try out that may help out, or may make things worse:

Use a single for loop instead of using .filter() and multiple .map() calls.
Instead of array.join(), gradually build the string.
If you have a large dataset, see if you can gradually consume a stream of data, instead of loading the whole thing into memory, all at once.


Writing a checkbox with jQuery If a checkbox is clicked, it will be added to the total, if not, it will be deducted or ignored. (The total number of checkboxes I have in real is 25.)

Is there a way to write the code below in a shorter version?

Function

function tblcheckboxes(){
    var a, b, c, d, e, f, g, h, i, j, k, l, m, n, o, p, q, r, s, t, u, v, total;
    if ($('#tcbx1').is(":checked")) {
         a = parseFloat($("#tcbx1").val(), 10);
    }
    if ($('#tcbx2').is(":checked")) {
         b = parseFloat($("#tcbx2").val(), 10);
    }
    if ($('#tcbx3').is(":checked")) {
         c = parseFloat($("#tcbx3").val(), 10);
    }
    if ($('#tcbx4').is(":checked")) {
         d = parseFloat($("#tcbx4").val(), 10);
    }
    if ($('#tcbx5').is(":checked")) {
         e = parseFloat($("#tcbx5").val(), 10);
    }

jQuery

$(document).ready(function(){
    $('#tcbx1').click(function(){
            tblcheckboxes();
    });
    $('#tcbx2').click(function(){
            tblcheckboxes();
    });
    $('#tcbx3').click(function(){
            tblcheckboxes();
    });
    $('#tcbx4').click(function(){
            tblcheckboxes();
    });
    $('#tcbx5').click(function(){
            tblcheckboxes();
    });
   });

Why not do something like this:

$(document).ready(function() {
     $('.containerClass-filter-expression-that-selects-all-your-chks').click(function() {
       recalculate();
     });
});

function recalculate(){
   $('.some-expression-that-selects-all-your-checkboxes').each(function() {
       if($(this).is('checked')) {
           //add

       } else {
           //substract from total
       }

   });
};


Implementation of a Markov Chain I read about how markov-chains were handy at creating text-generators and wanted to give it a try in python.

I'm not sure if this is the proper way to make a markov-chain.  I've left comments in the code.  Any feedback would be appreciated.

import random

def Markov(text_file):

    with open(text_file) as f:    # provide a text-file to parse
        data = f.read()

    data = [i for i in data.split(' ') if i != '']     # create a list of all words
    data = [i.lower() for i in data if i.isalpha()]    # i've been removing punctuation

    markov = {i:[] for i in data}    # i create a dict with the words as keys and empty lists as values

    pos = 0
    while pos < len(data) - 1:    # add a word to the word-key's list if it immediately follows that word
        markov[data[pos]].append(data[pos+1])
        pos += 1

   new = {k:v for k,v in zip(range(len(markov)), [i for i in markov])}    # create another dict for the seed to match up with

    length_sentence = random.randint(15, 20)    # create a random length for a sentence stopping point

    seed = random.randint(0, len(new) - 1)    # randomly pick a starting point

    sentence_data = [new[start_index]]     # use that word as the first word and starting point
    current_word = new[start_index]

    while len(sentence_data) < length_sentence:
        next_index = random.randint(0, len(markov[current_word]) - 1)    # randomly pick a word from the last words list.
        next_word = markov[current_word][next_index]
        sentence_data.append(next_word)
        current_word = next_word

   return ' '.join([i for i in sentence_data])

import random

def Markov(text_file):

Python convention is to name function lowercase_with_underscores. I'd also probably have this function take a string as input rather then a filename. That way this function doesn't make assumptions about where the data is coming from

    with open(text_file) as f:    # provide a text-file to parse
        data = f.read()

data is a bit too generic. I'd call it text.

    data = [i for i in data.split(' ') if i != '']     # create a list of all words
    data = [i.lower() for i in data if i.isalpha()]    # i've been removing punctuation

Since ''.isalpha() == False, you could easily combine these two lines

    markov = {i:[] for i in data}    # i create a dict with the words as keys and empty lists as values

    pos = 0
    while pos < len(data) - 1:    # add a word to the word-key's list if it immediately follows that word
        markov[data[pos]].append(data[pos+1])
        pos += 1

Whenever possible, avoid iterating over indexes. In this case I'd use

   for before, after in zip(data, data[1:]):
       markov[before] += after

I think that's much clearer.

   new = {k:v for k,v in zip(range(len(markov)), [i for i in markov])}    # create another dict for the seed to match up with

[i for i in markov] can be written list(markov) and it produces a copy of the markov list. But there is no reason to making a copy here, so just pass markov directly.

zip(range(len(x)), x) can be written as enumerate(x)

{k:v for k,v in x} is the same as dict(x)

So that whole line can be written as

  new = dict(enumerate(markov))

But that's a strange construct to build. Since you are indexing with numbers, it'd make more sense to have a list. An equivalent list would be

 new = markov.keys()

Which gives you a list of the keys

    length_sentence = random.randint(15, 20)    # create a random length for a sentence stopping point

    seed = random.randint(0, len(new) - 1)    # randomly pick a starting point

Python has a function random.randrange such that random.randrange(x) = random.randint(0, x -1) It good to use that when selecting from a range of indexes like this

    sentence_data = [new[start_index]]     # use that word as the first word and starting point
    current_word = new[start_index]

To select a random item from a list, use random.choice, so in this case I'd use

   current_word = random.choice(markov.keys())

    while len(sentence_data) < length_sentence:

Since you know how many iterations you'll need I'd use a for loop here.

        next_index = random.randint(0, len(markov[current_word]) - 1)    # randomly pick a word from the last words list.
        next_word = markov[current_word][next_index]

Instead do next_word = random.choice(markov[current_word])

        sentence_data.append(next_word)
        current_word = next_word

   return ' '.join([i for i in sentence_data])

Again, no reason to be doing this i for i dance. Just use ' '.join(sentence_data)


Simple encryption algorithm I originally made this as a way to use character strings as a base-256 number, but then realized that I could turn it into a method for encryption instead. The only thing is that since I'm using a basic addition algorithm for the encryption process, I'm iterating through a string by one character at a time (which seems kind of slow). Are there any other ways to improve the performance of this process or am I pretty much stuck with simple iteration?

Here's the encryption code:

void encodeStr(string encryptKey, string& inputToEncrypt) {
    assert(minVal < maxVal);
    if (inputToEncrypt.length() == 0) return;

    //preliminary remainder check initialization
    int remainder = minVal; //zero value
    int keyNumCount = 0;
    int keyCount = 0;

    //variables used to determine when to stop counting numbers
    size_t keyPos = 0;
    size_t count = 0;
    size_t countEnd = inputToEncrypt.length();

    //begin numerical addition. stop once the end of both strings is reached
    while (count < countEnd) {
        //convert current characters to unsigned chars
        keyNumCount = encryptKey[keyPos];
        if (keyNumCount < minVal) keyNumCount += minVal;

        keyCount = (count < inputToEncrypt.length()) ? inputToEncrypt[count] : 0;
        if (keyCount < minVal) keyCount += minVal;

        //take advantage of signed-char overflow to calculate current value
        remainder += keyNumCount + keyCount;
        inputToEncrypt[count] = remainder;

        //check if there is a remainder to be tabulated
        remainder = (remainder > maxVal) ? 1 : 0;
        ++count;
        ++keyPos;
        if (keyPos == encryptKey.size()) keyPos = 0;

        //add 1 to the count if there is a remainder at the end of keyNum
        if (count == countEnd && remainder != 0) {
            inputToEncrypt.push_back('\0');
            ++countEnd; //variables used to avoid integer overflow
        }
    }
}

And the decryption function:

void decodeStr(string decryptKey, string& inputToDecrypt) {
    assert(minVal < maxVal);
    if (inputToDecrypt.length() == 0) return;

    //preliminary remainder check initialization
    int remainder = minVal;
    int keyNumCount = 0;
    int keyCount = 0;

    //variables used to determine when to stop counting numbers
    size_t keyPos = 0;
    size_t count = 0;
    size_t countEnd = inputToDecrypt.length();

    //make sure the program exits before the number necomes negative
    while (count < countEnd) {
        //convert current characters to unsigned chars
        keyNumCount = decryptKey[keyPos];
        if (keyNumCount < minVal) keyNumCount += minVal;

        keyCount = inputToDecrypt[count];
        if (keyCount < minVal) keyCount += minVal;

        //take advantage of signed-char overflow to calculate current value
        remainder += keyCount - keyNumCount;
        inputToDecrypt[count] = remainder;

        //check if there is a remainder to be tabulated
        remainder = (remainder > maxVal) ? 1 : 0;
        ++count;
        ++keyPos;
        if (keyPos == decryptKey.size()) keyPos = 0;
    }
}

Since you key is const it would be nice to mark it as such (and avoid accidents of changing your key):

void encodeStr(string const& encryptKey, string& inputToEncrypt) {
//                    ^^^^^

To save the cost of the copy I would also make it a reference.

No idea what these are:

assert(minVal < maxVal);

The extra set of variables seems like a good idea but actually makes the code harder to read as you need to understand what the variable represents:

size_t countEnd = inputToEncrypt.length();
while (count < countEnd) {

// Simpler to

while (count < inputToEncrypt.length()) {

When is this condition every going to fail and give you 0?

keyCount = (count < inputToEncrypt.length()) ? inputToEncrypt[count] : 0;

If it was going to fail then you should have the same precautions on this:

inputToEncrypt[count] = remainder;

This is also bound to result in truncation of reminder. As inputToEncrypt as a string and thus each position is a char while remainder is the sum of 2 integers both greater than minval.

inputToEncrypt[count] = remainder;


Monitor a web page for when a desired price is met I have a code that keeps checking a webpage with a price on it and when the price meets a number I set it to, the thing is purchased. I am looking for ways to increase the speed of it.

I have added multiprocessing and its performance may have improved, but I don't know if its the best-case scenario for this code and there probably are better methods for speed and efficiency.

from multiprocessing.dummy import Pool
from functools import partial

session = requests.session()
session.cookies["cookie"] = ""

login = session.get("https://www.example.com")
if login.status_code == 200:
    print("logged in")
else:
    raise ValueError("Invalid Cookie")

crsf_token = ""

def token():
    while True:
        global crsf_token
        crsf_token = re.search(r"<script>XsrfToken.setToken\('(.*?)'\);</script>", session.get('https://www.example.com').text).group(1)
        time.sleep(5)

def _cthread():
    with Pool() as pool:
        while True:
            try:
                req = session.get(f"https://www.example.com")
                if req.status_code == 429:
                    time.sleep(5)
                    continue

                allposts = [f'https://www.example.com&Price={i["Price"]}'
                        for i in req.json()["data"]["Sellers"] if i["Price"] <= 10]
                if allposts:
                    pool.map(partial(session.post, headers={"X-CSRF-TOKEN": crsf_token}), allposts)
            except requests.urllib3.exceptions.ConnectTimeoutError as E:
                pass

threading.Thread(target=_cthread).start()
threading.Thread(target=token).start()

Constant reuse

Store your base URL, "https://www.example.com", in a constant for reuse.

HTTP codes

Some status codes, such as 200, are common and obvious, while others such as 429 are less so. Fundamentally these are all magic numbers, and requests.codes contains symbols - including too_many_requests.

URL construction

           allposts = [f'https://www.example.com&Price={i["Price"]}'
                    for i in req.json()["data"]["Sellers"] if i["Price"] <= 10]

Firstly - you don't need to be adding the URL in here. You can move the query parameter to the params dict to be passed to session.post. Then the URL can be passed to session.post directly without any baked-in query parameters.

Variable case

ConnectTimeoutError as E:

Don't capitalize E since it's a variable. Past that, you shouldn't even write as E, since you aren't using the exception.

Import long-form symbols

requests.urllib3.exceptions.ConnectTimeoutError

would benefit from an import of this class so that you can use it without package qualification.


Calculating Standard Deviation using SQL I have been reading a bit about statistics to improve the way I understand data. This is my attempt at calculating Standard Variance using data tables. I solved this simple question:

This is the code I used (I used SEDE to execute it):

CREATE TABLE #Numbers
(
  Number DECIMAL(10,4),
  Variance DECIMAL(10,4)
);
GO
INSERT INTO #Numbers (Number) VALUES
    (23),
    (37),
    (45),
    (49),
    (56),
    (63),
    (63),
    (70),
    (72),
    (82);
GO

DECLARE @Mean DECIMAL(10,4);
SET @Mean = (SELECT SUM (Number) / COUNT(Number) FROM #Numbers);
UPDATE #Numbers
SET Variance = POWER( (@Mean - Number), 2);
DECLARE @StdVariance DECIMAL(10,4);
SET @StdVariance = (SELECT SQRT( SUM(Variance) / COUNT(Number) ) FROM #Numbers);
SELECT
  @Mean AS [Mean],
  @StdVariance AS [Standard Variance];

Output:

(10 row(s) affected)

Mean    Standard Variance
------- -----------------
56.0000 16.8701

(10 row(s) affected)
(1 row(s) affected)

I'm less familiar with SQL Server than other RDBMS, so I want to make sure my use of temp tables and variables is done the "Transact-SQL Way". Is there a cleaner, smarter way to do this?

Assuming you want to reinvent the wheel…

You really shouldn't be using a data table as scratch space for calculations, even if it is a temporary table.  You should also not need to rely on variables either — that is not idiomatic SQL.  A Common Table Expression would solve both problems.

CREATE TABLE #Numbers
(
  Number DECIMAL(10,4)
);

INSERT …;

WITH Mean AS (
    SELECT SUM(Number) / COUNT(Number) AS Mean
        FROM #Numbers
), Deviation AS (
    SELECT Mean, POWER(Number - Mean, 2) AS Error
        FROM #Numbers CROSS JOIN Mean
)
SELECT Mean, SQRT(SUM(Error) / COUNT(Error)) AS [Standard Deviation]
    FROM Deviation
    GROUP BY Mean;


F#zzBuzz: Learning the easy way So I made a quick programme in F# to do FizzBuzz, I'm trying to learn more languages, so I figured I'd do something functional for a change. (Haskell is also on the list.)

FizzBuzz is pretty self-explanatory: every third value print Fizz, every fifth print Buzz (possible both if it's a multiple of 3 and 5).

[<EntryPoint>]
let main argv =
    let fizzBuzzSequence = seq {
        for value in 1..100 do
            let valueMod3 = value % 3
            let valueMod5 = value % 5

            if valueMod3 = 0 && valueMod5 = 0 then yield "FizzBuzz"
            else if valueMod3 = 0 && valueMod5 <> 0 then yield "Fizz"
            else if valueMod3 <> 0 && valueMod5 = 0 then yield "Buzz"
            else yield value.ToString()
    }
    for value in fizzBuzzSequence do printfn "%s" value
    0

First off, I don't see a real need for these two variables:

let valueMod3 = value % 3
let valueMod5 = value % 5

To me, they just seem a little unnecessary. In fact, writing out the expression value % 3 or value % 5 takes the exact same amount of keystrokes as writing valueMod3 or valueMod5.

Next, I'd highly recommend that you get into the habit of using Match Expressions rather than plain old if statements, as it's more functional, and will give you more benefits, like Pattern matching, over if statements. This means that this small chunk of if statements:

if valueMod3 = 0 && valueMod5 = 0 then yield "FizzBuzz"
else if valueMod3 = 0 && valueMod5 <> 0 then yield "Fizz"
else if valueMod3 <> 0 && valueMod5 = 0 then yield "Buzz"
else yield value.ToString()

Would become a Match Expression, like this:

match n with
| n when n % 3 = 0 && n % 5 = 0 -> yield "FizzBuzz"
| n when n % 3 = 0 -> yield "Fizz"
| n when n % 5 = 0 -> yield "Buzz"
| _ -> yield n.ToString()

After all these improvements to your code, I ended up with the following, working code:

let fizzbuzzSequence = seq {
    for n in 1 .. 100 do
        match n with
        | n when n % 3 = 0 && n % 5 = 0 -> yield "FizzBuzz"
        | n when n % 3 = 0 -> yield "Fizz"
        | n when n % 5 = 0 -> yield "Buzz"
        | _ -> yield n.ToString()
}

[<EntryPoint>]
let main argv =
    for item in fizzbuzzSequence do
        System.Console.WriteLine(item)

    0


DTO interfaces for implementing web service integration I am trying to define some interfaces to implement some web service integration middleware.
The SOA pattern that fits this system is probably the Multi-Channel Endpoint.

I expect a lot of mapping of data formats happening in this integration so I am trying to come up with some interfaces to implement this.

For each web service client I would like to provide a Service Facade that will map the input data to a common data structure (DTO) that will then be interpreted by the middleware which will orchestrate the services that  connect to the legacy applications.

My attempt at coding the interfaces goes like this:

// Protocol for mapping POJOs to some Map-like data structure
public interface Dto {

    // Map from POJO fields to DtoData
    public DtoData toData();

    // Map from DtoData to POJO
    public void fromData(DtoData data);

}

public class DtoData {
    Map<String, Object> data;

    public Map<String, Object> getData() {
        return data;
    }

    public void setData(Map<String, Object> data) {
        this.data = data;
    }
}

// This class provides default mapping methods from POJOs to Maps.
// Uses Jackson and Dozer for mapping back and forth between objects.
// TODO use DI to create mapper objects.
public abstract class AbstractDto implements Dto {

    @Override
    public DtoData toData() {
        ObjectMapper mapper = new ObjectMapper();
        DtoData map = mapper.convertValue(this, DtoData.class);
        return map;
    }

    @Override
    public void fromData(DtoData data) {
        DozerBeanMapper mapper = new DozerBeanMapper();
        mapper.map(data, this);
    }

}

public class DtoImpl extends AbstractDto implements Dto {

    private DtoData dtoData;

    public DtoData getDtoData() {
        return dtoData;
    }

    public void setDtoData(DtoData dtoData) {
        this.dtoData = dtoData;
    }

}

Any suggestions on how to improve this and pitfalls that I can expect?

I am not sure I understand what's the reason of your Dto and of its AbstractDto and DtoImpl implementations. Why does it need to hold a dtoData instance?
I think that you should separate the definition of Dtos, the definition of DtoData and the definition of a Mapper that translates between the two representation.

I think you should't really have a Dto interface or leave it to be just a marker interface, but I don't really like them as they are usually an anti pattern.

I don't like that you pass directly a Map to your DtoData. What about something implementing this (very Map-like) interface?

interface DtoData
{
    void setValue(String key, object value);
    void getValue(String key, object value);
    Set<String> getKeys();
}

Finally all the conversions should be handled by a Mapper, implementing this interface.

inteface Mapper
{
    Object toDto(DtoData dtoData);
    DtoData toDtoData(Object dto);
}

If you organise your program in this way you have different pieces that cover different responsibilities and it should be trival to change format of DtoData and the way you map them.

One possible improvement over that would be to let the compiler help you putting everything together in the right way. You can do that by rewriting Mapper such that it is generic. Something like Mapper<T,K>.


Rock Paper Scissors Beginner Python Game I recently made this game for rock, paper, scissors in Python 3. Everything works as intended, but I am sure this is not an optimal way of accomplishing this task. Wondering if there is any beginner friendly advice on how to improve upon this. Thank you!

def main():
    import random
    game_active = True
    options = ['Rock', 'Paper', 'Scissors']
    print("\nWelcome to Rock, Paper, Scissors!\n\nPlease choose one:\nR for Rock\nP for Paper\nS for Scissors\n")
    choice = input("Enter your choice: ")
    cpu_choice = random.choice(options)
    if choice != 'R' or 'P' or 'S':
        print("Invalid choice!")
    elif choice == 'R' or 'P' or 'S':
        if choice == 'R':
            choice = "Rock"
        elif choice == 'P':
            choice = "Paper"
        elif choice == 'S':
            choice = "Scissors"

    while choice != cpu_choice:
        if choice == "Rock":
            if cpu_choice == "Paper":
               print("\nYou chose rock and the computer chose paper.\nYou lose!")
            elif cpu_choice == "Scissors":
                print("\nYou chose rock and the computer chose scissors.\nYou win!")
        elif choice == "Paper":
            if cpu_choice == "Rock":
                print("\nYou chose paper and the computer chose rock. \nYou win!")
            elif cpu_choice == "Scissors":
                print("\nYou chose paper and the computer chose scissors. \nYou lose!")
        elif choice == "Scissors":
            if cpu_choice == "Rock":
                print("\nYou chose scissors and the computer chose rock. \nYou lose!")
            elif cpu_choice == "Paper":
                print("\nYou chose scissors and the computer chose paper. \nYou win!")
        break
    while cpu_choice == choice:
        print("You chose", choice + "!")
        print("Computer chooses", cpu_choice + "!")
        break
    cont = input("Play again? (Y/N): ")
    if cont == 'Y':
        main()
    else:
        print("Thank you for playing!")
main()

(This is also my first time using a function)

Welcome to Python!

Choosing Rock/Paper/Scissors/

This

if choice == 'R':
    choice = "Rock"
elif choice == 'P':
    choice = "Paper"
elif choice == 'S':
    choice = "Scissors"

can be

choice = [option for option in options if option.startswith(choice)][0]

Since you're grabbing only the first letter, you can make this comparison, instead of checking each option with its own if statement. This also takes advantage of list comprehension. The array is filled for every value that starts with choice. Since only one value will be in this list, you can simply grab the first value in the list [0].

Utilizing in

This

if choice != 'R' or 'P' or 'S':

can be this

if choice not in "RPS":

This simply checks if the choice is within the string, reducing the need to check each individual character separately.

Unnecessary while

This

while cpu_choice == choice:
    print("You chose", choice + "!")
    print("Computer chooses", cpu_choice + "!")
    break

is basically an if statement, since it's a check and only run once

if cpu_choice == choice:
    print("You chose", choice + "!")
    print("Computer chooses", cpu_choice + "!")

We can make it even neater using string formatting

if cpu_choice == choice:
    print(f"You chose {choice}!")
    print(f"Computer chooses {cpu_choice}!")

Also this

while choice != cpu_choice:

should be this

if choice != cpu_choice:

Imports

Imports should go outside of functions, and at the top of the module

import random

def main():
    ... code ...

Better Input Validation

Right now, you check if the input is valid. If it isn't, you print "Invalid choice!". But you don't stop the program. It keeps running with that choice. You can simplify this by using a while loop here, only breaking if the input is valid:

choice = input("Enter your choice: ")
while choice not in "RPS":
    print("Invalid choice!")
    choice = input("Enter your choice: ")

Unused Variables

This

game_active = True

is never used in your program. You should remove this to avoid confusion, as I initially thought this was the flag that determined if the game was to be run again.


jQuery function to link nested checkboxes $.fn.linkNestedCheckboxes = function () {
    var childCheckboxes = $(this).find('input[type=checkbox] ~ ul li input[type=checkbox]');
    childCheckboxes.change(function () {
        var checked = $(this).attr('checked');
        checked = checked || ($(this).closest('li').siblings('li').find('input:checked').length > 0)
        childCheckboxes.parent().closest('ul').siblings('input[type=checkbox]').attr('checked', checked);
    });

    var parentCheckboxs = childCheckboxes.parent().closest('ul').siblings('input[type=checkbox]')
    parentCheckboxs.change(function () {
        $(this).siblings('ul').find('input[type=checkbox]').attr('checked', $(this).attr('checked'))
    })
    return $(this);
};

Example of use:

<form>
    <input type="checkbox" name="parent" />

<input type="checkbox" name="child1" />

<input type="checkbox" name="child2" />

</form>

The result of this is:

When parent is checked/unchecked all the child checkboxes will be out into the same state
If nothing is checked & one of the children are then checked, the parent will be checked
When the last child is unchecked the parent will be unchecked

What I'd like to know is:

Have I just re-invented the wheel?
If not, is there a more efficient way of doing this? The selectors above seem a bit convoluted.
This works on my machine, but I haven't tested it beyond my limited requirements for one form, will this break anything other than my simplistic example HTML?

Firstly use .prop not .attr if you are using jQuery v1.6+.

Secondly your code has a bug in it. When you click the parent then uncheck both the children then uncheck the parent it will then mark the children checked even though the parent is unchecked.

If you mean that the parent is "checked" when any of the children are "checked":

http://jsfiddle.net/FS3CC/1/

$.fn.linkNestedCheckboxes = function () {
    var childCheckboxes = $(this).find('input[type=checkbox] ~ ul li input[type=checkbox]');

    childCheckboxes.change(function(){
        var parent = $(this).closest("ul").prevAll("input[type=checkbox]").first();
        var anyChildrenChecked = $(this).closest("ul").find("li input[type=checkbox]").is(":checked");
        $(parent).prop("checked", anyChildrenChecked);
    });

    // Parents
    childCheckboxes.closest("ul").prevAll("input[type=checkbox]").first()
        .change(function(){
            $(this).nextAll("ul").first().find("li input[type=checkbox]")
                .prop("checked", $(this).prop("checked"));
        });

    return $(this);
};

Firstly in the //Parents change event you want to set the children to what ever the value of the parent no matter what.

This is the parent element:

// Parents
childCheckboxes.closest("ul").prevAll("input[type=checkbox]").first()

its change event means set all the child check boxes to the same value

.prop("checked", $(this).prop("checked"));

The child check box change event is slightly more complicated but basically it finds any child check boxes that are checked and sets the parent accordingly.

(if you meant that the parent should be check ONLY when all the check boxes are checked its slightly different so please comment as such.)

A different approach would be to specify who the parent is on the element:

<form>
    <input type="checkbox" name="parent" id="parent" />

<input type="checkbox" name="child1" data-parent="parent" />

<input type="checkbox" name="child2" data-parent="parent" />

</form>

Which would make the selectors easier:

(from child)

var parent = $("#" + $(this).data("parent"));

(from parent)

var children = $("input[data-parent=" + $(this).attr("id") + "]");


Exposing a directive's computed variable I have a directive called meetingList with its own isolated scope that receives a list of meetings and displays the filtered items based on some criteria. The controller for this directive holds two variables the meetingList and the filteredMeetingList. The scope where this directive is created needs to know how many items the directive is displaying (filteredMeetingList.length) in order to do something (outside the directive). I am currently exposing this variable using two way databinding on the isolated scope to send the variable outside.

Is there a better way of doing this? is there some kind of standard for this?

I set up a simple Plunker to illustrate the scenario.

HTML

      <meeting-list meeting-list="appCtrl.meetingList" filtered-meetings-count="appCtr.filteredMeetingsCount"></meeting-list>
      Total meetings {{appCtrl.meetingList.length}}

      Meetings being displayed by the meetingList directive {{appCtr.filteredMeetingsCount}}

Javascript

  var app = angular.module('test', []);

  app.directive('meetingList', function(){
    return{
      restrict: 'E',
      scope: {
        meetingList: '=',
        //this is the variable that I use to expose the length of the filtered meetings to the external scope
        filteredMeetingsCount: '='
      },
      controller: function($scope){
        var self = this;
        var meetingList = $scope.meetingList;
        var filteredMeetingList = $scope.filteredMeetingList = [];

        filterMeetings();
        //Here I set the value after filtering
        $scope.filteredMeetingsCount = filteredMeetingList.length;
        function filterMeetings()
        {
          filteredMeetingList.push(meetingList[1]);
          filteredMeetingList.push(meetingList[2]);
        }

      },

      template: '
 {{meeting}}
'
    }
  });

  app.controller('AppCtrl', function($scope){
    var self = this;
    self.meetingList = ["meeting1", "meeting2", "meeting3"];
  });

Given your code, it only filters the list and assigns the value on initialization. From the outside world, the count appears like a read-only value. This is ok... sort of.

One problem I see is that the count can be bound somewhere else and potentially manipulated. This potentially makes the count out of sync with the list count. Your list could say one thing, the count another. I don't know of any way to make it read-only. You could provide a function into the directive, sort of an onchange, where it gets called on modification in the directive and passes in the value there. This would mean that the controller holds responsibility of caching that value in a property. Just make sure that property also doesn't get manipulated though.

Another way you could improve this and avoid the 2-way binding "hack" is to move out the filtering to the enclosing controller. That way, the controller knows about the filtered list and it's length. All you have to do then is to provide that filtered list into your directive, since all it does is just render that list.

An even better way (I guess) is to move out the list and logic into a service/factory. That way, your "single source of truth" is a service, and not the controller nor directive. All you have to do then is pull in the data (full list and a filtered list) from the service/factory into the controller (controller will know about both), and pass in the filtered list to the directive. Any operations that manipulate the data lives in the service/factory, and done via function calls to it. It's essentially what React does with it's Flux architecture, isolating state and state changes into one location to prevent confusion caused by binding and multiple actors.