Cropping a message using array splits I am trying to write a simple function to crop a given message to a specific length but at the same time not to cut the words in between and no trailing spaces in the end.

Example:

Input String: The quick brown fox jumped over the fence, K: 11

Output: The quick

Here is what I have tried:

  function crop(message, K) {
  var originalLen = message.length;
  if(originalLen<K)
  {
      return message;
  }
  else
  {
      var words = message.split(' '),substr;

      for(var i=words.length;i > 0;i--)
       {

           words.pop();

            if(words.join(' ').length<=K)
            {
              return words.join(' ');
            }
       }

  }
}

This function works fine but I am not very happy with the implementation. Need suggestions on the performance aspects and will there be a case where this won't work?

This is much slower than necessary.  It takes time to construct the array, and more to shorten the array word-by-word.  It's easy to imagine how this would go if words contains a whole book and K is some small number.

In general, you want an approach that inspects the original string to decide how much to keep, and then extracts that much, once, before returning it.

A regular expression is an efficient and compact way to find text that meets your criteria.  Consider:

function crop(message, K) {
    if(K<1) return "";
    const reK = new RegExp( `^.{0,${K-1}}[^ ](?= |$)` );
    return ( message.match(reK) || [ "" ] )[0];
}

.match returns an array with the matched text as the first element, or null if no match.  The alternative [ "" ] will provide an empty string as a return value if there is no match (when the first word is longer than K).

The regular expression, broken down, means:

^: match start of string
.: followed by any character
{0,10}: ... up to ten times (one less than K)
[^ ]: followed by a character that is not a space
(?=…): this is an assertion; it means the following expression must match, but is not included in the result:

: followed by a space
|: or
$: end-of-string

Exercise: can you generalize this approach to recognize any kind of whitespace (tabs, newlines, and so on)?


Handle Settings in Windows App So, I fixed my problem with a public ViewModel for my MainPage like this:

MainPage.xaml.cs:

private static MainPageVM Data = new MainPageVM();
public MainPage()
{
    this.InitializeComponent();
    SettingsPane.GetForCurrentView().CommandsRequested += OnCommandsRequested;

    this.DataContext = Data;

    this.SizeChanged += (s, args) =>
    {
        PageSizeChanged();
    };

    Windows.Storage.ApplicationData.Current.DataChanged += (a, o) =>
    {
        CoreApplication.MainView.CoreWindow.Dispatcher.RunAsync(CoreDispatcherPriority.Normal, () =>
        {
            Data.Theme = (int)ApplicationData.Current.RoamingSettings.Values["Theme"];
        });
    };
}

MainPageVM.cs:

private void SetValues()
{
    if (Windows.Storage.ApplicationData.Current.RoamingSettings.Values.ContainsKey("Theme"))
    {
        int value = (int)Windows.Storage.ApplicationData.Current.RoamingSettings.Values["Theme"];
        if (value >= 0 && value <= 2) { Theme = value; }
        else { Theme = 0; }
    }
    else { Theme = 0; }
}

private int _theme = 0;
public int Theme
{
    get { return _theme; }
    set
    {
        if (value == _theme) return;
        _theme = value;
        OnPropertyChanged();
    }
}

Settings.xaml.cs:

private void InitSettings()
{
    if (Windows.Storage.ApplicationData.Current.RoamingSettings.Values.ContainsKey("Theme"))
    {
        int value = (int)Windows.Storage.ApplicationData.Current.RoamingSettings.Values["Theme"];
        if (value >= 0 && value <= 2) { Theme.SelectedIndex = value; }
        else { Theme.SelectedIndex = 0; }
    }
    else { Theme.SelectedIndex = 0; }
}

private void ThemeChanged(object sender, SelectionChangedEventArgs e)
{
    ApplicationData.Current.RoamingSettings.Values["Theme"] = Theme.SelectedIndex;
    ApplicationData.Current.SignalDataChanged();
}

Settings.xaml:

    <StackPanel>
        <TextBlock FontSize="15" Margin="0,5">Text and background color:</TextBlock>
        <ComboBox Name="Theme" SelectionChanged="ThemeChanged">
            <ComboBoxItem Content="Black on White/Purple" Foreground="Black" Background="White"/>
            <ComboBoxItem Content="White on Black/White" Foreground="White" Background="Black"/>
            <ComboBoxItem Content="Black on White/Gold" Foreground="Black" Background="White"/>
        </ComboBox>
    </StackPanel>
</StackPanel>

There is similar code for each setting, but this is a good representation of it.  It doesn't feel right to use an int to represent my theme; should this be an enum?  Please tell me all the problems now so I don't have to refactor again later.

I don't really like the dependencies on all that static stuff. It effectively means your code becomes very hard to test.

Also this code:

if (Windows.Storage.ApplicationData.Current.RoamingSettings.Values.ContainsKey("Theme"))
{
    int value = (int)Windows.Storage.ApplicationData.Current.RoamingSettings.Values["Theme"];
    if (value >= 0 && value <= 2) { Theme = value; }
    else { Theme = 0; }
}
else { Theme = 0; }

seems repeated and should be consolidated into a single method.

I'd start cleaning it up by introducing an IThemeProvider which can be injected into your MainPage, MainPageVM and Settings classes. Would probably look like this:

class ThemeChangedEventArgs : EventArgs
{
    public readonly int NewTheme;

    public ThemeChangedEventArgs(int newTheme)
    {
        NewTheme = newTheme;
    }
}

interface IThemeProvider
{
    int GetCurrentTheme();
    void SetCurrentTheme(int theme);
    event EventHandler<ThemeChangedEventArgs> ThemeChanged;
}

Then your MainPage would look something like this:

private MainPageVM Data = new MainPageVM();
private IThemeProvider _ThemeProvider;

public MainPage(IThemeProvider themeProvider)
{
    _ThemeProvider = themeProvider;

    this.InitializeComponent();
    SettingsPane.GetForCurrentView().CommandsRequested += OnCommandsRequested;

    this.DataContext = Data;

    this.SizeChanged += (s, args) => PageSizeChanged()
    _ThemeProvider.ThemeChanged += (s, e) => Data.Theme = e.NewTheme;
}

MainPageVM:

private IThemeProvider _ThemeProvider;
public MainPageVM(IThemeProvider themeProvider)
{
    _ThemeProvider = themeProvider;
}

private void SetValues()
{
    Theme = _ThemeProvider.GetCurrentTheme();
}

and Settings:

private IThemeProvider _ThemeProvider;
public Settings(IThemeProvider themeProvider)
{
    _ThemeProvider = themeProvider;
}

private void InitSettings()
{
    Theme.SelectedIndex = _ThemeProvider.GetCurrentTheme();
}

private void ThemeChanged(object sender, SelectionChangedEventArgs e)
{
    _ThemeProvider.SetCurrentTheme(Theme.SelectedIndex);
}

Code looks a lot cleaner now I'd say and you can easily mock the IThemeProvider out and test it.

Implementation of the provider could be like this (based on what you currently do):

public class ApplicationDataThemeProvider : IThemeProvider
{
    public ApplicationDataThemeProvider()
    {
        Windows.Storage.ApplicationData.Current.DataChanged += (a, o) =>
        {
            CoreApplication.MainView.CoreWindow.Dispatcher.RunAsync(CoreDispatcherPriority.Normal, () =>
            {
                OnThemeChanged((int)ApplicationData.Current.RoamingSettings.Values["Theme"]);
            });
        };
    }

    public int GetCurrentTheme()
    {
        if (Windows.Storage.ApplicationData.Current.RoamingSettings.Values.ContainsKey("Theme"))
        {
            int value = (int)Windows.Storage.ApplicationData.Current.RoamingSettings.Values["Theme"];
            return (value >= 0 && value <= 2) ? value : 0;
        }
        return 0;
    }

    public void SetCurrentTheme(int theme)
    {
        ApplicationData.Current.RoamingSettings.Values["Theme"] = theme;
        ApplicationData.Current.SignalDataChanged();
    }

    public event EventHandler<ThemeChangedEventArgs> ThemeChanged;
    private void OnThemeChanged(int newTheme)
    {
        var handler = ThemeChanged;
        if (handler != null)
        {
            handler(this, new ThemeChangedEventArgs(newTheme));
        }
    }
}

This encapsulates all the nasty global app data stuff in a single class where it should be a lot easier to handle and debug.


Header navigation with React I'm new to React and started this project as a sandbox to experiment with it. I implemented a main page wich has a header and some content. The content is selected on the header. I'm wondering how it stands on the react best practices and other ways it could be implemented.

On the main app, we hold as state the index of the content we want to show, and we pass to the header as a property a reference to the funtion that handles the index state, like so

 class App extends Component {
  constructor() {
    super();
    this.state = {
      cidx: 0,
      //content:[<Heroes/>, <Teams/>]
    }

    this.updateContent = this.updateContent.bind(this);
    this.getContent = this.getContent.bind(this);
  }

  updateContent(idx) {
    this.setState({cidx:idx});
  }

  getContent() {
    //return this.state.content[this.state.cidx];
    switch(this.state.cidx) {
      case 0:
        return <Heroes/>
      case 1:
        return <Teams/>
    }
  }

  render() {
    const Content = this.getContent();
    return (

        {Content}

    );
  }
}

export default App;

Like that we can inject the content in our render function based on user interaction with the header. I used a switch statement in getContent to avoid having an array of objects in memory all the time. Is it actually better performance-wise ?

This is the header code

class Header extends Component {
  constructor(props) {
    super(props);
  }

  render() {
    const items = this.props.textItems.map(
      (item, index)=>{
        return <NavItem key={index} onClick={()=>{this.props.updateContent(index)}}>{item}</NavItem>
      }
    );
    return (

        <Navbar className="deep-purple" brand='Dota Wizard' right>
          {items}
        </Navbar>

    );
  }
}

export default Header;

A working version of the project is found on https://glitch.com/~absorbing-buzzard

Quick review.
This is a very basic review as I am not a react fan. Do not accept this post as an answer or you will likely dissuade others from adding a better one.
Your question

"I used a switch statement in getContent to avoid having an array of objects in memory all the time. Is it actually better performance-wise ?"

By "better performance-wise"  I assume you mean CPU cycles. The answer is, "No its not better."
Computation complexity
For 2 items the difference is tiny, but if you had a long list of items say n is number of items. To display the last item would require n comparisons so worst case is O(n), and best case display the first item is O(1). Picking random items the complexity is O((log n)c) with c just above 2 (AKA polylogarithmic)
What you had return this.state.content[this.state.cidx];, basicly a lookup table, is O(1) the best performance you can get.
Storage complexity
Many times there is a price to pay for using a lookup table and that is memory. But in this case that makes no difference as you are after already stored items rather than looking up a precomputed value.
As I understand it the react elements you defined as <Heroes/>, <Teams/> are store in an internal node tree, having them in an array means you have an extra item in the state object to reference the array and one reference per item. That would require storage O(2n + 1) rather than O(n). Where n in this case refers to a reference to the react elements, not the elements themselves.
O(2n+1) and O(n) are the same, and tiny compared to the elements actual needs.
Style
You could make a slight change
  updateContent(idx) {
    this.setState({cidx:idx});
  }

to
  updateContent(cidx) {
    this.setState({cidx});
  }

But really I only say so as filler as your code looks good, like its almost a cut and paste from the best of react, a good thing (I am not implying you cut and pasted anything )
I do have a long list of style points in regard to your code, but as that list directly contradicts the react style guide it would be rather bad advice.
Merry Xmas


observing asp.net session Is this useful or is there an obvious other way?

The context is solving the problem of bubbling events in an asp.net site built with user controls and nested user controls.  I found myself writing a ton of events.

So instead of events, i'm using a technique to allow any client to watch an arbitrary session value.

A shared context class for Page and UserControl.  Updates to session values go through here as well as registrations for session value changes.

public class WebSiteContext
{
    private WebSitePage _webSitePage;

    public WebSiteContext(WebSitePage webSitePage)
    {
        _webSitePage = webSitePage;
    }

    internal void SetSessionValue<T>(string key, T value)
    {
        object current = _webSitePage.Session[key];
        if (current != null && current.Equals(value)) return;
        _webSitePage.Session[key] = value;
        if (_dic.ContainsKey(key)) _dic[key](value);
    }

    internal Dictionary<string, Action<object>> _dic = new Dictionary<string, Action<object>>();

    internal void WhenSessionValueChanges(string key, Action<object> action)
    {
        _dic.Add(key, action);
    }
}

Base page constructor creates context instance.

public class WebSitePage : System.Web.UI.Page
{
    public WebSitePage()
    {
        this.WebSiteContext = new WebSiteContext(this);
    }

    public WebSiteContext WebSiteContext { get; set; }
}

Base user control property to get context from parent page.

public class WebSiteUserControl : System.Web.UI.UserControl
{
    public WebSiteContext WebContext {
       get {
          return (this.Page as WebSitePage).WebSiteContext;
       }
    }
}

A user control makes a change to a session value.

public partial class AdvertiserSelector : WebSiteUserControl
{
    protected void ComboBox_SelectedIndexChanged(object sender, EventArgs e)
    {
       this.WebContext.SetSessionValue(
          SessionKeys.current_advertiser_id,
          (sender as ComboBox).SelectedValue);
    }
}

Now in my web page code behind, instead of subscribing to an event, an action is registered for a session value change.

namespace WebApplication1.Screens.CampaignSpend
{
    public partial class Default : WebSitePage
    {
        protected void Page_Load(object sender, EventArgs e)
        {
            this.WebSiteContext.WhenSessionValueChanges(SessionKeys.current_advertiser_id, c => GridView1.DataBind());
        }
    }
}

What you have implemented is just a change event for the session value, but as you are using a delegate instead of event only one control at a time can register for the event.

Use an event instead of a delegate, and you have a regular change event for session variables.


Cleaning up complex if statements for Chess game Shown below is a function I wrote for pawn move generation, I find it difficult to understand what the code does in a glance due to the cumbersome if statements, so any changes I make are done unnecessarily slowly. Is there a way to avoid these complex if statements so that the code is more maintainable?

  if(standardPawn.indexOf(this.board[position.y][position.x]) != -1){
          if(position.x + 1 < 8){
            if(board[position.y][position.x + 1].canEnpas === true && standardPawn.indexOf(board[position.y][position.x + 1]) == -1){
              moves.push({x: position.x + 1, y: position.y - 1});
            }
          }
          if(position.y == 6){
            if(board[position.y - 1][position.x] == "vacant"){
              moves.push({x: position.x, y: position.y - 1});
            }
            if(board[position.y - 2][position.x] == "vacant" && board[position.y - 1][position.x] == "vacant"){
              moves.push({x: position.x, y: position.y - 2});
            }
          } else if(position.y - 1 >= 0){
            if(board[position.y - 1][position.x] == "vacant"){
              moves.push({x: position.x, y: position.y - 1});
            }
          }
          if(position.x + 1 < 8 && position.y - 1 >= 0){
            if(getPieceType({x: position.x + 1, y: position.y - 1}, this.board) != pieceType && board[position.y - 1][position.x + 1] != "vacant"){
              moves.push({x: position.x + 1, y: position.y - 1});
            }
          }
          if(position.x - 1 >= 0 && position.y - 1 >= 0){
            if(getPieceType({x: position.x - 1, y: position.y - 1}, this.board) != pieceType && board[position.y - 1][position.x - 1] != "vacant"){
              moves.push({x: position.x - 1, y: position.y - 1});
            }
          }
        } else {
          if(position.x + 1 < 8){
            if(board[position.y][position.x + 1].canEnpas === true && standardPawn.indexOf(board[position.y][position.x + 1]) != -1){
              moves.push({x: position.x + 1, y: position.y + 1});
            }
          }
          if(position.y == 1){
            if(board[position.y + 1][position.x] == "vacant"){
              moves.push({x: position.x, y: position.y + 1});
            }
            if(board[position.y + 2][position.x] == "vacant" && board[position.y + 1][position.x] == "vacant"){
              moves.push({x: position.x, y: position.y + 2});
            }
          } else if(position.y + 1 < 8){
            if(board[position.y + 1][position.x] == "vacant"){
              moves.push({x: position.x, y: position.y + 1});
            }
          }
          if(position.x + 1 < 8 && position.y + 1 < 8){
            if(getPieceType({x: position.x + 1, y: position.y + 1}, this.board) != pieceType && board[position.y + 1][position.x + 1] != "vacant"){
              moves.push({x: position.x + 1, y: position.y + 1});
            }
          }
          if(position.x - 1 >= 0 && position.y + 1 < 8){
            if(getPieceType({x: position.x - 1, y: position.y + 1}, this.board) != pieceType && board[position.y + 1][position.x - 1] != "vacant"){
              moves.push({x: position.x - 1, y: position.y + 1});
            }
          }
        }

You can enhance the readability by extracting the conditons of your ifs to functions without chanceing the performance. Plus you can remove your duplicate code like positon.x +1 < 8


Efficiency of a linked list partitioning algorithm I have come across a fun puzzle, sorting a linked list, and I wanted to go for quicksort. However, I am not quite sure whether my partitioning algorithm is efficient enough. I also believe that my code is not clean.

Is it possible to remove some variables or make it more efficient?

#include <iostream>
#include <cstdlib>

using namespace std;

struct Node
{
    int data;
    Node *next;
};

void printList(Node *list)
{
    Node *temp = list;
    while(temp != NULL)
    {
        cout<<temp->data<<" ";
        temp = temp->next;
    }
}

int partition(Node *list, int start, int end)
{
    Node *startPtr = list, *endPtr = list, *head = list;

    for(int i = 0; i < start; i++)
        if(startPtr == NULL || startPtr->next == NULL)
            return -1;
        startPtr = startPtr->next;

    for(int i = 0; i < end; i++)
    {
        if(endPtr == NULL || endPtr->next == NULL)
            return -1;
        endPtr = endPtr->next;
    }

    Node *temp = NULL, *temp2 = startPtr;
    int pivot = endPtr->data;

    while(temp2->next != endPtr)
    {
        if(temp2->data < pivot)
        {
            if(temp != NULL)
                temp = temp->next;
            else
                temp = startPtr;

            swap(temp->data, temp2->data);
        }
        temp2 = temp2->next;
    }
    temp = temp->next;
    swap(temp->data, endPtr->data);

    int index;
    Node *temp3 = list;

    for(index = 0; temp3->data != pivot; index++)
        temp3 = temp3->next;

    return index;
}

int main()
{
    Node *a = new Node;
    Node *b = new Node;
    Node *c = new Node;
    Node *d = new Node;
    Node *e = new Node;
    Node *f = new Node;

    a->next = b;
    a->data = 2;
    b->next = c;
    b->data = 6;
    c->next = d;
    c->data = 3;
    d->next = e;
    d->data = 1;
    e->next = f;
    e->data = 7;
    f->data = 5;

    cout<<partition(a, 0, 5)<<endl;

    return 0;
}

Yeah, you're missing brackets, but I think I get what you're trying to do. If I'm wrong, ignore this obviously.

printList can just use a for-loop:

void printList(Node *list)
{

 for(Node *temp = list; temp != NULL; temp = temp->next;)
 {
    cout<<temp->data<<" ";
 }
}

Now for the partition function, you're always returning -1 if startPtr == NULL or endPtr == NULL. But you say that those point to what list points to (at the start of the function). In that case, just check if list is null first. Your function won't do anything if it is anyway:

if(list == NULL)
   return -1;

Now in those for-loops, all you have to do is check if startPtr->next and endPtr->next == NULL.

In your while loop, you have this code:

if(temp2->data < pivot)
{
    if(temp != NULL)
        temp = temp->next;
    else
        temp = startPtr;

    swap(temp->data, temp2->data);
}

But what if, for example, temp == NULL and temp2 == startPtr? Assuming swap() does what it says it does, you're unnecessarily swapping values. It might be expensive, and it might not be. I dunno. The function might also do something weird if you try to swap the same exact values. (As a side note,  you're just passing the value of data. If swap doesn't take values by reference, you won't actually be doing anything to temp->data and temp2->data).

Also, in the declaration of your while loop, you're not checking for if temp2 == NULL.

while(temp2->next != endPtr) //This is will throw a Null Pointer Access Violation exception if temp2 == NULL

This could happen, because you aren't checking if temp2->next == NULL before you say temp2 = temp2->next. If it ever is, your loop will crash the program. I don't know if it's possible for temp2 to reach the end of the list, but if it is, you should just add a conditional.

After your while-loop, you have:

temp = temp->next;
swap(temp->data, endPtr->data);

That could potentially cause the same problems I described before.

Anyway, these are just suggestions. I didn't check thoroughly to see if there were variables you could remove, though.


Redis lookup with a default value I have some code to perform a redis lookup, but if the key to look up either doesn't exist or there's an issue connecting to Redis at all, it will use a default value.

The logic currently looks like this:

begin
 redis = Redis.new(:url => url)
 returned_value = redis.get(key)
 if returned_value == nil and defined?(default) != nil
   default
 else
   returned_value
 end
rescue Exception => e
  if default
    debug "Connection to redis failed with #{e} - Returning default value of #{default}"
    default
  else
    raise(Puppet::Error, "connection to redis server failed - #{e}")
  end
end

It feels like there might be a simpler way of doing this, right now this code feels a little wordy.

Any suggestions?

First of all NEVER rescue ExceptionError. If you want a to rescue from any runtime exception, just omit exception class, otherwise you might catch SignalException which happens upon ruby process received some signal (e.g. SIGTERM) and instead of behave correctly you will silently skip that.

So, your rescue part should look actually like this:

begin
  # ...
rescue SocketError, Redis::CannotConnectError => e
  raise Puppet::Error, "connection to redis server failed - #{e}" unless default
  debug "Connection to redis failed with #{e} - Returning default value of #{default}"
  default
end

Now your main block can be also improved:

begin
  Redis.new(:url => url).get(key) || default
rescue
  # ...
end

Notice that redis returns either String or Nil, e.g.:

Redis.current.set(:x, 123)
Redis.current.get(:x) # => "123"

Redis.current.set(:x, false)
Redis.current.get(:x) # => "false"

So, summing all above together, you can form that into a method:

def redis_get(key, url:, default: nil)
  Redis.new(:url => url).get(key) || default
rescue SocketError, Redis::CannotConnectError => e
  raise Puppet::Error, "connection to redis server failed - #{e}" unless default
  debug "Connection to redis failed with #{e}; Return default: #{default}"
  default
end


Classic programming interview questions I'm going to post some of my solutions to common interview questions. I would love it if you could critique me from a C++ purist's standpoint. Also, tell me if there's a better algorithm for each one of these.

Find the mode (most common element) of an array.

Note: My solution returns an arbitrary mode if there's a tie; assumes the array is non-empty, too

int mode(int* arr, int sz) {
    std::map<int,int> cntMap;
    int most = arr[0];
    for (int i = 0; i < sz; i++) {
       if (!cntMap.count(arr[i])) {
          cntMap[arr[i]] = 0;
       } else {
          cntMap[arr[i]]++;
       }
       if (cntMap[arr[i]] > most) {
          most = cntMap[arr[i]];
       }
    }
    return most;
}

Sum the largest 2 largest elements of an array.

Note: Assumes the array contains at least 2 elements

int largest2(int* arr, int sz) {
    int largest(arr[0]), secondLargest[arr[0]];
    for (int i = 0; i < sz; i++) {
       if (arr[i] > largest) {
          secondLargest = largest;
          largest = arr[i];

       } else if (arr[i] > secondLargest && arr[i] <= largest) {
          secondLargest = arr[i];
       }
    }
    return (largest + secondLargest);
}

Find whether an array as repeated elements, where it's known that the numbers of the array are between 1 and 100.

bool contains_repeats(int* arr, int sz) {
    std::vector<bool> booVec(100,false);
    for (int i = 0; i < sz; i++) {
       if (booVec[arr[i]-1]) {
           return true;
       } else {
           booVec[arr[i]-1] = true;
       }
    }
    return false;
}

Is the signature of the function imposed ? Your solution would work for any kind of iterable objects and would be probably more idiomatic than passing a pointer and a size. Also, the logic would work for any (comparable) types and not just int so it might be worth making it a templated function.
In your search of the most common element, do you want to return the number of occurences or the element itself ?
In your sum of the two biggest, I think you can compare to largest only once.

You just need to do something like :

   if (arr[i] > secondLargest) {
       if (arr[i] > largest) {
           secondLargest = largest;
           largest = arr[i];
       } else {
           secondLargest = arr[i];
       }
   }


Angular template for a dropdown menu I'm ashamed to ask such an elementary question but I tried to DRY up this template a couple ways and couldn't come up with anything I was satisfied with. Can you help?

  %ul.dropdown-menu(aria-labelledby="single-button" role="menu" uib-dropdown-menu)
    %li(role="menuitem")
      %a(href="/legal-forms/admin/document_templates/{{ documentTemplate.id }}/edit") Edit
    %li(role="menuitem")
      %a(href="/legal-forms/admin/document_templates/{{ documentTemplate.id }}/open_in_wizard") Create Document
    %li(role="menuitem")
      %a(href="/legal-forms/admin/document_templates/{{ documentTemplate.id }}/preview" document-template-preview="true" id="documentTemplate.id") Preview
    %li(role="menuitem" ng-hide="documentTemplate.parent_id")
      %a(href=true ng-click="copy(documentTemplate)") Copy
    %li(role="menuitem")
      %a(href=true ng-click="delete(documentTemplate)") Delete

I think this is solid code. All I can think of is a repeat pointing to an array in the controller. I do this on some of our web apps that have high-freq of changes to menu items.

I don't know if this would really be a good refactor otherwise as it just abstracts away more of the template but it definitely DRY's it up.

BTW I'm not familiar with that templating lib so I apologize if there are any problems with the syntax.

template

 %ul.dropdown-menu(aria-labelledby="single-button" role="menu" uib-dropdown-menu)
        %li(role="menuitem" ng-repeat="item in doc_dropdown")
          %a(
             href={{ ( item.is_path ) ? item.pathLocation : true }}
             ng-click={{ item.clickTrigger }}
             ng-hide={{ ( item.can_hide ) ? "documentTemplate.parent_id" : "" }}
            )
                {{item.text}}

somewhere in the controller

doc_dropdown = [
    {
        is_path: true,
        pathLocation: "/legal-forms/admin/document_templates/{{ documentTemplate.id }}/edit",
        text: "Edit",
        click_trigger: null,
        can_hide: false
    },{
        is_path: true,
        pathLocation: "/legal-forms/admin/document_templates/{{ documentTemplate.id }}/open_in_wizard",
        text: "Create Document",
        click_trigger: null,
        can_hide: false
    },{
        is_path: true,
        pathLocation: "/legal-forms/admin/document_templates/{{ documentTemplate.id }}/preview",
        text: "Preview",
        click_trigger: null,
        can_hide: false
    },{
        is_path: false,
        pathLocation: null,
        text: "Copy",
        click_trigger: "copy(documentTemplate)",
        can_hide: true
    },{
        is_path: false,
        pathLocation: null,
        text: "Delete",
        click_trigger: "delete(documentTemplate)",
        can_hide: false
    },
];


Python - Random dice I am a fairly new programmer, and I stumbled with a beginner practice project, a random dice number generator. The code works, but I would like to know if there is an efficient way compared to mine to write it.

#Random dice
import random

#returns a number (side of the dice)
def roll_dice():
  print (random.randint(1, 6))

print("""
Welcome to my python random dice program!
To start press enter!
Whenever you are over, type quit.
""")

flag = True
while flag:
   user_prompt = input(">")
   if user_prompt.lower() == "quit":
      flag = False
   else:
     print("Rolling dice...\nYour number is:")
     roll_dice()

You can make it look nicer!

Instead of writing comments in front of your functions, do it with docstrings:

def roll_dice():
    """Print a number between 1 and 6 (side of the dice)"""
    print(random.randint(1, 6))

You can observe that I made a couple of changes to this:

removed the extra space you had in your print() function
added the docstring I mentioned above
modified the content of docstring (your function doesn't return anything, it just prints a random number). A beginner programmer might get the wrong idea.
used 4-spaces indentation instead of two.
2 new lines in front of your function

Now, let's try to make it even better!

You have two magic numbers, 1 and 6. Since you put the logic inside a function, let's make use of it and define those as arguments to our function:

def roll_dice(min_dice, max_dice):
    """Print a number between min_dice and max_dice (side of the dice)"""
    print(random.randint(min_dice, max_dice))

The above has the advantage of an easier customization of your program. More, you can abstract things even more, and give those arguments a default value:

def roll_dice(min_dice=1, max_dice=6):
    """Print a number between min_dice and max_dice (side of the dice)"""
    print(random.randint(min_dice, max_dice))

More, you can go a step further and make the function actually do something. Let's make it so that it returns a value. Just replace the print() function with return.

Okay, so far so good. I think we've managed to handle this part pretty well. Let's move on.

First, let's apply the changes we did in the first part, to this one too:

print("""
Welcome to my python random dice program!
To start press enter!
Whenever you are over, type quit.
""")

flag = True
while flag:
    user_prompt = input(">")
    if user_prompt.lower() == "quit":
        flag = False
    else:
        print("Rolling dice...\nYour number is:")
        roll_dice()

What I don't like about this, is the fact that you didn't wrap the logic inside a function. Let's do that first:

def play():
    print("""
    Welcome to my python random dice program!
    To start press enter!
    Whenever you are over, type quit.
    """)
    flag = True
    while flag:
        user_prompt = input("> ")
        if user_prompt.lower() == "quit":
            flag = False
        else:
            print("Rolling dice...\nYour number is:")
            roll_dice()

The changes that I'd like to make to this function are the following:

remove the flag variable
move out the intro message in it

def play():
    while True:
        user_prompt = input("> ")
        if user_prompt.lower() == "quit":
            return False
        else:
            print("Rolling dice...\nYour number is: {}".format(roll_dice()))

Moving next, let's build our main() function:

def main():
    print("Welcome to my python random dice program!\n"
          "To start press ENTER!\n"
          "Whenever you are over, type quit.\n")
    play()

Last but not least, let's call our main function:

if __name__ == "__main__":
    main()

You can see I added an extra line: if __name__ == "__main__". By doing the main check, you can have that code only execute when you want to run the module as a program and not have it execute when someone just wants to import your module and call your functions themselves.

The full code:

import random

def roll_dice(min_dice=1, max_dice=6):
    """Print a number between min_dice and max_dice (side of the dice)"""
    return random.randint(min_dice, max_dice)

def play():
    """Return false if user enters 'quit'. Otherwise, print a random number"""
    while True:
        user_prompt = input("> ")
        if user_prompt.lower() == "quit":
            return False
        else:
            print("Rolling dice...\nYour number is: {}".format(roll_dice()))

def main():
    print("Welcome to my python random dice program!\n"
          "To start press ENTER!\n"
          "Whenever you are over, type quit.\n")
    play()

if __name__ == "__main__":
    main()


MVC partial views and AngularJS modules I am using MVC+Angular. In MVC, I created partial views for search, listing, etc. I show them using:

@Html.Partial("_Search")

@Html.Partial("_Featured")

@Html.Partial("_Listing")

Previously each partial view had an ng-app and ng-controller.

Now I combined them all into one ng-app. I added ng-app to master page, and I just used the ng-controller in every partial view.

 <div ng-controller="Search" class="slide">
some ng-repeat goes here
</div>

Angular code:

angular.module("ProductListing", [])
.controller("Search", function ($scope,$templateCache, $http) {
    $(document).on("keyup", ".txtTempleSearch", function () {
        var searchTerm=$(this).val();
        $.get(configUrl + "SearchGrouped/" + searchTerm, function (data) {
            $scope.Results = JSON.parse(data)[0].result;
                $scope.$apply();
        });
    });
})
.controller('featured', function ($scope) {
    $.get(configUrl + 'Featured', function (data) {
        $scope.featuredTemples = data;
        $scope.$apply();
    });
})

Is this approach correct or is it better in terms of maintainability/performance/best practice to have a different ng-app for different partial views and use angular.bootstrap?

I think this is primarily opinion based, but in the AngularJS docs, there is the following:

  Use this directive to auto-bootstrap an AngularJS application. The ngApp directive designates the root element of the application and is typically placed near the root element of the page - e.g. on the <body> or <html> tags.

Also, note that is not possible to have nested ng-app, so, merging the group related controllers into one unique module is a good thing.


Java beginner's console calculator Just started out on Java and I would like to know your opinions on this calculator I made, took me 30 minutes so you can be rough.

import java.util.Scanner;
public class Java101 {

    public static void main(String[] args) {

        System.out.println(" Welcome to Java Calculator v0.1 \n");
        Scanner Scan = new Scanner(System.in);

        System.out.println("\n Please enter two numbers");
        int xe;
        int xo;
        System.out.print("\n First number: ");
        xe = Scan.nextInt();
        System.out.print("\n Second number: ");
        xo = Scan.nextInt();

        System.out.println("\n Select between (*,/,+,-)\n Type out the character in a single letter: ");
        String Operation = Scan.next();
        String EO = "You have selected ";

                switch (Operation) {

        case "*": System.out.println(EO + "* \n Your Result: "+( xe * xo )); break;
        case "/": System.out.println(EO + "/ \n Your Result: "+ ( xe / xo )); break;
        case "+": System.out.println(EO + "+ \n Your Result: "+ ( xe + xo ));break;
        case "-": System.out.println(EO + "- \n Your Result: "+( xe - xo )); break;
        default: System.out.println("\n Please select a valid character"); }

        //Close
        Scan.close();
        System.out.println(" Closing Application ");
    }
}

You created a working calculator. Let me suggest some improvements for the next program version.

First some formal aspects:

Read and follow the Java code conventions: e.g. variables and fields should begin lower-case, closing braces should be placed on their own line. We Java programmers are REALLY used to these conventions, so programs that don't follow them confuse us and lead to mis-interpretations.
Use an IDE that can automatically maintain a proper code indentation (the "switch" line is out of alignment).
Use spacing consistently (insert a space after an opening parenthesis or not?).
You can omit comments like //Close if the next line does exactly this, explicitly. Every reader of your program will see that you are closing your resource, even without your comment.
Use variable names like "operation" (good example), and avoid names like "xe", "xo", or "eo". A variable name should clearly tell what its content means.
Don't declare variables some lines before you use them (like you did with xe and xo). To me, that looks like old-fashioned C language from the 1970s.

So, a formally-improved version might be:

import java.util.Scanner;

public class Java101 {

    public static void main(String[] args) {
        System.out.println(" Welcome to Java Calculator v0.1 \n");
        Scanner scanner = new Scanner(System.in);

        System.out.println("\n Please enter two numbers");
        System.out.print("\n First number: ");
        int firstNumber = scanner.nextInt();
        System.out.print("\n Second number: ");
        int secondNumber = scanner.nextInt();
        System.out.println("\n Select between (*,/,+,-)\n Type out the character in a single letter: ");
        String operation = scanner.next();

        String eo = "You have selected ";

        switch (operation) {
        case "*":
            System.out.println(eo + "* \n Your Result: "+ ( firstNumber * secondNumber ));
            break;
        case "/":
            System.out.println(eo + "/ \n Your Result: "+ ( firstNumber / secondNumber ));
            break;
        case "+":
            System.out.println(eo + "+ \n Your Result: "+ ( firstNumber + secondNumber ));
            break;
        case "-":
            System.out.println(eo + "- \n Your Result: "+ ( firstNumber - secondNumber ));
            break;
        default: System.out.println("\n Please select a valid character");
        }

        scanner.close();
        System.out.println(" Closing Application ");
    }
}

Then regarding structure:

Don't do everything in the main() method. At least separate user interface and algorithmic tasks.
Run your program and have a critical look at the inputs and outputs. Some outputs are unnecessary, some info could be added. E.g. of the three input elements (two numbers and operation), why do you just repeat the operation in the result output?
Your program is procedural. Your next step should be to learn about object orientation. Then your application might get structured into a Calculator and  a CalculationTask class with methods like readTask(), compute() and printResult().


Scraper to grab a Stack Overflow profile name I've created a script in Python to log into stackoverflow.com using credentials and fetch the profilename once logged in. I've tried to do it using class. I created the methods within that class in such a way so that they work like chain. Should I stick to this design or there is anything better I can pursue? Whatever it is I would like this get_profile() method to be seperated like how it is now.

from bs4 import BeautifulSoup
import requests

class StackOverflowBot(object):

    login_url = "https://stackoverflow.com/users/login?ssrc=head&returnurl=https%3a%2f%2fstackoverflow.com%2f"

    def __init__(self,session,username,password):
        self.session = session
        self.username = username
        self.password = password
        self.login(self.session,self.username,self.password)

    def login(self,session,username,password):
        session.headers['User-Agent'] = 'Mozilla/5.0'
        req = session.get(self.login_url)
        soup = BeautifulSoup(req.text, "lxml")
        payload = {
            "fkey": soup.select_one("[name='fkey']")["value"],
            "email": username,
            "password": password,
        }
        req = session.post(self.login_url,data=payload)
        return self.get_profile(req.text)

    def get_profile(self,htmlcontent):
        soup = BeautifulSoup(htmlcontent,"lxml")
        item = soup.select_one("[class^='gravatar-wrapper-']").get('title')
        print(item)

if __name__ == '__main__':
    with requests.Session() as session:
        StackOverflowBot(session,"username","password")

IMO the answer highly depends on how you are planning to extend the functionality of your class.
If its only function is fetching the username, it's probably better to transform it into a function. Class is an overkill.

A few thoughts if you're going to expand it:

You can take a look at the Stack Exchange API and see if it matches your needs if you haven't already.
You can cache user data in the class fields.
If you don't care too much about making one extra request, you can make one when you call get_profile. For this, you'll probably need to store the session credentials.
You can try using HEAD method instead of GET when you don't need the body (dunno though if the site will handle it as intended).

Also, in Python 3 there is no reason to extend object for new classes.


Inverse mapping with bilinear interpolation on an image The following is my solution for an inverse mapping with bilinear interpolation on an image. The original image is img and newmatrix is the transformed image. invRot is the inverse transformation matrix.

How can I optimize the nested for loops (or remove them altogether) to give me a better time complexity for large values of row and col?

newmatrix = np.zeros((row, col, 3), np.uint8)
for r in range(row):
    for c in range(col):
        if offset > 0:
            offset = -1 * offset
        pt = np.array([r+offset,c,1]) #Adjust the offset.
        newpt = np.matmul(invRot, pt) #Reverse map by reverse rotation and pick up color.

        #Check the bounds of the inverse pts we got and if they lie in the original image,
        #then copy the color from that original pt to the new matrix/image.
        if (newpt[0] >= 0 and newpt[0] < (yLen - 1) and newpt[1] >= 0 and newpt[1] < (xLen - 1)):
            x = np.asarray(newpt[1])
            y = np.asarray(newpt[0])

            x0 = np.floor(x).astype(int)
            x1 = x0 + 1
            y0 = np.floor(y).astype(int)
            y1 = y0 + 1

            Ia = img[y0, x0]
            Ib = img[y1, x0]
            Ic = img[y0, x1]
            Id = img[y1, x1]

            color1 = (x1-x) * (y1-y) * Ia
            color2 = (x1-x) * (y-y0) * Ib
            color3 = (x-x0) * (y1-y) * Ic
            color4 = (x-x0) * (y-y0) * Id

            weightedAvgColor = color1 + color2 + color3 + color4
            newmatrix[r][c] = weightedAvgColor

I'm sure there are ways to vectorize this, but I've learned from past examples that it's often easier to just use numba.jit in these cases (where "these cases" means entirely numeric operations with simple loops and no complex python object interactions beyond numpy). You technically ask for a better "time complexity", which would require a different algorithm/approach to this problem. That's not so easy to provide, since your current approach fundamentally requires individually processing how each source pixel is represented in each target pixel; that's O(N) or O(H*W) depending on how you want to frame the number of pixels, and I don't see any way around that. This process could be parallelized using a GPU or vectorized code; however, the easiest thing to try first is to make your current code more efficient. If that provides the speedup you require, then you can stop there.

Just naively putting your code through numba.jit, however, doesn't provide any speedup. To figure out why, I used numba.jit(nopython=True), which errors out when doing anything that numba can't convert into efficient C code. This showed a few minor things to change, such as converting np.floor(x).astype(int) to int(np.floor(x)) (which is equivalent, since these are single integers and not arrays). Also, your modification of offset seems like it would only run once and only on the first iteration, if at all, so I moved it outside the loop. Your bounds checking condition can be simplified with a little Python-Fu. And finally, I've modified your variable names to conform to PEP8 style.

The below code produces the same results are your original code, but is able to be efficiently compiled by numba.jit, providing ~20x speedup in my tests.

import numpy as np
from numba import jit

@jit(nopython=True)
def numba_func(img, inv_rot, offset, row, col):
    y_len, x_len, _ = img.shape

    new_matrix = np.zeros((row, col, 3), np.uint8)
    if offset > 0:
        offset *= -1
    for r in range(row):
        for c in range(col):
            pt = np.array([r + offset, c, 1])
            y, x, _ = inv_rot @ pt  #Reverse map by reverse rotation and pick up color.

            #Check the bounds of the inverse pts we got and if they lie in the original image,
            #then copy the color from that original pt to the new matrix/image.
            if 0 <= y < (y_len - 1) and 0 <= x < (x_len - 1):
                x0 = int(np.floor(x))
                x1 = x0 + 1
                y0 = int(np.floor(y))
                y1 = y0 + 1

                Ia = img[y0, x0]
                Ib = img[y1, x0]
                Ic = img[y0, x1]
                Id = img[y1, x1]

                color1 = (x1-x) * (y1-y) * Ia
                color2 = (x1-x) * (y-y0) * Ib
                color3 = (x-x0) * (y1-y) * Ic
                color4 = (x-x0) * (y-y0) * Id

                weighted_avg_color = color1 + color2 + color3 + color4
                new_matrix[r, c] = weighted_avg_color

    return new_matrix

If that's not fast(er) enough, there are other options, but certainly they'll require a more significant re-work of the code. Again, though, due to the nature of the problem, I don't think you'll be able to achieve better "time complexity", just faster code; this doesn't seem like the kind of problem with a better algorithmic approach, just better implementations.


SslStream-compatible ReadLine extension This code is a Extension to just be able to read a line quickly and efficiently with support for SslStream.

What i'm looking to achieve:

Lower CPU usage by improving how it stores bytes for being converted to string later
Lower CPU usage by possibly not reading per-byte some how. In my debug tests its using up the most CPU by far. 33%/55% of samples are the ReadByte

So your probably asking why im using a Queue as a buffer?
Basically the Queue allows me to more efficiently store bytes in an array format.
byte[] is un-acceptable due to the fact its size can't be changed dynamically (you would have to make a new byte, move over the old data, add the new, replace the old byte[], in-efficient)

queue however (or Stack if you .Reverse() it with linq) lets you add bytes whenever you want and then .ToArray when you need it as a byte[]
Same thing can be done with List<byte> but I dont feel like it's the best tool for the job in this case.

Another thing I could do here is just convert with Encoding.GetString on every byte and StringBuilder it but that seems to use more CPU and a lot more memory.

Any suggestions?

public static string ReadLine(this Stream stream, ref int bodySize, Encoding encoding) {
    bool bodySizeWasSpecified = bodySize > 0;
    byte b = 0;
    Queue<byte> Buffer = new Queue<byte>();
    while (true) {
        #region Try Get 1 Byte from Stream
        try {
            int i = stream.ReadByte();
            if (i == -1) {
                break;//stream ended/closed
            }
            b = (byte)i;
        } catch {
            return null;//timeout //not authenticated context
        }
        #endregion
        #region If there's a body size specified, decrement back 1
        if (bodySizeWasSpecified) {
            bodySize--;
        }
        #endregion
        #region If Byte is \n or \r
        if (b == 10 || b == 13) {
            #region If ByteArray is Empty and the byte is \n reloop so we dont start with a leading \n
            if (Buffer.Count == 0 && b == 10) {
                continue;
            }
            #endregion
            #region We hit a newline, lets finish the reads here.
            break;
            #endregion
        }
        #endregion
        #region Add the read byte to the Byte Array
        Buffer.Enqueue(b);
        #endregion
        #region Break if bodysize was greater than 0 but now its 0
        if (bodySizeWasSpecified && bodySize == 0) {
            break;
        }
        #endregion
    }
    return encoding.GetString(Buffer.ToArray());
}

So your probably asking why im using a Queue as a buffer? Basically the Queue allows me to more efficiently store bytes in an array format. byte[] is un-acceptable due to the fact its size can't be changed dynamically (you would have to make a new byte, move over the old data, add the new, replace the old byte[], in-efficient)

Well if you read the docs for Stream.ReadByte() you would see this

  Notes to Inheritors

  The default implementation on Stream creates a new single-byte array and then calls Read(Byte[], Int32, Int32). While this is formally correct, it is inefficient. Any stream with an internal buffer should override this method and provide a much more efficient version that reads the buffer directly, avoiding the extra array allocation on every call.

and if we dig into the reference source of Stream we would stumble over the default implementation

public abstract int Read([In, Out] byte[] buffer, int offset, int count);

// Reads one byte from the stream by calling Read(byte[], int, int).
// Will return an unsigned byte cast to an int or -1 on end of stream.
// This implementation does not perform well because it allocates a new
// byte[] each time you call it, and should be overridden by any
// subclass that maintains an internal buffer.  Then, it can help perf
// significantly for people who are reading one byte at a time.
public virtual int ReadByte()
{
    Contract.Ensures(Contract.Result<int>() >= -1);
    Contract.Ensures(Contract.Result<int>() < 256);

    byte[] oneByteArray = new byte[1];
    int r = Read(oneByteArray, 0, 1);
    if (r==0)
        return -1;
    return oneByteArray[0];
}

which isn't overwritten neither by NetworkStream nor by AuthenticatedStream (which is inherited by SslStream).

That beeing said, let's dig into the code...

Region's are considered as an antipattern especially if they occur inside a method. Take a look at Are #regions an antipattern or code smell?

Take e.g

    #region If Byte is \n or \r
    if (b == 10 || b == 13)
    {

this should be replaced by either

    if (b == '\n' || b == '\r')
    {

or by

    const char NewLine = '\n';
    const char Return = '\r';

    if (b == NewLine || b == Return)
    {

I don't really like the catch here because it isn't specific. You should consider to remove the try..catch completely and let the calling code handle any thrown exception. This leads to another problem: The method is public but you don't validate if stream is null. You will just let the exception be thrown and catched by the catch I mentioned.

I would add two overloaded ReadLine() methods to distinguish between needing the bodySize and let these methods return an IEnumerable<byte> which I would then convert to a byte[]. The methods would be called like so

 public static string ReadLine(this Stream stream, ref int bodySize, Encoding encoding)
{
    if (stream == null) { throw new ArgumentNullException("stream"); }

    byte[] result;

    if (bodySize > 0)
    {
        result = stream.ReadLine(bodySize).ToArray();
        bodySize -= result.Length;
    }
    else
    {
        result = stream.ReadLine().ToArray();
    }

    return encoding.GetString(result);

}

which is much easier to read.

The methods themself could look like so

private const char NewLine = '\n';
private const char Return = '\r';

private static IEnumerable<byte> ReadLine(this Stream stream, int bodySize)
{
    int i;
    int counter = 0;
    while (bodySize > 0 && (i = stream.ReadByte()) > 0)
    {
        byte b = (byte)i;
        if (b == NewLine || b == Return)
        {
            // Break only if it isn't the first byte.
            if (counter > 0 || b != NewLine) { yield break; }

            continue;

        }
        counter++;
        bodySize--;
        yield return b;
    }
}

private static IEnumerable<byte> ReadLine(this Stream stream)
{
    int i;
    int counter = 0;
    while ((i = stream.ReadByte()) > 0)
    {
        byte b = (byte)i;
        if (b == NewLine || b == Return)
        {
            // Break only if it isn't the first byte.
            if (counter > 0 || b != NewLine) { yield break; }

            continue;

        }
        counter++;
        yield return b;
    }
}

or if we want to be clever we could change the second method like so

private static IEnumerable<byte> ReadLine(this Stream stream)
{
    return stream.ReadLine(int.MaxValue);
}

Please note that the provided code isn't tested.


Lexicographically smallest wave like array without sorting Given an array of integers, sort the array into a wave like array and return it. In other words, arrange the elements into a sequence such that a1 >= a2 <= a3 >= a4 <= a5.....

Example:

Given [1, 2, 3, 4]

One possible answer: [2, 1, 4, 3]

Another possible answer: [4, 1, 3, 2]

NOTE: If there are multiple answers possible, return the one that's lexicographically smallest. So, in example case, you will return [2, 1, 4, 3].

My solution is \$O(n\log n)\$ time since they want lexicographically. I'm unable to come up with a linear time solution for lexicographically smallest case. My questions here is, is it even possible to do that in \$O(n)\$ without sorting? I looked up online quite a bit and don't see any linear time solution for this scenario.

  public ArrayList<Integer> wave(ArrayList<Integer> a) {
    Collections.sort(a);
    if(a.size() == 0 || a.size() == 1)
        return a;
    int temp = -1;
    for(int i = 1; i < a.size(); i++) {
        if(i%2 == 0 && a.get(i) < a.get(i-1)) {
            temp = a.get(i);
            a.set(i, a.get(i-1));
            a.set(i-1, temp);
        }
        if(i%2 == 1 && a.get(i) > a.get(i-1)) {
            temp = a.get(i);
            a.set(i, a.get(i-1));
            a.set(i-1, temp);
        }
    }
    return a;
  }

I don't think this could be done without sorting, but there are a couple of aspects that can be improved with the current code. First of all, there is code duplication; the part

temp = a.get(i);
a.set(i, a.get(i-1));
a.set(i-1, temp);

is duplicated between the two branches of the code. A first step would be to suppress that duplication. There are several approaches, like merging the two if statements in a single one with

if (i%2 == 0 && a.get(i) < a.get(i-1) || i%2 == 1 && a.get(i) > a.get(i-1))

but it becomes a bit unclear. Another approach would be to factor that into a swap method, that swaps two indexes of a list... and you actually don't need to write one, because it already exists! You can just have:

Collections.swap(a, i, i - 1);

without having your own temporary variable.

But there is an even simpler approach: since the list was sorted in ascending order before hand, the result can be found directly by swapping the items two by two. It makes sure that the result is the smallest lexicographically (thanks to the sort) and that each item verify the condition (thanks to the swap). So instead of having an index i going over the number and increasing by 1, have it increase by 2 and always swap i with i + 1.

for (int i = 0; i < a.size() - 1; i += 2) {
    Collections.swap(a, i, i + 1);
}

This takes care of 2 problems at once:

There is no need for any if checks with regard to the parity of i: by construction of the loop, it will always be even, so the loop will swap an element with the element after it, and then go to the next unswapped element.
It also fixed the duplication issue.

Note that the loop needs to go to a.size() - 1 because we're increasing by steps of 2, and an odd number of elements shouldn't cause problems.

if (a.size() == 0 || a.size() == 1)
  return a;

Instead of checking if the size is 0, use isEmpty() which is cleaner. But actually, you don't need those early-returns to begin with, since the rest of the code handles them fine without any overhead (the for loop won't be executed, and the method will return directly). You can safely remove them.

public ArrayList<Integer> wave(ArrayList<Integer> a)

You should make the methods as generic as they can be:

Don't code against ArrayList. What if the user wants to pass another type of list? For example, consider the following:

wave(Arrays.asList(1, 2, 3, 4, 5))

This won't compile because the given list is not an ArrayList. This should be perfectly safe to do, even though asList returns a fixed-size list, the method is not going to add or remove elements from it, just sort and swap values. Therefore, switch to using the general List:

public List<Integer> wave(List<Integer> a)

That also forces the user to want to wave only lists of integers. What if they want to sort longs?

public <T extends Comparable<? super T>> List<T> wave(List<T> a)

would enable that use-case. The elements have to be comparable so that the constraint "a1 >= a2 <= a3 >= a4 <= a5" is possible, so adding extends Comparable makes sense.
The method actually modifies in-place the given list. It does not return a new list, but updates the given list. I would make the method not return the updated list as well. If the method returns something, the user probably expects it to return a new object, not to modify the given argument. Therefore, when the method does modify its argument in-place, don't return it; it makes it clear that the method is doing this in-place modification. The JDK utility methods do this as well (for example in the Collections or Arrays classed).


Google Code Jam Google String problem: a sequence resulting from bit flips and reversals I'm currently having a go at some past Google Code Jam problems to help me practice my programming skills.

One of these problems is Problem A ("Googol String") from Round A APAC Test 2016. Here is the problem for reference (you can find the details via the link):

  Problem

  A "0/1 string" is a string in which every character is either 0 or 1. There are two operations that can be performed on a 0/1 string:

  switch: Every 0 becomes 1 and every 1 becomes 0. For example, "100" becomes "011".
  reverse: The string is reversed. For example, "100" becomes "001".

  Consider this infinite sequence of 0/1 strings:

  S0 = ""

  S1 = "0"

  S2 = "001"

  S3 = "0010011"

  S4 = "001001100011011"

  ...

  SN = SN-1 + "0" + switch(reverse(SN-1)).

  You need to figure out the Kth character of Sgoogol, where googol = 10100.

  Input

  The first line of the input gives the number of test cases, T. Each of the next T lines contains a number K.

  Output

  For each test case, output one line containing "Case #x: y", where x is the test case number (starting from 1) and y is the Kth character of Sgoogol.

After an hour (probably too long to spend on one problem, but it's practice!), I developed a solution.

Here it is:

def switch(num):
    newnum = ""
    for char in num:
        newnum += ("1" if char == "0" else "0")
    return newnum

def reverse(num):
    newnum = ""
    for i in range(len(num)-1, -1, -1):
        newnum += num[i]
    return newnum

def find(digits):
    googol = (10**100)
    s = ["", ""]

    for n in range(1, googol + 1):
        s[1] = s[0] + "0" + switch(reverse(s[0]))

        if len(s[1]) >= digits:
            return s[1]

        s[0] = s[1]

r_fname = "A-small-practice.in"
w_fname = "output-small.txt"

with open(r_fname, "r") as file:
    contents = file.readlines()

with open(w_fname, "w+") as file:
    t = int(contents[0].strip("\n"))

    for case in range(1, t+1):
        k = int(contents[case])
        print("Case {0}: finding the {1}th digit".format(str(case), str(k)))

        string = find(k)
        solution = string[k-1]

        print("Case #{0}: {1}".format(str(case), solution))
        file.write("Case #{0}: {1}\n".format(str(case), solution))

The above can successfully generate an output for the small input file, but the main issue I am having is that it runs far too slow for the large input file (almost as if not running at all).

I have already tried to improve the code a little - I thought adding the line:

s[0] = s[1]

in the for loop would mean that the list will always only consist of two items, reducing memory allocation.

My other fear is that I have coded too much for something that could be written in a more concise way (I'm not sure about this one).

Instead of trying to actually calculate Sgoogol, let's try to figure out some properties of the general case Sn and see if that can help.

\$S_n\$ has an odd number of digits

It is obviously of the form \$2k+1\$

\$S_n\$ has length \$2^n - 1\$

This is easily proved with recursion

\$S_n[\frac{2^n}{2} + 1] = S_n[2^{n-1} + 1] = 0\$

From the definition. I'm assuming 0-based indexing.

If \$k > 2^{n-1} + 1\$, then \$S_n[k] = switch(S_n[(2^n - 1) - 1 - k])\$

From the definition. This allows us to convert between halves of the string.

This suggests a simpler algorithm.

def find_value(index, n):
    if n == 2:
        return [True, True, False][index]
    if k == 2 ** (n-1) + 1:
        return True
    if k > 2 ** (n-1) + 1:
        return not find_value((2^n - 1) - 1 - k, n-1)
    return find_value(k, n-1)

And you can convert to 0 or 1 afterwards. (This may blow the stack, so you may have to rewrite iteratively)


LINQ to Entities query with lots of grouping I have been presented with a simple but slow performing LINQ to Entities query which I've pasted below.

Could anybody give me some pointers on how to improve performance?  The query below has been significantly improved since I inherited it but I don't know what else I can do without moving to the DB (which isn't an option at this stage)

var created = DateTime.Now.AddMonths(-6);

            return (from t1 in db.Opportunities
                    from s1 in db.OpportunityStatus.Where(x => x.OpportunityStatus_ID == t1.StatusReason_ID)
                    from t2 in db.Leads.Where(x => x.Lead_ID == t1.Lead_ID)
                    from t3 in db.Tasks.Where(x => (x.Type_ID == 4) && (x.Item_ID == t1.Opportunity_ID)).DefaultIfEmpty()
                    from t4 in db.TaskAppointments.Where(x => x.Parent_Task_ID == t3.Task_ID).DefaultIfEmpty()
                    from t5 in db.Addresses.Where(x => x.Address_ID == t4.Address_ID).DefaultIfEmpty()
                    from n1 in db.Notes.Where(x => x.Type_ID == 4 && (x.Item_ID == t1.Opportunity_ID)).DefaultIfEmpty()
                    from n2 in db.Notes.Where(x => x.Type_ID == 5 && (x.Item_ID == t1.Lead_ID)).DefaultIfEmpty()
                    from t6 in db.Quotations.Where(x => x.OpportunityId == t1.Opportunity_ID).DefaultIfEmpty()
                    from u1 in db.UserNames.Where(x => x.User_Username == t1.Owner).DefaultIfEmpty()
                    from u2 in db.UserNames.Where(x => x.User_Username == t2.Owner).DefaultIfEmpty()

                    where (t1.Company_ID == company) && (t1.Created >= created)

                    orderby (t1.Created) descending

                    group new { t1, t3, t4, s1, t6, u1, u2, n1, n2, t5 } by new
                    {
                        t1.Opportunity_Title,
                        t2.Company_Name,
                        s1.OpportunityStatus_Name,
                        ts = u1.Name,
                        fs = u2.Name
                    } into g

                    let latestAppointment = g.Max(uh => uh.t4.AppointmentStart)
                    let latestAppointmentStatus = g.FirstOrDefault(x => x.t4.AppointmentStart == latestAppointment).t4.AppointmentStatu.Status_Name
                    let latestAppointmentCity = g.FirstOrDefault(x => x.t4.AppointmentStart == latestAppointment).t5.City

                    let latestNoteDate = g.Max(uh => uh.n2.Date)
                    let latestNote = g.FirstOrDefault(x => x.n2.Date == latestNoteDate).n2.Note_Text

                    let latestOpportunityNoteDate = g.Max(uh => uh.n1.Date)
                    let latestQuotationDate = g.Max(uh => uh.t6.Created)

                    select new PipelineViewModel
                    {
                        Id = 0,
                        CompanyName = g.Key.Company_Name,
                        OpportunityTitle = g.Key.Opportunity_Title,
                        CompanyCity = latestAppointmentCity,
                        OpportunityStatusName = g.Key.OpportunityStatus_Name,
                        LastAppointmentDate = latestAppointment,
                        LastAppointmentStatus = latestAppointmentStatus,
                        LastOpportunityNoteDate = latestOpportunityNoteDate,
                        LastNoteDate = latestNoteDate,
                        LastNote = latestNote,
                        LastQuoteDate = latestQuotationDate,
                        TelesalesUserName = g.Key.ts,
                        FieldSalesUserName = g.Key.fs
                    }).Take(howMany);

This can't be called "simple". And it's not possible to tell where bottleneck is because of so many joins and subqueries.

Subqueries are certainly suspect, you can optimize them by using Dictionary<> (as you would add indexes in real DB).

I would also try to split this huge query into small ones, put ToArray() at the end of each one, and run a profiler to get some hints on what is slow.


My implementation of FixedSizedPriorityQueue I implemented a fixed sized priority queue.
How can I improve my code?

public class FixedSizedPriorityQueue {
    private final int capacity;
    private boolean sorted = false;
    private double smallest = Double.POSITIVE_INFINITY;
    private ArrayList<Double> list;

    protected static final Comparator<Double> comparator = new Comparator<Double>() {
        @Override
        public int compare(Double o1, Double o2) {
            return o2.compareTo(o1);
        }
    };

    public FixedSizedPriorityQueue(int capacity) {
        this.capacity = capacity;
        list = new ArrayList<Double>(capacity);
    }

    public void add(double value) {
        if (list.size() < capacity) {
            list.add(value);
            sorted = false;
            if (value < smallest) {
                smallest = value;
            }
        } else {
            if (value > smallest) {
                if (!sorted) {
                    Collections.sort(list, comparator);
                    sorted = true;
                }
                list.set(capacity - 1, value);
                if (list.get(capacity - 2) > value) {
                    smallest = value;
                } else {
                    smallest = list.get(capacity - 2);
                    sorted = false;
                }
            }
        }
    }

    public int getSize() {
        return list.size();
    }

    public Double poll() {
        if (list.size() == 0) return null;
        if (!sorted) {
            Collections.sort(list, comparator);
            sorted = true;
        }
        Double value = list.get(0);
        list.remove(0);
        if (list.size() == 0) smallest = Double.POSITIVE_INFINITY;
        return value;
    }

    public void print() {
        for (int i = 0; i < list.size(); i++) {
            System.out.println(i + ": " + list.get(i));
        }
        System.out.println("Smallest: " + smallest);
    }
}

The following is used multiple times and it could be extracted out:

if (!sorted) {
    Collections.sort(list, comparator);
    sorted = true;
}

to a helper method:

private void sortIfNeccessary() {
    if (sorted) {
        return;
    }
    Collections.sort(list, comparator);
    sorted = true;
}

Note the guard clause.
List.remove returns the removed elements.

Double value = list.get(0);
list.remove(0);

The code above could be simplified to this:

final Double value = list.remove(0);

The comparator could be substituted with the reverseOrder method/comparator:

Collections.sort(list, Collections.reverseOrder());

The list could be final. final helps readers, because they know that the reference always points to the same instance and it doesn't change later. https://softwareengineering.stackexchange.com/questions/115690/why-declare-final-variables-inside-methods but you can find other questions on Programmers.SE in the topic.
Furthermore, its type could be only List<Double> instead of ArrayList<Double>. See: Effective Java, 2nd edition, Item 52: Refer to objects by their interfaces
The print method violates the single responsibility principle. Other clients may want to write the elements to a log file or a network socket so you shouldn't mix the queue logic with the printing. I'd consider creating a List<Double> getElements method (which would return a copy or unmodifiable version of list to make sure that malicious clients does not modify the inner state of the queue) or providing an iterator to clients and move the print method to another class.
Does it make sense to create a queue with negative capacity? If not, check it and throw an IllegalArgumentException.
Actually, it does not work with capacities less than 2. It throws a java.lang.ArrayIndexOutOfBoundsException: -1 when the capacity is 1 because of the list.get(capacity - 2) call. (Effective Java, Second Edition, Item 38: Check parameters for validity)

public FixedSizedPriorityQueue(final int capacity) {
    if (capacity < 2) {
        throw new IllegalArgumentException("Illegal capacity: " + capacity);
    }
    ...
}

Instead of

if (list.size() == 0) ...

you could use

if (list.isEmpty()) ...

It's the same but easier to read.


Longest Substring Without Repeating Characters in JS The task

is taken from leetcode

  Given a string, find the length of the longest substring without
  repeating characters.

  Example 1:

  Input: "abcabcbb"

  Output: 3

  Explanation: The answer is "abc", with the length of 3.

  Example 2:

  Input: "bbbbb"

  Output: 1

  Explanation: The answer is "b", with the length of 1.

  Example 3:

  Input: "pwwkew"

  Output: 3

  Explanation: The answer is "wke", with the length of 3.
               Note that the answer must be a substring, "pwke" is a subsequence and not a substring.

My solution

var lengthOfLongestSubstring = function(s) {
    const len = s.length;
    if (len < 2) { return len; }
    let res = [];
    let tmp = [];
    [...s].forEach(x => {
      if (tmp.includes(x)) {
        tmp = [...tmp.slice(tmp.findIndex(y => y === x ) + 1)];
      }
      tmp.push(x);
      if (tmp.length > res.length) { res = tmp; }
    });
  return res.length;
};

Style

Nice usage of arrow functions and spread operators. You can extend this to the function name as well:

const lengthOfLongestSubstring = s => { ... };

The code switches between two and four spaces within a block and between blocks. Choose one and stick with it throughout the entire program (the auto-formatter built into Stack Exchange does the job well).
Avoid the intermediate variable len here:

    const len = s.length;
    if (len < 2) { return len; }

The extra variable obfuscates the direct and explicit s.length and offers no performance benefit (as it would in C with strlen, which is linear). s.length is a numerical property and doesn't walk the list per call.
Use vertical whitespace around blocks. Here's how I'd rewrite the above lines:

    ...
    if (s.length < 2) {
      return s.length;
    }

    let res = [];
    ...

Then again, I prefer to omit this logic because the rest of the function body will handle the precondition automatically.
Variable names can be a bit more specific: tmp => seen, x => char, res => longest.

Performance

Your solution runs in O(len(longest_subsequence) * n) time, which is in the 70th percentile of solutions for this problem. The culprits are includes, findIndex, slice and spread inside the for loop, all of which require visiting up to every element in tmp.

We can improve the time complexity to linear and reach the 99th percentile. The key is using an object as a hash map instead of an array to keep track of the history. For each character in s, add it to the object with a value of its latest-seen index. Keep track of the start of the current candidate run. Whenever we encounter an item already in the map, check to ensure its index is indeed inside the current run. If so, record a new longest (if applicable) and begin a new candidate run.

Here's the code:

const lengthOfLongestSubstring = s => {
  let longest = 0;
  let start = 0;
  const seen = {};

  [...s].forEach((char, i) => {
    if (char in seen && start <= seen[char]) {
      longest = Math.max(i - start, longest);
      start = seen[char] + 1;
    }

    seen[char] = i;
  });

  return Math.max(s.length - start, longest);
};


Batch file to copy specific folders This is the batch file i have created which copy the specific folders which I want. I use the specific server folder name of which I want to copy. Please suggest any improvements.

@echo off
:: variables
echo This script takes the backup of file SwiftALM Important folders
set /P source=Enter source folder Example D:\jboss6.1\server\swift:
set /P destination=Enter Destination folder:
set /P Folder=Enter Folder name:
@echo folder=%folder%
mkdir %destination%\%folder%
set xcopy=xcopy /E/V/Q/F/H/I
echo echo conf folder will be copied
%xcopy% %source%\conf %destination%\%folder%\conf
echo conf folder is copied
echo lib folder will be copied
%xcopy% %source%\lib %destination%\%folder%\lib
echo lib folder is copied
echo deploy folder will be copied
%xcopy% %source%\deploy %destination%\%folder%\deploy
echo deploy folder is copied
echo deployers folder will be copied
%xcopy% %source%\deployers %destination%\%folder%\deployers
echo deplyers folder is copied
echo files will be copy press enter to proceed
pause

SETLOCAL ensures that any environment variables you set don't affect the calling process's environment. It's like a sandbox for variables, directory changes, and other shell settings.

I usually use @ECHO instead of plain ECHOs so that if the @ECHO OFF is ever temporarily disabled (say, for debugging purposes), the echo statements don't produce needless noise in the output.

I also suggest using a loop to repeat the nearly-duplicate statements:

@ECHO OFF
SETLOCAL
:: variables
@ECHO This script takes the backup of file SwiftALM Important folders
SET /P source=Enter source folder Example D:\jboss6.1\server\swift:
SET /P destination=Enter Destination folder:
SET /P folder=Enter Folder name:

@ECHO folder=%folder%

MKDIR %destination%\%folder%
SET xcopy=xcopy /E/V/Q/F/H/I

FOR /F %%s IN (conf lib deploy deployers) DO (
    echo %%s folder will be copied
    %xcopy% %source%\%%s %destination%\%folder%\%%s
    IF NOT ERRORLEVEL 1 @ECHO %%s folder has been copied.
)

@ECHO Files have been copied. Press enter to proceed.
PAUSE


What is the best way to extract method? The code snippet is what I want to do. Previously I use a different version of my code. But it is out of joint according to Python coding guidelines because I was assigned my lambda function to a variable. It was an obvious violation. Here is my violator code:

my_dict = {
    'first_name': 'Jimmy Floyd',
    'last_name': 'Hasselbaink'
}

db_data = {}

def create_info(db_data, data):
    split_join = lambda field: '_'.join(data.get(field).split()) if field in data else None
    name = split_join('name')
    first_name = split_join('first_name')
    last_name = split_join('last_name')
    code = '_'.join(filter(None, [name, first_name, last_name])).lower()
    db_data['code'] = code
    return db_data

Then I have extracted my split_join variable to a method. The code is below:

my_dict = {
    'first_name': 'Jimmy Floyd',
    'last_name': 'Hasselbaink'
}

db_data = {}

def split_join(data, field):
    return '_'.join(data.get(field).split()) if field in data else None

def create_info(db_data, data):
    name = split_join(data, 'name')
    first_name = split_join(data, 'first_name')
    last_name = split_join(data, 'last_name')
    code = '_'.join(filter(None, [name, first_name, last_name])).lower()
    db_data['code'] = code
    return db_data

new_db_data = create_info(db_data, my_dict)
print(new_db_data)

I have forced into writing split_join method in three times. Is there any way to prevent this situation?

db_data = {}

def create_info(db_data, …):
    …
    return db_data

new_db_data = create_info(db_data, …)

This feels plain wrong. Either your create_info function mutates data, or it generates some; but not both. Judging by the name, your function should create the dictionary it returns:

def create_info(data):
    …
    code = …
    return {'code': code}

db_data = create_info(my_dict)
print(db_data)

If you want to avoid passing data as a parameter to split_join and not use a lambda altogether, you can still define a nested function which will capture the needed variables:

def create_info(data):
    def split_join(field):
        return '_'.join(data.get(field).split()) if field in data else None
    …
    code = …
    return {'code': code}

db_data = create_info(my_dict)
print(db_data)

You can avoid calling split_join several times by using map:

def create_info(data):
    def split_join(field):
        return '_'.join(data.get(field).split()) if field in data else None
    name, first_name, last_name = map(split_join, ('name', 'first_name', 'last_name'))
    code = '_'.join(filter(None, [name, first_name, last_name])).lower()
    return {'code': code}

db_data = create_info(my_dict)
print(db_data)

What is interesting with that, is that you can feed the result of map directly to the filter:

def create_info(data):
    def split_join(field):
        return '_'.join(data.get(field).split()) if field in data else None

    code = '_'.join(
            filter(
                None,
                map(split_join, ('name', 'first_name', 'last_name'))
            )
    )
    return {'code': code.lower()}

db_data = create_info(my_dict)
print(db_data)

Last improvement would be to not rely on None being generated when there is no data available, but '' instead, since you’re only manipulating strings:

def create_info(data):
    def split_join(field):
        return '_'.join(data.get(field, '').split())

    code = '_'.join(
            filter(
                bool,
                map(split_join, ('name', 'first_name', 'last_name'))
            )
    )
    return {'code': code.lower()}

db_data = create_info(my_dict)
print(db_data)

Update According to your comment:

You can obviously remove the need for a nested function by switching back to a lambda:

def create_info(data):
    code = '_'.join(
            filter(
                bool,
                map(lambda field: '_'.join(data.get(field, '').split()),
                    ('name', 'first_name', 'last_name')
                )
            )
    )
    return {'code': code.lower()}

db_data = create_info(my_dict)
print(db_data)

But readability is becoming worse. In any case, it is often recommended to convert map + lambda into an explicit list-comprehension or generator expression instead. I’m choosing the generator expression here because we won't need an intermediate list:

def create_info(data):
    code = '_'.join(
            filter(
                bool, (
                    '_'.join(data.get(field, '').split())
                    for field in ('name', 'first_name', 'last_name')
                )
            )
    )
    return {'code': code.lower()}

db_data = create_info(my_dict)
print(db_data)


Validation for DTO using DataAnnotations I need validation for all DTO objects using System.ComponentModel.DataAnnotations.
You can see how I implemented. Idea is to have one abstract class that will be inherited  in all dto classes.
This abstract class have check if object is valid and get all validation results.

Is this good approach?

What do you think?

dto base class :

  public abstract class DtoBase : IValidatableObject
    {
        public virtual IEnumerable<ValidationResult> GetValidationResult()
        {
            return Validate(new ValidationContext(this));
        }

        public bool IsValid()
        {
            return Validate(new ValidationContext(this)).Count() == 0;
        }

        public virtual IEnumerable<ValidationResult> Validate(ValidationContext validationContext)
        {
            var results = new List<ValidationResult>();
            Validator.TryValidateObject(this, validationContext, results, true);

            return results;
        }
    }

dto work item :

 public class WorkItemDto : DtoBase
    {
        public WorkItemDto()
        {
        }

        public int Id { get; set; }

        [StringLength(500, MinimumLength = 200)]
        public string Description { get; set; }

        [Range(20, 5000)]
        public int ItemNumValue { get; set; }

        public ICollection<ItemUsageDto> Usage { get; set; }
    }

Example how to use in wpf or in mvc project (it have to work in any client):

 var item = new WorkItemDto();
 item.Description = "my descryption";
 item.ItemNumValue = 5;
 item.Id = 7;

 var isValid = item.IsValid();
 var allResults = item.GetValidationResult();

What do you think about non static validators? While making validators smarter, we will sooner or later need to wire up their dependencies (like repositories)... Validators can easily do much more while being instantiated by IoC container.

I would suggest to define this interface and class:

interface IModelValidator<in T>
{
    IEnumerable<string> Validate(T subject);
}

class ModelValidator<T> : IModelValidator<T>
{
    public ModelValidator(IEnumerable<IModelValidator<T>> validators)
    {
        Validators = validators;
    }

    public IEnumerable<string> Validate(T subject) =>
        Validators.SelectMany(v => v.Validate(subject));

    IEnumerable<IModelValidator<T>> Validators { get; }
}

Now some demo code; models:

class Order { }
class MailOrder : Order { }

Validators:

class OrderValidator : IModelValidator<Order>
{
    public IEnumerable<string> Validate(Order subject)
    {
        yield return "Bad order.";
    }
}

class MailOrderValidator : IModelValidator<MailOrder>
{
    public IEnumerable<string> Validate(MailOrder subject)
    {
        yield return "Bad mail order.";
    }
}

Something to check validity of the order:

class Controller
{
    public Controller(ModelValidator<MailOrder> validator)
    {
        var order = new MailOrder();
        foreach (var message in validator.Validate(order))
            Console.WriteLine(message);
    }
}

So now it is just about proper registering them in IoC container (here Autofac):

class Program
{
    static void Main(string[] args)
    {
        var builder = new ContainerBuilder();
        builder.RegisterSource(new ContravariantRegistrationSource());
        builder.RegisterGeneric(typeof(ModelValidator<>))
            .AsSelf();
        builder.RegisterType<OrderValidator>()
            .AsImplementedInterfaces();
        builder.RegisterType<MailOrderValidator>()
            .AsImplementedInterfaces();
        builder.RegisterType<Controller>()
            .AsSelf();

        IContainer container = builder.Build();
        var controller = container.Resolve<Controller>();
    }
}

You could play with it to discover how perfectly contravarians works here. You just declare what to validate - compatible validators will show up. No need to change client code - just register more validators if necessary. One of them might be IModelValidator<DtoBase> with attribute checking.


C++ code that convolves a data file for smoothing random data curves of microcanonical ensemble graphs I am new to this, I have a convolution program that takes in a data file, convolves it and outputs another file. I am attaching the code here.

#include<bits/stdc++.h>
using namespace std

void convolute()
{
    ifstream fin;
    ofstream fout;

    int count = 0;
    double a=0,b=0;

    string input_file_string = "maxclus_500000node_3M_5000ens_666.dat";

    string output_file_string = "1convolute_"+input_file_string;

    fin.open(input_file_string.c_str());

    while(fin) //to know the size of array to initialize
    {
        fin>>a>>b;
        count++;

    }

    fin.close();

    double* c = NULL;
    c = new double[count+1];
    double* d = NULL;
    d = new double[count+1];

    for(int i=0;i<count+1;i++)
    {
        c[i] = 0;
        d[i] = 0;
    }

    fin.open(input_file_string.c_str());

    int n = 1;

    while(fin) //takes in data
    {
        fin>>a>>b;
        c[n] = a;
        d[n] = b;
        n++;
    }

    fin.close();

    double* binom = NULL;
    binom = new double[count];

    double* summ = NULL;
    summ = new double[count+1];

    for(int i=0;i<count+1;i++) summ[i] = 0;

    for(int i=0;i<count;i++) binom[i] = 0;

    //here the main convolution of data takes place
    for(int j=1;j<count;++j)
    {
        double prob = j*1.0/(count-1);
        binom[j] = 1;

        for(int i=j+1;i<count;++i)
        binom[i] = binom[i-1]*((count-1)-i+1)*1.0/i*prob/(1-prob);

        for(int i=j-1;i>=0;--i)
        binom[i] = binom[i+1]*(i+1)*1.0/((count-1)-i)*(1-prob)/prob;

        double sum = 0;

        for(int i=0;i<count;++i) sum += binom[i];
        for(int i=0;i<count;++i) binom[i] /= sum;

        sum = 0;

        for(int i=1;i<count;++i) sum += d[i]*binom[i];

        summ[j] = sum;

        if(j%1000==0)// just to make the process faster and check progress
        cout<<count-1<<'\t'<<j<<endl;

    }

    cout<<"writing to file "<<endl;

    fout.open(output_file_string.c_str());

    for(int i=1;i<count;i++) fout<<c[i]<<'\t'<<summ[i]<<endl;

    fout.close();

    delete [] c;
    c = NULL;
    delete [] d;
    d = NULL;

    delete [] binom;
    binom = NULL;

    delete [] summ;
    summ = NULL;

}

int main()
{
     convolute();

     return 0;
}

The main convolution takes place inside the 'j' loop, i have marked it inside the code, the data file it handles, the smallest one is with 1499994 points (all 'double' points, 2columns), it takes over 15++ hours to finish the whole program, badly in need of help to optimize it, so that it still does the same thing, but wayyyyyy faster!
p.s. i am also using gcc optimization flags, -Ofast -march=native -funroll-loops etc but still no improvement.

Algorithm takes \$O(n^2)\$ time

Fundamentally, your algorithm takes \$O(n^2)\$ time because it used two nested loops.  On a large input such as 1.5 million items, you must perform at a minimum somewhere around \$2.25 * 10^{12}\$ operations, which will definitely take some time.

However, there are at least two main ways I can think of to speed up your program:

Use multithreading to take advantage of your computer's multiple cores.  Since your program is just doing a massive computation, you can split the work so that each of N cores does 1/N of the work.  Assuming you had a 4 core computer, you could reasonably expect to get a 4x speedup in your program.  It is beyond the scope of my review to present a multithreaded solution, but you could research how to do this on your own.
You can improve the main loop.  I noticed a few things that you could do to speed up your main loop, which I will present in the following sections.

Remove the division loop

The first thing I noticed is that you have a division loop, but it isn't necessary.  Look here:

    double sum = 0;

    for(int i=0;i<count;++i) sum += binom[i];
    for(int i=0;i<count;++i) binom[i] /= sum;  // <-- This loop

    sum = 0;

    for(int i=1;i<count;++i) sum += d[i]*binom[i];

    summ[j] = sum;

Notice how you compute the sum of the binom array, divide each of the binom[i] entries by the total, and then add up the total of d[i] * binom[i].  Mathematically, if you skipped the division loop and just divided by the total at the end, you would get the same answer.  In other words, you could rewrite the above like this:

    double total = 0;

    for(int i=0;i<count;++i) total += binom[i];

    double sum = 0;

    for(int i=1;i<count;++i) sum += d[i]*binom[i];

    summ[j] = sum / total;

Note that the division loop was the costliest of the three loops, since division is slower than addition and multiplication.

No need for binom array

The next thing you could do is to compute the sum on the fly instead of storing your intermediate results into a binom array.  The binom array isn't really needed except to compute the final sum.  So you could just add to sum as you compute the binom values instead of creating an array and adding up the array at the end.  So I rewrote your main loop to look like this:

    //here the main convolution of data takes place
    for (int j=1;j<count;++j) {
        double prob     = (double) j / (count-1);
        double factor   = 0;
        double binom    = 0;
        double prev     = 0;
        double binomTot = 1;
        double sum      = d[j];

        factor = prob / (1-prob);
        prev   = 1;
        for (int i=j+1;i<count;++i) {
            binom     = prev * ((count-1)-i+1)/i*factor;
            binomTot += binom;
            sum      += d[i] * binom;
            prev      = binom;
        }

        factor = (1-prob)/prob;
        prev   = 1;
        for (int i=j-1;i>=0;--i) {
            binom     = prev * (i+1) / (count-1-i) * factor;
            binomTot += binom;
            sum      += d[i] * binom;
            prev      = binom;
        }

        summ[j] = sum / binomTot;

        if(j%1000==0)// just to make the process faster and check progress
        cout<<count-1<<'\t'<<j<<endl;
    }

With this modified loop, the program ran 2x faster than the original program, on a test case of 20000 random floats.

Cache reused values

Finally, you can speed up your loop even more by caching the values that are reused on each loop.  I noticed that these values:

((count-1)-i+1)/i
(i+1) / (count-1-i)

are used in your inner loops, and they only depend on i and count.  So instead of doing the computation over and over again, we can precompute these values once and then just use the precomputed values in the inner loops.  This avoids costly division operations.

Here is a rewrite using cached values:

double *factor1 = new double[count];
double *factor2 = new double[count];

for (int i=0;i<count;++i) {
    factor1[i] = (double) ((count-1)-i+1) / i;
    factor2[i] = (double) (i+1) / (count-1-i);
}

//here the main convolution of data takes place
for (int j=1;j<count;++j) {
    double prob     = (double) j / (count-1);
    double factor   = 0;
    double binom    = 0;
    double prev     = 0;
    double binomTot = 1;
    double sum      = d[j];

    factor = prob / (1-prob);
    prev   = 1;
    for (int i=j+1;i<count;++i) {
        binom     = prev * factor1[i] * factor;
        binomTot += binom;
        sum      += d[i] * binom;
        prev      = binom;
    }

    factor = (1-prob)/prob;
    prev   = 1;
    for (int i=j-1;i>=0;--i) {
        binom     = prev * factor2[i] * factor;
        binomTot += binom;
        sum      += d[i] * binom;
        prev      = binom;
    }

    summ[j] = sum / binomTot;

    if(j%1000==0)// just to make the process faster and check progress
    cout<<count-1<<'\t'<<j<<endl;
}
delete [] factor1;
delete [] factor2;

This rewrite was about 4x faster than the original version, on a test case of 20000 random floats.  However, on a test case of 100000 random floats, it was 14x faster.  My guess is that the original program scaled badly with larger inputs because of the temporary binom array polluting the cache.  I would advise testing this version for yourself on your own computer with various sized inputs to see how it performs.


Conway's Game of Life without objects I was wondering if anyone could provide some feedback on my solution to Conway's game of life. I tried to solve it without using objects. It returns a dictionary of living points on a grid and points that were alive at some point. I'm wondering if there is a better way to check the surrounding points or if storing the active points in a dictionary is a bad idea.

def list_to_check(x,y):
    return  [(x + 1, y),
             (x - 1, y),
             (x +1, y +1 ),
             (x -1, y -1 ),
             (x +1, y -1 ),
             (x -1, y +1 ),
             (x, y+1),
             (x, y-1)]

def neighbors_count(x,y,cells):
    count = 0
    for item in list_to_check(x,y):
       if item in cells:
           count += cells[item]
    return count

def next_state(cells):
    next_state = {}
    for point in cells.keys():
        alive = cells[point] == 1
        neighbors = neighbors_count(point[0],point[1],cells)
        if not alive and neighbors == 3:
            next_state[point] = 1
        elif neighbors > 3 or neighbors < 2:
            next_state[point] = 0
    return next_state

def add_to_grid_alive(cells):
    to_check = []
    to_add = {}
    for item in cells.keys():
        checking = [ x for x in list_to_check(item[0],item[1]) if x not in to_check and x not in cells ]
        to_check += checking

    for item in to_check:
        if neighbors_count(item[0],item[1],cells) == 3:
            to_add[(item[0],item[1])] = 1
    return to_add

def tick(cells):
    changes = next_state(cells)
    expansions = add_to_grid_alive(cells)
    cells.update(changes)
    cells.update(expansions)
    [ cells.pop(point,None) for point in cells.keys() if cells[point] == 0 ]

def print_grid( size, cells):
    grid = []
    for y in range(size):
        grid.append([])
        for x in range(size):
            grid[y].append(0)
    for point in cells.keys():
        if  0 <= point[0] < size and 0 <= point[1] < size:
            grid[point[1]][point[0]] = cells[point]
    return grid

#testing

def load_list(inputs):
    point_dict = {}
    for point in inputs:
        point_dict[point] = 1
    return point_dict

inputs = [(1,2),(2,2),(3,2),(2,3),(3,3),(4,3),(13,2),(13,3),(12,3),(13,4)]

test_array_dict = load_list(inputs)

for x in range(20):

    print '>>>>>>>>>>>>>>>>>>>>', x + 1

    for item in print_grid(20,test_array_dict)[::-1]:
        print item
    tick(test_array_dict)

Update

I added [ cells.pop(point,None) for point in cells.keys() if cells[point] == 0 ] to the tick function and it really sped up the the code on higher iterations

You're representing the cells mainly using a dictionary of coordinates, converting to a two-dimensional array only for the purposes of printing the output.  That is a good representation for sparse boards, but not so much for crowded boards.

The algorithm that you use, though, is cumbersome, particularly illustrated by these few lines:

for item in cells.keys():
    checking = [ x for x in list_to_check(item[0],item[1]) if x not in to_check and x not in cells ]
    to_check += checking

In other words, make a list of the neighbors of each cell, but make sure that each coordinate is listed just once.  Then, once you have that list to_check, what do you do with it?

for item in to_check:
    if neighbors_count(item[0],item[1],cells) == 3:
        to_add[(item[0],item[1])] = 1

You count each cell's neighbors!  But the number of neighbors is precisely the amount of overlapped processing that would have occurred had you not bothered to deduplicate the to_check list to begin with!  So, instead of drawing a list of interesting cells and counting their neighbors, why not just have each existing cell increment the neighbor count of each neighboring coordinate?

In addition to the change in algorithm, I also recommend:

Take advantage of list/set/dict comprehensions more.
Represent the board as a set of live cells, rather than a dict.
Rename list_to_check() to neighbors().
Rename print_grid() to grid(), since it actually doesn't print anything.
Construct the next state from scratch rather than by mutation: test_array_dict = tick(test_array_dict)

I'd rewrite three functions as follows (and eliminate neighbors_count(), next_state(), and add_to_grid_alive():

def tick(live_cells):
    """ Takes a set of coordinates of live cells, and returns a set of
    coordinates of the live cells in the next generation. """
    neighbor_count = {}
    for xy in live_cells:
        for neighbor in neighbors(*xy):
            neighbor_count[neighbor] = 1 + neighbor_count.get(neighbor, 0)
    return set(xy for xy in neighbor_count
                   if neighbor_count[xy] == 3
                   or xy in live_cells and neighbor_count[xy] == 2)

def grid(size, cells):
    return [[int((x, y) in cells) for x in range(size)] for y in range(size)]

def load_list(inputs):
    return set(inputs)

Actually, tick() would be even better written using a collections.Counter.  I don't know if you consider that "using an object".