Extracting an XML config file from ZIP file and parsing it I am currently working with an app that should load various info on a very specific zip file which contains the XML config file, using the DotNetZip library on C#, here's the code:

        public static void InitializeData(string configFilePath)
    {
        if (File.Exists(configFilePath))
        {
            using (ZipFile zip1 = ZipFile.Read(configFilePath))
            {
                zipEntriesCount = zip1.Count;
                if (zip1.ContainsEntry(installConfigFileName))
                {
                    var entries = zip1.SelectEntries(installConfigFileName);
                    ZipEntry[] entriesArray = new ZipEntry[entries.Count];
                    entries.CopyTo(entriesArray, 0);
                    entriesArray[0].Extract(installConfigStream);
                    installConfigData = StreamToString(installConfigStream);
                    XmlDocument xmlDoc = new XmlDocument(); // Create an XML document object
                    xmlDoc.Load(installConfigStream); // Load the XML document from the specified file

                    // Get elements
                    XmlNodeList XmlAppTitle = xmlDoc.GetElementsByTagName("appTitle");
                    XmlNodeList XmlAppCompany = xmlDoc.GetElementsByTagName("appCompany");
                    XmlNodeList XmlAppIconPath = xmlDoc.GetElementsByTagName("appIconPath");
                    XmlNodeList XmlAppLargeIconPath = xmlDoc.GetElementsByTagName("appLargeIconPath");

                    // Store app config data globally
                    // eg: globalVar = XmlVar[0].InnerText;
                    appTitle = XmlAppTitle[0].InnerText;
                    appCompany = XmlAppCompany[0].InnerText;
                    appIconPath = XmlAppIconPath[0].InnerText;
                    appLargeIconPath = XmlAppLargeIconPath[0].InnerText;
                }
            }
        }
    }

My question is, is there any better/more compact way of reaching same functionality?

The repeated part of your code can be extracted into a sub-function.

string InnerTextOfFirst(XmlDocument doc, string tagName)
{
  return doc.GetElementsByTagName(tagName)[0].InnerText;
}

Then use it like this:

appTitle = InnerTextOfFirst(xmlDoc, "appTitle");
appCompany = InnerTextOfFirst(xmlDoc, "appCompany");
appIconPath = InnerTextOfFirst(xmlDoc, "appIconPath");
appLargeIconPath = InnerTextOfFirst(xmlDoc, "appLargeIconPath");

This removes about a third of the inner most block.

Things to look out for:

What happens if the xml file does not have one of the tags you are expecting? It's likely that this block of code won't know the correct thing to do in this case. You should write the code so that when this happens, it is clear to the caller what the real issue is.
The C# naming convention for local variables is camelCase. While most of the variables are named correctly, the XmlNodeList variables are not.
There is a comment that refers to global variables. Having global mutable variables will make it harder to reason about how the code go into a certain state. It would be better to create a simple data class and have the code that parses the file return a new instance of that class with the parsed values.


Reduce repetition in Pandas DataFrame column assignments Could the aggregate functions after using grouping functions like groupby() and resample() be looped for cleaner code

____DateTime.____    |  Steps  | day_segment
2017-10-25 00:00:00| 4  | night
2017-10-25 00:01:00| 7  | night
2017-10-25 00:02:00| 10 | night
2017-10-25 00:03:00| 26 | night
.
.
.
2017-10-25 10:04:00| 80 | morning
2017-10-25 10:05:00| 45 | morning
2017-10-25 10:06:00| 53 | morning

DataFrame: data

day_segments = ['morning','afternoon','evening','night']
#all_steps is a list input from the user
all_steps = ['sumallsteps','maxallsteps','minallsteps','avgallsteps','stdallsteps']
finalDataset = pd.DataFrame()

#Descriptive Statistics Features:
for day_segment in day_segments:
    resampledData = pd.DataFrame()
    resampledData = data.loc[data['day_segment'] == str(day_segment)]
    if("sumallsteps" in all_steps):
        finalDataset["step_" + str(day_segment) + "_sumallsteps"] = resampledData['steps'].resample('D').sum()

    if("maxallsteps" in all_steps):
        finalDataset["step_" + str(day_segment) + "_maxallsteps"] = resampledData['steps'].resample('D').max()

    if("minallsteps" in all_steps):
        finalDataset["step_" + str(day_segment) + "_minallsteps"] = resampledData['steps'].resample('D').min()

    if("avgallsteps" in all_steps):
        finalDataset["step_" + str(day_segment) + "_avgallsteps"] = resampledData['steps'].resample('D').mean()

    if("stdallsteps" in all_steps):
        finalDataset["step_" + str(day_segment) + "_stdallsteps"] = resampledData['steps'].resample('D').std()
    finaDataset.to_csv('data'+str(day_segment)+'.csv',index=True)

IS THERE A WAY TO MAKE THE 'IF' STATEMENTS AND ITS FUNCTIONS SHORTER? LOOP IT IN A DICTIONARY SOMEHOW?

There are more assignments like this in my code so I would want to know if there is a way I could just loop the aggregate functions

Try to get a good book or tutorial about pandas. The things you want to do is relatively easy with groupby.agg

for segment, segment_data in df.groupby("day_segment"):
    aggregated_data = segment_data.resample("D").agg(
        ["sum", "mean", "std", "min", "max"]
    )
    aggregated_data_renamed = aggregated_data.copy()
    aggregated_data_renamed.columns = aggregated_data.columns.droplevel(0).map(
        lambda x: f"step_{segment}_{x}allsteps"
    )

    filename = f"data{segment}.csv"
    aggregated_data_renamed.to_csv(filename, index=True)

This uses mean instead of avg. If this is a problem, you can add another rename function call somewhere


Java Multitasking (String concatenation) I would like to know if I did everything right and if there a way to do it more simple?

Task:
Task link to html file

My main:

package zad2;

public class Main {
  public static void main(String[] args) throws InterruptedException {
    StringTask task = new StringTask("A", 70000);
    System.out.println("Task " + task.getState());
    task.start();
    if (args.length > 0 && args[0].equals("abort")) {
    /*
       <- here add the code interrupting the task after a second
          and run it in a separate thread
    */
        Thread.sleep(500);
        task.abort();
        task.startNewTread();
    }
    while (!task.isDone()) {
      Thread.sleep(500);
      switch(task.getState()) {
        case RUNNING: System.out.print("R."); break;
        case ABORTED: System.out.println(" ... aborted."); break;
        case READY: System.out.println(" ... ready."); break;
        default: System.out.println("unknown state");
      }

    }
    System.out.println("Task " + task.getState());
    System.out.println(task.getResult().length());
  }
}

My StringTask:

package zad2;

public class StringTask implements Runnable {
    String line;
    String letter;
    int amount;
    boolean isAborted;
    TaskState stat;
    boolean wasAborted = false;

    public StringTask(String s, int i){
        line = "";
        letter = s;
        amount = i;
        stat = TaskState.CREATED;
        isAborted = false;
    }
    public StringTask(String s){
        this(s, 0);
    }
    @Override
    public void run() {
        while (amount > 0 && !isAborted){
            line += letter;
            amount--;
        }
        if(wasAborted == false) stat = TaskState.READY;
        else stat = TaskState.ABORTED;
    }

    public String getResult (){
        return line;
    }
    public TaskState getState (){
        return stat;
    }
    public void start (){
        isAborted = false;
        stat = TaskState.RUNNING;
        new Thread(this).start();
        //System.out.println("start");
        //run();
    }
    public void startNewTread (){
        isAborted = false;
        stat = TaskState.RUNNING;
        new Thread(this).start();
    }

    public void abort (){
        stat = TaskState.ABORTED;
        wasAborted = true;
        isAborted = true;
    }
    public boolean isDone(){
        if (stat == TaskState.READY || stat == TaskState.ABORTED) return true;
        return false;
        }
}

My TaskState:

package zad2;
public enum TaskState {
    CREATED, RUNNING, ABORTED, READY
}

JavaDoc, JavaDoc, JavaDoc.

package zad2;

That is not a good package name. It should ideally be:

package com.yourmailprovider.username.stringstuffy;

or

package com.yourwebsite.stringstruffy;

This makes it very easy to see from where the used classes are from.

task.start();
...
task.startNewTread();

Why are there two start methods?

public class StringTask implements Runnable

This is odd, your class implements Runnable but is not supposed to be used as a Runnable. Instead it has its own set of methods. I suggest that you do not implement Runnable and instead encapsulate the Runnable as private inner class.

String line;
String letter;
int amount;
boolean isAborted;
TaskState stat;
boolean wasAborted = false;

Why are all these variables package private and not private?

public StringTask(String s, int i)

These are very, very bad variable names. Here are two simple rules for variables naming:

Name a variable after what it contains or is doing.
You are only allowed to use one letter variable names for dimensions (x, y, z).

while (amount > 0 && !isAborted){
    line += letter;
    amount--;
}

What you want is a StringBuilder. A string concatenation is a very costly operation, everytime you concatenate a string a new String object has to be created (because String is immutable). The StringBuilder instead simply extends its internal buffer.

if(wasAborted == false) stat = TaskState.READY;
else stat = TaskState.ABORTED;

Oh dear, that is awful to read. I know that it looks more complicated, but you really should make a habit out of using braces for ifs:

if (!wasAborted) {
    stat = TaskState.READY;
} else {
    stat = TaskState.ABORTED;
}

public void start (){
    isAborted = false;
    stat = TaskState.RUNNING;
    new Thread(this).start();
    //System.out.println("start");
    //run();
}
public void startNewTread (){
    isAborted = false;
    stat = TaskState.RUNNING;
    new Thread(this).start();
}

Why are there two start methods again? They are even doing the same thing.

public boolean isDone(){
    if (stat == TaskState.READY || stat == TaskState.ABORTED) return true;
    return false;
    }

Now this is a prime example of confusing formatting.

CREATED, RUNNING, ABORTED, READY

What's the difference between READY and CREATED? Why do you care if the task has been aborted or not? Does not clean up its state at some point?

Why is there a separate boolean for if the task has been aborted?


Clone and replace attributes that are instances of a specific class For a given object that may be an instance of a class, a built-in container or type, I would like to copy that object, but replace all instances of a given class with something else.  I can do this with two calls to deepcopy as shown below, but there may be a more straightforward approach.

Create some complex nested object:

class T(object):
    pass

class A(object):
    def __init__(self):
        self.x = 10
        self.t1 = T()

class B(object):
    def __init__(self):
        self.a = A()
        self.y = 20
        self.t2 = T()

b = B()

Get a mapping from ids to objects:

visited = {}
copy.deepcopy(b, visited)

Create a custom dict type to replace instances of T with 100:

import collections

class Replacer(collections.MutableMapping):
    def __init__(self, idmap):
        self.store = dict()
        self.idmap = idmap
    def __getitem__(self, key):
        if isinstance(self.idmap[key], T):
            return 100
        return self.store[key]
    def __setitem__(self, key, value):
        self.store[key] = value
    def __delitem__(self, key):
        del self.store[key]
    def __iter__(self):
        return iter(self.store)
    def __len__(self):
        return len(self.store)

Copy and replace all T instances:

bb = copy.deepcopy(b, Replacer(visited))

In [121]: bb.t2
Out[121]: 100

In [122]: bb.a.t1
Out[122]: 100

You are overly complicating what you are trying to achieve by defining your Replacer class. You need two passes because your logic is:

store each id of custom objects into visited, associated to a copy of it;
change the value associated to ids of T objects into 100.

See how in 1. you already made a copy? You don't need that, you need to change how objects are copied in the first place.

The deepcopy function tries to copy a custom object x using one of 3 functions, in that order:

x.__deepcopy__;
x.__reduce_ex__(2);
x.__reduce__().

__reduce_ex__ and __reduce__ are a bit special and provided by object by default so better not to touch it. __deepcopy__, however, is copy.deepcopy own mechanism and should be used anytime you want to change how deepcopy should create new objects out of old ones.

If all you want to do is to turn all Ts into 100 when deepcopying an arbitrary object, then all you have to do is to implement T.__deepcopy__ in such a way:

class T(object):
    def __deepcopy__(self, memo): # expected signature for copy.deepcopy
        return 100

Every deepcopy will then turn each T into 100:

Ex1:

>>> b = [1,2,"hello",T(),('a',3,T()),T()]
>>> copy.deepcopy(b)
[1, 2, 'hello', 100, ('a', 3, 100), 100]

Ex2, using your A and B classes:

>>> b = B()
>>> bb = copy.deepcopy(b)
>>> bb.t2
100
>>> bb.a.t1
100

Note that in the context of your implementation and various calls, the T.__deepcopy__'s memo parameter would be an alias to visited, use it as you see fit if you want to implement custom logic. Read the last paragraph of the documentation for specific use cases.


Get all children recursively in Entity Framework Core In Entity Framework Core we can have recursive entities. But we cannot do an "Include" for these recursives (can we?). So I wrote a Get-method that does it for me:

First, this is my RecursiveEntity<T> base-class:

public class Entity : IEntity
{
    [Key]
    public int Id { get; set; }
}

public abstract class RecursiveEntity<TEntity> : Entity, IRecursiveEntity<TEntity>
    where TEntity : RecursiveEntity<TEntity>
{
    public virtual TEntity Parent { get; set; }
    public virtual ICollection<TEntity> Children { get; set; }
}

And this is my Repository-base-class for recurisve entities:

public abstract class RecursiveRepository<T, TDataContext>
    where T : RecursiveEntity<T> where TDataContext : DbContext
{
    protected IEnumerable<T> Get(Expression<Func<T, bool>> expression)
    {
        IQueryable<T> parents = DataContext.Set<T>()
            .Include(x => x.Children)
            .Where(w => w.Parent == null)
            .Where(expression);

        foreach (T entity in parents)
        {
            if (entity.Children != null && entity.Children.Any())
                entity.Children = _getChildren(entity, expression).ToList();

            yield return entity;
        }
    }

    private IEnumerable<T> _getChildren(T parentEntity, Expression<Func<T, bool>> expression)
    {
        IQueryable<T> children = DataContext.Set<T>()
            .Include(x => x.Parent)
            .Where(w => w.Parent != null && w.Parent.Id == parentEntity.Id)
            .Where(expression);

        foreach (T entity in children)
        {
            entity.Children = _getChildren(entity, expression).ToList();
            yield return entity;
        }
    }
}

Any suggestion what I can do better?
Is it a good solution to do that many DataContext<Set>-accesses and assign them  to the parent with ToList()?

If you remove virtual from Children it'll work with Include(e => e.Children):

  public abstract class RecursiveEntity<TEntity>
    : Entity, IRecursiveEntity<TEntity>
    where TEntity : RecursiveEntity<TEntity>
  {
    public virtual TEntity Parent { get; set; }
    public ICollection<TEntity> Children { get; set; }
  }

The meaning with virtual is to provide lazy loading. If you can't live with that, you'll have to do it manually, as you do.

Alternatively I think, you can add "MultipleActiveResultSets=True" to your connection string.

UPDATE

I seems that EF 6 and EFCore work differently when it comes to Include?

EFCore has the Include() and ThenInclude pattern but that is rather useless for recursive initialization.

When loading manually have you then experimented with the abilities to load navigation properties on each object as in:

public IEnumerable<TEntity> Get<TEntity>(Expression<Func<TEntity, bool>> filter) where TEntity: RecursiveEntity<TEntity>
{
  foreach (TEntity entity in Set<TEntity>().Where(e => e.Parent == null).Where(filter))
  {
    GetChildren(entity, filter);
    yield return entity;
  }
}

private void GetChildren<TEnity>(TEnity parent, Expression<Func<TEnity, bool>> childFilter) where TEnity : RecursiveEntity<TEnity>
{
  Entry(parent).Collection(e => e.Children).Query().Where(childFilter).Load();
  // Entry(parent).Reference(e => e.Parent).Load(); // I think this shouldn't be necessary because loading the children will load the parent on them

  if (parent.Children != null)
  {
    foreach (TEnity child in parent.Children)
    {
      GetChildren(child, childFilter);
    }
  }
}

It should produce the same result as yours, but is maybe a little clearer and in line with the EF concept.


Summary page with react-redux: container or presentational component I'm building a Summary page for merchant activity in a marketplace with react and redux.

The application has already a structure where there is a "MerchantContianer"

class MerchantContainer extends Component {
  render() {
    const merchList = [];
    for(let key in this.props.merchants) {
      for(let id in this.props.merchants[key].byId) {
        let row = this.props.merchants[key].byId[id];
        merchList.push((
          <Merchant
            type={key}
            key={id}
            date={row.date}
            name={row.name}
            amount={row.amount}
            delete={() => this.props.onDeleteRow(row.id, key)} />
      ));
    }
  };
    return (

        {merchList}

    )
  }
}

const mapStateToProps = state => {
  return {
      merchants: state.list.merchants
  };
};

const mapDispatchtoProps = dispatch => {
  return {
    onDeleteRow: (id, type) => {dispatch(actions.deleteMerchantsFromList(id, type))}
  };
};

export default connect(mapStateToProps, mapDispatchtoProps)(MerchantContainer);

and a Merchant component

const Merchant = props => {
  const classArr = [classes.Merchant, classes[props.type]].join(' ');
  return (

      {props.name}
      {props.date}
      {props.amount}
      <Button click={props.delete} btnType="Delete">Delete</Button>

  )
};

export default Merchant;

In addition there is a ReviewsContainer with the same structure of MerchantContainer but for a list of reviews (with a Review container with the same structure of Merchant).

Now I want to create a Summary component

const Summary = props => {
  return (

    <MerchantContainer />
    <ReviewsContainer />

 );
}

And use a Switch Component in App.js

<Switch>
        <Route path="/merchants" component={MerchantContainer} />
        <Route path="/reviews" component={ReviewsContainer} />
        <Route path="/summary" component={Summary} />
</Switch>

It is ok to use a "dumb" component for the Summary component? Or do I have to restructure so the Summary is the only container and MerchantContainer and ReviewsContainer must work with props from Summary?

One of the other features is a Dashboard component that will include the Summary and other components (is still not defined in the requirements for this task). So this dilemma will be repeated.

In my opinion (which also aligns with the official Redux FAQ ("Should I only connect my top component, or can I connect multiple components in my tree?"):

If MerchantContainer and ReviewContainer use mostly the same state or actions, you could reduce duplication by having a common container for both of those.
If MerchantContainer and ReviewContainer don't have any state in common (which I think is the case here), there's no real value in having Summary as a common container. Quite the oppositive: If you'd make Summary the common container, Summary would have access to state and actions that Summary doesn't care about at all.

To summarize: It's perfectly fine to have multiple connected components that might be children of a "dumb" component. If you have a similar setup in a couple of the children, you might consider extracting a common container.


Really simple immutable class I have a User entity. Each instance of the User class should have a first name, surname and email, and these shall never be assigned values aside from during their initialisation, since they should never change (in the context of my code).

public class User
{
    private string _firstName, _surname, _email;

    public string FirstName => _firstName;
    public string Surname => _surname;
    public string Email => _email;

    public User(string firstName, string surname, string email)
    {
        _firstName = firstName;
        _surname = surname;
        _email = email;
    }
}

As simple as it is, I'd like a quick review of the above since it's how I've started coding up all of my classes in favour of immutability (unless a particular class' property is subject to change).

For example, do you feel I should be using readonly instead? Or some other access modifier?

Also, are there any opinions on how I've declared multiple variables on one line (the privates)?

Make it simpler, you can use read-only properties:

public class User
{
    public string FirstName { get; }
    public string Surname { get; }
    public string Email { get; }

    public User(string firstName, string surname, string email)
    {
        FirstName = firstName;
        Surname = surname;
        Email = email;
    }
}

Now let's consider to make it closed, I do not see any extension point then if you don't have any other requirement against this you should also mark it as not inheritable:

public sealed class User

What next? Make it safer. Your class and its ctor are public then you should validate parameters:

public User(string firstName, string surname, string email)
{
    if (firstName == null)
        throw new ArgumentNullException(nameof(firstName));

    if (surname == null)
        throw new ArgumentNullException(nameof(surname));

    if (email == null)
        throw new ArgumentNullException(nameof(email));

    FirstName = firstName;
    Surname = surname;
    Email = email;
}

If you're using C#7 then consider to use throw exceptions (see pgerchev's comment and t3schb0t's answer).

Class has not to be public? Remove public from its declaration. Constructor is not intended to be called outside your own assembly but class has to be public? Mark ctor as internal and assert  about its arguments.

Note about Debug.Assert: obviously this does not mean you won't need to validate somewhere these arguments (see also comments) when building in release mode, just that here you assume they're right. See also Exception Vs Assertion, consider pre/post-conditions not an alternative to validation but - sort of - run-time unit testing:

internal User(string firstName, string surname, string email)
{
    Debug.Assert(firstName != null);
    Debug.Assert(surname != null);
    Debug.Assert(email != null);

    FirstName = firstName;
    Surname = surname;
    Email = email;
}

Do you need any other validation (email has not to be empty and it has to be a valid e-mail address, for example)? Add them all there. Be careful with people's name validation (see David's linked post).

Anything else? Make it easier to debug:

[DebuggerDisplay("{FirstName} {Surname} ({Email})")]
public sealed class User

Last note: yes, there is an [ImmutableObject] attribute you may use to decorate your class but AFAIK it's used only by PropertyGrid control. You may add it as reminder (for you or for future readers) that class has to be immutable but nothing else.


Optimize JQuery/JavaScript Link Checker Below is a script I've written that uses AJAX to check if all links on the page are alive or dead. Could this be modified to be more efficient in any way? Thank you very much.

$(document).ready(function() {
    var liveLinks = [];
    var deadLinks = [];

    function updateLive() {
        $live = Number($('#liveLinkCount').text()) + 1;
        $('#liveLinkCount').text($live);
    }

    function updateDead() {
        $dead = Number($('#deadLinkCount').text()) + 1;
        $('#deadLinkCount').text($dead);
    }

    $('body').prepend("
Hide<button id='checkLinks'>Check Links</button>
Live: 0 - Dead: 0
");
    $('#linkChecker').css({
        "width": "130px",
        "border": "1px solid black",
        "text-align": "center"
    });
    $('#hideChecker').css({
        "text-decoration": "underline",
        "float": "right",
        "padding": "1px",
        "cursor": "pointer"
    });
    $('body').on('click', '#checkLinks', function() {
        $('#linkChecker').append("
<button id='logReport'>Log Report</button>");
        $('a').each(function(i) {
            var self;
            self = this;
            $.ajax({
                type: "post",
                url: $(self).prop('href'),
                success: function() {
                    liveLinks.push($(self).prop('href'));
                    updateLive();
                    $(self).css({
                        "color": "green"
                    });
                },
                error: function() {
                    deadLinks.push($(self).prop('href'));
                    updateDead();
                    $(self).css({
                        "font-weight": "bold",
                        "color": "red"
                    });
                }
            });
        });
    });
    $('body').on('click', '#logReport', function() {
        console.log(new Array(24 + 1).join('\n'));
        console.log("Live Links:");
        for (var i in liveLinks) {
            console.log("  " + liveLinks[i]);
        }
        console.log("Dead Links:");
        for (var i in deadLinks) {
            console.log("  " + deadLinks[i]);
        }
    });
    $('body').on('click', '#hideChecker', function() {
        $('#linkChecker').fadeOut();
    });
});

Firstly, for...in is used to enumerate over object properties, not to iterate through arrays.  You should use a traditional for loop for this.

Secondly, you could probably refine your console logging portion a bit. It may not have any impact to application performance, but it would make your console look prettier.

$('body').on('click', '#logReport', function() {
    var i, j, k, l;

    console.group("Live Links:");
    for (i=0, j=liveLinks.length; i<j; i++) {
        console.log(liveLinks[i]);
    }
    console.groupEnd();
    console.group("Dead Links:");
    for (k=0, l=deadLinks.length; k<l; k++) {
        console.log(deadLinks[k]);
    }
    console.groupEnd();
});

As far as I know, this works for both Google Chrome and Mozilla Firefox (FireBug).  See here for more information.


Creating map from ids I am getting List ids and by using those ids I am creating a Map by fetching related data of id from database, here I am checking prevId with currId if these are not equal then I am putting them into oCMap.

Can this code be optimized?  Or are there any improvements (like is there any better algorithm)?  Please offer suggestions.

  public void populateMap(List<Number> ids)
  {
    Object oName, cName, startDate, endDate;
    String header = "
";
    String footer = "
";
    String rowStart = "
";
    String rowEnd = "
";
    String columnStart = "&nbsp;";
    String columnEnd = "	";
    SimpleDateFormat displayDateFormat = new SimpleDateFormat ("dd/MM/yyyy");
    String convertedDateString;
    Number prevId = null;
    Number currId = null;
    StringBuilder sBuild = new StringBuilder();
    StringBuilder sbLoop = new StringBuilder();

    for(int j = 0; j < ids.length; j++)
    {

      Row row = getRow(ids[j]);
      currId  = (Number)row.getAttribute("OwnerId");
      oName   = row.getAttribute("oName");
      cName = row.getAttribute("ContracttypeName");
      startDate    = row.getAttribute("StartDate");
      endDate      = row.getAttribute("EndDate");

      if(prevId != null && !currId.equals(prevId))
      {
        if(sbLoop.length() > 0)
        {
          sBuild.append(header);
          sBuild.append(sbLoop);
          sBuild.append(footer);
        }
        _oCMap.put(prevId, sBuild); //HashMap declared at class level

        sBuild = new StringBuilder();
        sbLoop = new StringBuilder();
      }

      sbLoop.append(rowStart);
      if(oName != null)
      {
        sbLoop.append(columnStart + oName + columnEnd);
      }
      else
      {
        sbLoop.append(columnStart + columnEnd);
      }
      sbLoop.append(columnStart + cName + columnEnd);
      if(startDate != null)
      {
        convertedDateString = displayDateFormat.format(((Date)startDate).dateValue());
        sbLoop.append(columnStart + convertedDateString + columnEnd);
      }
      else
      {
        sbLoop.append(columnStart + columnEnd);
      }
      if(startDate != null && endDate != null)
      {
        sbLoop.append(columnStart + " t/m" + columnEnd);
      }
      else
      {
        sbLoop.append(columnStart + columnEnd);
      }
      if(endDate != null)
      {
        convertedDateString = displayDateFormat.format(((Date)endDate).dateValue());
        sbLoop.append(columnStart + convertedDateString + columnEnd);
      }
      else
      {
        sbLoop.append(columnStart + columnEnd);
      }
      sbLoop.append(rowEnd);
      prevId = currId;

      if(j ==  ownerContract.length - 1)  //code for putting into map for last id.
      {
        if(sbLoop.length() > 0)
        {
          sBuild.append(header);
          sBuild.append(sbLoop);
          sBuild.append(footer);
        }
        _oCMap.put(prevId, sBuild); //HashMap declared at class level
      }
    }
  }

Indentation
Java's indentation convention is 4 spaces, and not 2 as in your code. You may choose the align = signs between grouped lines, but if you do - be consistent - in your assignment block there are 3 different indentations...

Variables, constants and literals
Variables, which are dynamic and may change as the method progresses, should be named in camelCase, and should be declared as close to their use as possible (no need to declare them at the beginning of the method).
Constants, which do not change when the code is run, should be declared as constants outside the method, and be named in ALL_CAPS.
Personally I think that you should consider dropping the constants altogether, and use literals, since they are less ambiguous then using constants in this case:

sBuild.append("
");
sBuild.append(sbLoop);
sBuild.append("
");

reads better than:

sBuild.append(HEADER);
sBuild.append(sbLoop);
sBuild.append(FOOTER);

which reads better than:

sBuild.append(header);
sBuild.append(sbLoop);
sBuild.append(footer);

Make operations close to where they are used
Same as variable declaration, it is better to calculate a variable value close to where it is used:

Object oName = row.getAttribute("oName");
if(oName != null)
{
    sbLoop.append(columnStart + oName + columnEnd);
}
else
{
    sbLoop.append(columnStart + columnEnd);
}

Naming
You should give variable names which convey information about their purpose and usage. oName and cName are borderline, if they are part of the domain's nomenclature, but sBuild and sbLoop can definitely be improved.

Proper usage of StringBuilder
So, you've decided to use StringBuilder to optimize string concatenations. That's good, but then you use + to concatenate strings before you append them to your StringBuilder! #append method returns this exactly so you can avoid this:

sbLoop.append(columnStart).append(oName).append(columnEnd);

You use sBuild only to wrap your sbLoop with a header and a footer, I suggest you consider simply prepend sbLoop with the prefix, and append it the suffix without using a temp object:

if(prevId != null && !currId.equals(prevId))
{
    if(sbLoop.length() > 0)
    {
        sbLoop.insert(0, header);
        sbLoop.append(footer);
    }
    _oCMap.put(prevId, sbLoop); //HashMap declared at class level

    sbLoop = new StringBuilder();
}

I also think that you should consider not save the StringBuilder in your map, but rather a toString() of it.

Less boilerplate
Your code has a repeating pattern which is something like this:

if(something != null)
{
    sbLoop.append(columnStart + something + columnEnd);
}
else
{
    sbLoop.append(columnStart + columnEnd);
}

I think a better pattern might be:

sbLoop.append(columnStart);
if(something != null)
{
    sbLoop.append(something);
}
sbLoop.append(columnEnd);


Should I split class or keep it all in one? Currently, in my project I have one class that is dedicated to the all of the queries for the IBM i and converts it into usable models for my .NET code. There is already 12 methods in this class and there will be many, many more to come. This is for a tool that pulls data from our IBM i and pushes it to our internet database server.

Should I split this class up? Should I make it a partial class and put the code across multiple files? Should I do multiple classes?

Just wondering what the best practice is on something like that.

Currently this is the framework I have in place

class IbmIDatabase
{
    private DateTime ZERO_DATE = new DateTime(1900, 1, 1, 0, 0, 0);
    private string _connString = String.Empty;
    SystemCodeRepository scr = new SystemCodeRepository();

    /// <summary>
    /// Initializes a new instance of the <see cref="IbmIDatabase"/> class.
    /// </summary>
    public IbmIDatabase()
    {
    }

    #region System Codes
    /// <summary>
    /// Gets all system codes.
    /// </summary>
    /// <returns></returns>
    public IEnumerable<SystemCode> GetAllSystemCodes()
    {
    }
    #endregion System Codes

    #region Citations
    /// <summary>
    /// Gets all citations.
    /// </summary>
    /// <returns></returns>
    public IEnumerable<ParkingTicket> GetAllCitations()
    {
    }
    #endregion Citations

    #region Queue
    /// <summary>
    /// Gets the queued records.
    /// </summary>
    /// <returns></returns>
    public IEnumerable<QueuedRecord> GetQueuedRecords()
    {
    }

    /// <summary>
    /// Marks the queue as processed.
    /// </summary>
    /// <param name="id">The id.</param>
    public void MarkQueueAsProcessed(int id)
    {
    #endregion Queue

    #region Utility Bill Customer
    /// <summary>
    /// Gets the utility bill customer.
    /// </summary>
    /// <param name="id">The customer id.</param>
    /// <returns></returns>
    public Customer GetUtilityBillCustomer(int id)
    {
    }

    /// <summary>
    /// Gets the utility bill customer.
    /// </summary>
    /// <returns></returns>
    public IQueryable<Customer> GetUtilityBillCustomer()
    {
    }
    #endregion

    #region Utility Bill Details
    /// <summary>
    /// Gets the customer history.
    /// </summary>
    /// <param name="id">CustomerId</param>
    /// <returns></returns>
    public IQueryable<BillHistory> GetCustomerHistory(int id)
    {
    }

    /// <summary>
    /// Gets the customer history.
    /// </summary>
    /// <returns></returns>
    public IQueryable<BillHistory> GetCustomerHistory()
    {
    }
    #endregion

    #region Utility Bill Payment Details
    /// <summary>
    /// Gets the customer payment history.
    /// </summary>
    /// <param name="id">The id.</param>
    /// <returns></returns>
    public IQueryable<BillPaymentHistory> GetCustomerPaymentHistory(int id)
    {
    }

    /// <summary>
    /// Gets the customer payment history.
    /// </summary>
    /// <returns></returns>
    public IQueryable<BillPaymentHistory> GetCustomerPaymentHistory()
    {
    }
    #endregion

    #region Utility Bill Summary
    /// <summary>
    /// Gets the customer summary.
    /// </summary>
    /// <param name="id">CustomerId</param>
    /// <returns></returns>
    public IEnumerable<BillSummary> GetCustomerSummary(int id)
    {
    }

    /// <summary>
    /// Gets the customer summary.
    /// </summary>
    /// <returns></returns>
    public IQueryable<BillSummary> GetCustomerSummary()
    {
    }
    #endregion
}

Within each method can be quite a few lines of code (up to many 100 or so?). I left one of the longest ones so far in the sample code.

This class has a well defined role and by keeping it intact you are satisfying the Single Responsibility Principle. So, I would leave it like that. Perhaps you might want to look into how you could refactor some common code out of each method and into a separate utility class, but that's all.


Checking an array is sorted in either order efficiently Here is my code, it works fine, just wondering if there any anything to make it even better.

   public static boolean isSorted(int[] arr)
   {
      int n = arr.length;
      int i = 0;
      while (arr[i] == arr[i + 1])  //  same elements at the beginning
         i++;

      if (arr[i] < arr[i + 1])      //  candidate for ascending, non-decreasing
      {
         i++;

         for (; i < n - 1; i++)
         {
            if (arr[i] > arr[i + 1])
               return false;
         }

         return true;
      }

      else                          //  candidate for descending, non-increasing
      {
         i++;

         for (; i < n - 1; i++)
         {
            if (arr[i] < arr[i + 1])
               return false;
         }

         return true;
      }
   }

It works fine, I have just avoided any class or other method.

We could make it better by making it more maintainable by simplifying the code and reducing duplication in the three for-loops. The problem is essentially checking that all successive pairs in the array are either equal or have the same sort order, so let's code it that way:

import static java.lang.Integer.compare;
import static java.lang.Integer.signum;

public static boolean isSorted(final int[] arr) {
    // The order in which the array is.
    // -1: ascending
    //  0: all elements are the same
    //  1: descending
    int arrayOrder = 0;

    for (int i = 0; i < arr.length - 1; i++) {
        // Signum is actually useless, since compare returns -1..1 but
        // we believe the documentation, not the code.
        final int pairOrder = signum(compare(arr[i], arr[i + 1]));

        if (arrayOrder == 0) {
            // All elements so far have been equal. First non-equal pair
            // defines the order expected from the following pairs..
            arrayOrder = pairOrder;
        } else if (pairOrder != arrayOrder && pairOrder != 0) {
            // If any pair is not equal and deviates from array order,
            // the array is not sorted.
            return false;
        }
    }

    return true;
}

Is this more efficient? No. There are more operations inside the for-loop, but we're talking about two equals checks between integers in an O(N) algorithm. If yours takes 4.7s on my laptop, mine takes 5.4s... for 2^31-1 elements.

A benefit in a single for-loop is that we don't need to expose the loop counter outside the loop itself. Code becomes harder to follow and more error prone when the loop counter is modified outside it's logical scope.


Music collection with lyric songs and instrumental pieces I have problems with dealing with derived classes that inherit from the same base class.

I have 2 types of songs:

Songs with lyrics that have: Title, Tags (sad, happy etc.), lyrics (one line), writer.
Instrumental song: Title, Tags (same as lyric songs), instrumentals (a string), performer, BPM.

I thought it would be a good idea to implement a base class Song, since Title and Tags are fields that are common to the both types of the songs. Then, implement the class LyricSong and InstrumentalSong that inherit from Song.

class Song
{

public:
    Song(std::string title, std::string tags);

    std::string getTitle() const { return this->_title; }
    std::string getTags() const { return this->_tags; }

    // Since i had the need in virtual functions and since all virtual functions have to be
    // implemented, i used default string and int to be returned.
    virtual std::string getLyrics() const { return std::string(); }
    virtual std::string getLyricsBy() const { return std::string(); }
    virtual std::string getInstruments() const { return std::string(); }
    virtual std::string getPerformer() const { return std::string(); }
    virtual int getBPM() const { return 0; }

    static songContainer readSongsFromFile(std::string songsFileName);

private:
    std::string _title;
    std::string _tags;

};

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

class LyricSong : public Song
{

public:
    LyricSong(std::string title, std::string tags, std::string lyrics, std::string lyricsBy);

    std::string getLyrics() const { return this->_lyrics; }
    std::string getLyricsBy() const { return this->_lyricsBy; }

private:
    std::string _lyrics;
    std::string _lyricsBy;
};

//---------------------------------------------------------------------------//
class InstrumentalSong : public Song
{

public:
    InstrumentalSong(std::string title, std::string tags,\
                     std::string instruments, std::string performer, int BPM);

    std::string getInstruments() const { return this->_instruments; }
    std::string getPerformer() const { return this->_performer; }
    int getBPM() const { return this->_BPM; }

private:
    std::string _instruments;
    std::string _performer;
    int _BPM;
};

Issues that have arisen:

The code is cumbersome and messy.
Every time I'll need to add new song types, it will force me to modify Song (add more functions and make them virtual - Polymorphic issues).
Will there be a way to know what type of Song I work with, in case of iterating over an array of Song pointer that point to various song types?

Are there any common design principles that can help me with this?

Polymorphic classes must include a virtual destructor. Otherwise they are not truly polymorphic: you cannot delete such an instance using the pointer to the base class.

A class hierarchy should (typically) be designed according to the Liskov Substitution Principle. This means that any derived class must be substitutable for the base class: anywhere a base class is expected to work, the derived class must as well. Your hierarchy conforms to this, but I wanted to point it out.

  Will there be a way to know what type of Song I work with, in case of iterating over an array of Song pointer that point to various song types?

That's what dynamic_cast is for - it returns nullptr when the object is not of the desired type.

Song * song = ...;
auto isong = dynamic_cast<InstrumentalSong*>(song);
if (isong) {
  /* it's instrumental */
  ...
}
auto lsong = dynamic_cast<LyricSong*>(song);
if (lsong) {
  /* it's lyric */
  ...
}

The type of isong is deduced to InstrumentalSong*, etc.

A basic question to be asked is: do you even care about a class structure? What do you do with all that song data?


Determining if a word is an anagram of another The question was to see if a word is an anagram of another word.

How would you improve this? It returns True if an anagram, or False otherwise.

# write the function is_anagram
def is_anagram(test, original):
    if len(test) != len(original):
        return False
    for letter in test.lower():
        if letter not in original.lower():
            return False
    for letter in original.lower():
        if letter not in test.lower():
            return False
    return True

You need to check if each letter occurs the name number of times in both strings.

One method would be to sort the letters and compare the lists of letters for equality.

Here is my approach:

def is_anagram(test, original):
  return sorted(list(test.lower())) == sorted(list(original.lower()))

This algorithm is far from optimal runtime O(n*logn), but I have chosen it because of its simplicity.
For an Efficient algorithm see the comment of @corsiKa for a quite simple O(n) algorithm.


Control Signin Class - Remove Dependencies <?php

/**
 *      Module  :       Control
 *      Name    :       ControlSigin
 *      Input   :       Text Array
 *      Output  :       Pass or Fail(w/responseText)
 *      Notes   :
 *      Call    :
                        $instanceControlSignIn = new ControlSignIn();
                        $instanceControlSiginIn->invoke();

 */

class ControlSignIn extends Post
{
    private $databaseObject;
    private $textObject;
    private $messageObject;

    public function __construct()
    {
        parent::__construct();

        $this->databaseObject = new Database();
        $this->textObject = new Text($this->_postProtected);
        $this->messageObject = new Message();
    }

    public function invoke()
    {
        if(Constant::VALIDATE_ON==true)
        {
            $continue = $this->checkInput();
        }
        else
        {
            $continue = true;
        }

        if($continue==true)
        {
            if($this->validatePass())
            {
                $this->setSessionVariables();
                Control::send(Mark::pass);
            }
            else
            {
                $this->messageObject->display('validate');
                return false;
            }
        }
    }

    private function checkInput()
    {
        if(!$this->textObject->checkEmpty())
        {
            $this->messageObject->display('empty');
            return true;
        }
        if(!$this->textObject->checkPattern('email'))
        {
            $this->messageObject->display('email');
            return false;
        }
        if(!$this->textObject->checkPattern('pass'))
        {
            $this->messageObject->display('pass');
            return false;
        }
    return true;
    }

    private function validatePass()
    {
        $result=$this->databaseObject->_pdoQuery('single', 'signin_pass',array($this->_postProtected['email']));
        $pass=crypt($this->_postProtected['pass'], $result['pass']);
        $result=$this->databaseObject->_pdoQuery('single', 'signin_validate',array($this->_postProtected['email'], $pass));
        return (count($result)==2) ? true : false;
    }

    private function setSessionVariables()
    {
        $result=$this->databaseObject->_pdoQuery('single', 'pull_session',array($this->_postProtected['email']));
        Session::activate($result['picture'],$result['id'],$result['name'],'bookmarks');
    }
}

Without a question there's no way to give a definitive answer.  Still, here's a few comments you might want to take on board.

You're instantizing your dependencies in your constructor.  This means that your class is now irrevocably coupled to those dependencies.  This makes testing and reuse very difficult, as it's basically impossible to test your class in isolation and if you move the class over to another project then you'll have to move all the dependencies over as well.  You should use dependency injection instead and pass your dependencies to the constructor instead.
It looks like you're statically invoking some static methods as well.  You should avoid doing this for the same reasons that you should inject dependencies.  A static dependency is still a dependency, and your class will now be coupled to those classes whose static methods you're using.  Furthermore, static methods tend to be frowned upon as they represent global state.  For reasons I'm sure you're familiar with, global state is a Bad Thing.  You'll be better off passing in instances of objects instead.
Your header comment is non-standard.  Good to see you're including comments at all, but there are various automated tools out there (DocBlox for example) that are capable of generating documentation files for you, but only if your comments are in the right format.  If you look at the documentation for those tools they'll give you a rundown of how to format a comment and what tags are available to you.  IDEs like Netbeans and Eclipse can also use the docblocks to provide you with pop-up help, they'll show your comment when you type the class name so you'll be reminded of what it does.
Aside from your header comment (which is good), you don't comment your code at all.  You should at the very least put a docblock comment at the start of all your methods to tell you what they do so that they can be documented by tools like DocBlox and so that you'll get help on your class from your IDE.  You should also provide a running commentary in plain-English comments as to what's going on in your code.

One last tip. You can abbreviate the code at the start of your invoke() method as follows:

    $continue = Constant::VALIDATE_ON==true?
        $this->checkInput():
        true;


Rolling 100-sided "dice" until they are the same number I've just begun my journey into python (1 week now) and I've written couple of lines incorporating everything I've learned so far. It would be greatly appreciated if anyone could guide me in writing a better formatted (appropriate style) version of this.

I've just written a couple of lines on finding two random rolls that stop when they roll the same number. When the same number is rolled, that number is recorded along with how many rolls it took to get there. I've also made the code to find those values and store them in a list, finding the average of roll number(same number) as well as roll count(How many rolls it took). Here is the code:

import random
totalrolls = 0
rollList = []
rollcountlist = []
while totalrolls < 500:
    totalrolls = totalrolls + 1
    rollcount = 0
    roll = random.randint(1,100)
    rolltwo = random.randint(1,100)
    while roll != rolltwo:
        roll = random.randint(1,100)
        rolltwo = random.randint(1,100)
        #print roll
        #print rolltwo
        rollcount = rollcount + 1
    if roll == rolltwo:
        rollList.append(roll)
        rollcountlist.append(rollcount)
        print "Random number that matched was: %s, after %s rolls. " % (roll, rollcount)
if totalrolls == 500:
    print "Finished rolling %s times." % totalrolls

averageroll = sum(rollList) / len(rollList)
averagerollcount = sum(rollcountlist) / len(rollcountlist)

print rollList
print rollcountlist
print averageroll
print averagerollcount

You have an off-by-one error.  If you get lucky and get a matching pair immediately, then you would report "Random number that matched was: …, after 0 rolls."  I would say that "after 1 roll(s)" would be the right way to report that.  (Alternatively, you could phrase it as "after 0 mismatched rolls".)

Naming could be improved slightly.  I suggest:

rollcountlist → roll_counts, because it's shorter and more readable.  Since englishisnotgerman, you should use an underscore, as recommended by PEP 8.
rollList → roll_values, because "values" makes it clearer what is being stored.  Watch your capitalization consistency.
averageroll → average_value and averagerollcount → average_count, for similar reasons.
totalrolls → experiment_count, because totalrolls sounds like it represents a cumulative sum of rollcount, which it isn't.  Introducing the term "experiment" makes it clear that you are counting something else.
roll → roll1 and rolltwo → roll2, for consistency.  Or, as I've done below, make one two-element tuple called rolls.

The if conditions after the while loops are superfluous.  The inner loop can only be exited when the roll values match.  The outer loop can only be exited after 500 experiments.

Counting loops can be more elegantly expressed.  For a fixed number of iterations, such as for the outer loop, use the idiom for i in xrange(n): ….  For an unbounded count, such as for the inner loop, use for i in itertools.count().

In Python 2, the / operator performs integer division (rounding down) when dividing an integer by an integer.  For a more accurate mean, convert at least one of the operands to a float first.

When interpolating an integer into a format string, use the %d placeholder instead of %s for clarity.

from itertools import count
import random

roll_values = []
roll_counts = []
for experiment_count in xrange(1, 501):
    for roll_count in count(1):
        rolls = (random.randint(1,100), random.randint(1, 100))
        if rolls[0] == rolls[1]:
            break
    roll_values.append(rolls[0])
    roll_counts.append(roll_count)
    print "Random number that matched was: %d, after %d rolls." % (rolls[0], roll_count)

average_value = float(sum(roll_values)) / len(roll_values)
average_count = float(sum(roll_counts)) / len(roll_counts)

print "Finished rolling %d times." % experiment_count
print roll_values
print roll_counts
print average_value
print average_count


Forming combinations of a 16 bit binary number I wrote this code to form combinations of a 16 bit binary number (I know it is in output is string format, but it is my goal to just print all possible 16 bit binary numbers). My question is, can this code be optimized? How can I calculate the time complexity of this code?

public class Combinations {

    static int len;

    public static void formCombinations(StringBuilder start,String[] possible)
    {
        if(start.length()==len)
        {
            System.out.println(start);
            return;
        }
        for(int i=0;i<possible.length;i++)
        {
            formCombinations(start.append(possible[i]), possible);
            start.deleteCharAt(start.length()-1);
        }
    }

    public static void formCombinations(int len,String[] a) {
        Combinations.len=len;
        formCombinations(new StringBuilder(""), a);
    }

    public static void main(String[] args) {
        String a[]={"0","1"};
        formCombinations(16, a);
    }
}

Give your operators some breathing space, e.g.:

for (int i = 0; i < possible.length; i++)

possible is not very good name (it implies that something is impossible. I recommend alphabet instead.
possible never changes. It makes it a good candidate for a final class member. Passing it down recursively wastes some stack space.
I don't think overloading formCombinations is warranted.
I see nothing obviously wrong performance wise. After all, you generate all permutations, and you generate each permutation exactly once. Of course, StringBuilder may impose some hidden penalties; Java experts would give an authoritative answer.

In any case, an iterative solution requires less space, doesn't need a StringBuilder, and may serve as an iterator (in a sense of supporting has_next() and next() methods). I highly recommend implementing it: it is simple and instructive. Hint: think of next permutation as adding 1 to a binary number.


Elevator Interview Problem OOP - Revised Elevator program code challenge, revised:

Can someone please critique my Elevator problem - I wanted to use OOP principles and coding standards. Also, does logic make sense?

internal enum Status
{
    GoingUp,
    GoingDown,
    Stopped
}

internal class Elevator
{
    private readonly SortedList<int, bool> _sorted = new SortedList<int, bool>();
    public readonly int TopFloor;

    public Elevator(int topFloor)
    {
        TopFloor = topFloor;
    }

    public int CurrentFloor { get; set; } = 1;
    public Status Status { get; set; } = Status.Stopped;

    public void AddRequest(int floor)
    {
        if (!_sorted.ContainsKey(floor))
            _sorted.Add(floor, false);

        switch (Status) {
            case Status.GoingDown:
                MoveDown();
                break;

            case Status.Stopped:
                if (CurrentFloor < floor)
                    MoveUp();
                else
                    MoveDown();
                break;

            case Status.GoingUp:
                MoveUp();
                break;

            default:
                break;
        }
    }

    public void MoveUp()
    {
        var max = _sorted.OrderByDescending(x => x.Key).First();
        for (var i = CurrentFloor; i <= max.Key; i++) // Go to top most requested floor
            if (_sorted.ContainsKey(i))
                Stop(i);
            else
                continue;

        Status = Status.Stopped;
    }

    public void MoveDown()
    {
        var min = _sorted.OrderBy(x => x.Key).First();
        for (var i = CurrentFloor; i >= min.Key; i--)
            if (_sorted.ContainsKey(i))
                Stop(i);
            else
                continue;

        Status = Status.Stopped;
    }

    public void Stop(int floor)
    {
        _sorted.Remove(floor);
        CurrentFloor = floor;
        Console.WriteLine("Stopped at: {0}", CurrentFloor);
    }
}

internal class Manager
{
    private readonly Elevator _elevator = new Elevator(10);

    public void ButtonPressed(int floor)
    {
         // testing...
        _elevator.AddRequest(10);
        _elevator.AddRequest(10);
        _elevator.AddRequest(5);
        _elevator.AddRequest(2);
        _elevator.AddRequest(9);
        _elevator.AddRequest(6);
    }
}

Elevator program code challenge, revised:

where is that?

_sorted is a very very bad name. I have to read all the code to figure out exactly what that is.

public void AddRequest(int floor)

Rename the parameter to something like requestedFloor
Rename the method to something like CallElevator or RequestFloor

Does not account for which floor a call is coming from. I think it would matter if the elevator passing 2nd floor for the 7th, then a request comes for the 5th - originating from the 1st floor.

Looks like the code throws away requests for floors already in the list. But if I'm on the 1st floor asking for the 5th, why ignore my request just because the elevator is carrying someone to 5 already?

Fuzzy code

It is hard to tell exactly what the elevator logic is. And harder to tell if it meets requirements.

var min = _sorted.OrderBy(x => x.Key).First();

Clever but not clear. What is the elevator doing? What is the floor-stop choosing logic? This is "the bowels of the code" exposed at the highest logic level. I should be reading what the elevator is doing, not googling MSDN.

public int CurrentFloor { get; set; } = 1;
public Status Status { get; set; } = Status.Stopped;

Why are these public? In principle a well made OO class hides state and exposes functionality.

case Status.Stopped:
    if (CurrentFloor < floor)
        MoveUp();
    else
        MoveDown();
    break;

Flawed logic. If the floor (requested floor?) is the same as the current floor, the elevator goes down. Even if this code works is not the point. I should not have to use a debugger to know if the code covers edge cases.

P.S. what happens when we're on the 1st floor? Does it still go down?

Two collections makes sense: a queue of floor requests and an ordered list of floors the elevator is traversing. This fundamentally decouples two things that should not be conflated, but are so in this code.

Edit

Decisions on fundamental data structures strongly influences higher level design. In this case I think floor decision/prioritzation functionality would more likely end up in it's own class(es) - decoupling the decision logic from the elevator's movement.

This decoupling would make it easier to fix any issues due to incomplete/vague requirements. With the current code, things would break and change everywhere.

end Edit


Performance of summing one value from join of three tables I have a table I want to get a sum of information on, but the value I need to query on is in two different tables.

The one with values I need to sum is say, kudos, which has a value indicating +1 (1) or -1 (2) in the Action column, which is associated to the discussion it's on, or the comment it's on:

Kudos
+-----------+--------------+--------+
| CommentID | DiscussionID | Action |
+-----------+--------------+--------+
|        24 |         NULL |      1 |
|      NULL |            4 |      1 |
|        60 |         NULL |      2 |
+-----------+--------------+--------+

  The value I want to filter by is the author's ID, which is either in
  the Comment table or the Discussion table:

Comments
+-----------+--------+
| CommentID | Author |
+-----------+--------+
|        29 |      5 |
|        24 |      1 |
|        22 |      1 |
|        21 |      1 |
+-----------+--------+

Discussions
+--------------+--------+
| DiscussionID | Author |
+--------------+--------+
|            4 |      1 |
|            5 |      2 |
|            6 |      2 |
|            7 |      7 |
+--------------+--------+

In this example, Author 1 would have a score of 2, because row 1 in the Kudos table would count because it matches with row 1 in the comment table, and row 2 in the Kudos table matches the 1st row in the Discussions table.

Additionally, I need to use the query that does this as a subquery, because I'm getting all the users in a select statement, and then getting the sum of this subquery as a field associated with the userID, like:

SELECT tblUser.Name AS Name, (subquery) AS rep FROM User_Table AS tblUser;

What I've come up with is the following:

SELECT tblU.Name AS Name, tblU.UserID AS ID,
    (SELECT SUM(IF(Action=1, 1, -1)) FROM Kudos AS tblK
    LEFT JOIN Discussions AS tblD ON tblK.DiscussionID = tblD.DiscussionID
    LEFT JOIN Comments AS tblC ON tblC.CommentID = tblK.CommentID
    WHERE tblD.InsertUserID = id OR tblC.InsertUserID = id) AS rep
FROM Users AS tblU;

which gives me:

+--------------+----+------+
| Name         | id | rep  |
+--------------+----+------+
| Jack         |  1 |  357 |
| Joe          |  2 |  824 |
| Jim          | 12 |   48 |

But it takes 0.23 seconds to run for a small set (like 13 users). I've heard from co-workers that the OR condition is bad for performance, but I'm not sure how this can be optimized at all.

Avoid LEFT JOIN clauses

I'm not sure that I'd pick the OR as the problem.  The real problem is more likely to be the LEFT JOIN clauses.  A LEFT JOIN is a slow operation and to be avoided if you aren't looking for NULL values.

Try using two subselects instead:

SELECT tblU.Name AS Name, tblU.UserID AS ID,
  (SELECT SUM(IF(Action=1, 1, -1))
   FROM Kudos AS tblK
     INNER JOIN Discussions AS tblD ON tblK.DiscussionID = tblD.DiscussionID
   WHERE tblD.InsertUserID = tblU.UserID) +
  (SELECT SUM(IF(Action=1, 1, -1))
   FROM Kudos AS tblK
     INNER JOIN Comments AS tblC ON tblC.CommentID = tblK.CommentID
   WHERE tblC.InsertUserID = tblU.UserID ) AS rep
FROM Users AS tblU;

Note that this only works if Kudos never has a non-null CommentID in a row with a non-null DiscussionID.  This also has the side effect of getting rid of the OR, but I'm not sure that's what matters.

Use indexes

Another thing that you can do is add indexes on the relevant columns if you don't already have them.  Hopefully you already have PRIMARY KEY indexes on the ID column for each table.  If that's not enough, it is possible to add indexes on things like UserID, Name and DiscussionID, InsertUserID, Action.  These can move critical columns into memory.

Consider merging Comments and Discussions

The Comments and Discussions tables use the same columns here.  Consider putting both in the same table with a type column.  Then Kudos would have to do only one join.

You don't include the various table definitions.  If there's too much stuff in Comments and Discussions for a simple merge, you could also consider refactoring them into three tables.  The ID column and author could be set in one table while the other two tables could hold the comment and discussion specific content respectively.  I would consider a merge to be a better solution than this if possible.  This is a backup suggestion.


Stack implementation in C I have to solve an exercise from ANSI C book:

  Add the commands to print the top elements of the stack without
  popping, to duplicate it, and to swap the top two elements. Add a
  command to clear the stack.

Here is my implementation in RPN Calculator:

#include <ctype.h>
#include <stdio.h>
#include <stdlib.h>

#define MAXOP 100
#define NUMBER '0'
#define MAXVAL 100
#define BUFSIZE 100

int getop(char []);
void push(double);
double pop(void);
double top(void);
void clear(void);
void swap(void);
void duplicate(void);

int getch(void);
void ungetch(int);

int sp = 0;
double val[MAXVAL];
char buf[BUFSIZE];
int bufp = 0;

int main(void) {
    int type;
    double op2;
    char s[MAXOP];

    while ((type = getop(s)) != EOF) {
        switch (type) {
        case NUMBER:
            push(atof(s));
            break;
        case '+':
            push(pop() + pop());
            break;
        case '*':
            push(pop() * pop());
            break;
        case '-':
            op2 = pop();
            push(pop() - op2);
            break;
        case '/':
            op2 = pop();
            if (op2 != 0.0)
                push(pop() / op2);
            else
                printf("error: zero divisor\n");
            break;
        case '%':
            op2 = pop();
            if (op2 != 0)
                push((int) pop() % (int) op2);
            else
                printf("error: zero divisor\n");
            break;
        case 'c':
            clear();
            printf("stack empty!\n");
            break;
        case 'd':
            duplicate();
            printf("duplicate element\n");
            break;
        case 's':
            swap();
            printf("swap elements\n");
            break;
        case '?':
            printf("top element on the stack: %f\n", top());
            break;
        case '\n':
            printf("\t%.8g\n", pop());
            break;
        default:
            printf("error: unknown command %s\n", s);
            break;
        }
    }
    return 0;
}

void push(double f) {
    if (sp < MAXVAL)
        val[sp++] = f;
    else
        printf("error: stack full, can't push %g\n", f);
}

double pop(void) {
    if (sp > 0)
        return val[--sp];
    else {
        printf("error: stack empty\n");
        return 0.0;
    }
}

double top(void) {
    if (sp > 0)
        return val[sp - 1];
    else {
        printf("error: stack empty\n");
        return 0.0;
    }
}

void clear(void) {
    if (sp > 0) {
        while (--sp > 0);
    } else {
        printf("error: stack empty\n");
    }
}

void duplicate(void) {
    if (sp > 0) {
        int elem = val[sp - 1];
        push(elem);
    } else {
        printf("error: stack empty\n");
    }
}

void swap(void) {
    if (sp > 0 && (sp - 1) > 0) {
        int elem1 = val[sp - 1];
        int elem2 = val[sp - 2];
        val[sp - 1] = elem2;
        val[sp - 2] = elem1;
    } else {
        printf("error: stack empty\n");
    }
}

int getop(char s[]) {
    int i, c;

    while ((s[0] = c = getch()) == ' ' || c == '\t')
        ;
    s[1] = '\0';
    if (!isdigit(c) && c != '.' && c != '-')
        return c;
    i = 0;
    if (c == '-')
        s[++i] = c = getch();
    if (isdigit(c))
        while (isdigit(s[++i] = c = getch()))
            ;
    if (c == '.')
        while (isdigit(s[++i] = c = getch()))
            ;
    s[i] = '\0';
    if (c != EOF)
        ungetch(c);
    return NUMBER;
}

int getch(void) {
    return (bufp > 0) ? buf[--bufp] : getchar();
}

void ungetch(int c) {
    if (bufp >= BUFSIZE)
        printf("ungetch: too many characters\n");
    else
        buf[bufp++] = c;
}

What are your opinions about it? Is it proper and optimal way to implement these functions?

Your clear function uses while (--sp > 0); when sp = 0; does the same thing in faster and clearer way. also consider not throwing an error when clearing an empty stack, because it still behaves as expected.
The condition sp > 0 && (sp - 1) > 0 is esentially just sp > 1 or sp >= 2, which I would prefer, but that is just a matter of personal style.
Instead of using val[sp-1] to get the top element of the array (in swap and duplicate) use the top() function, as it is a good idea to express your stack operations in terms of more primitive stack operations.
Follwing the concept of the defining operation in terms of primitive operations, duplicate can be written as:

void duplicate(void) {
    push(top());
}

and swap can be written as:

void swap(void) {
    int elem1 = pop();
    int elem2 = pop();
    push(elem1);
    push(elem2);
}

Notice how there is no error checking in these functions,
because push(), pop(), and top() handle the full or empty
stack for us.


Iterator over row and columns I am writing a simple raytracer just for fun. I want to iterate over rows, and each row must be an iterator over columns. A strategy I produced is the following:

class ViewPlane(object):
    def __init__(self, resolution, pixel_size, gamma):
        self.resolution = resolution
        self.pixel_size = pixel_size
        self.gamma = gamma

    def __iter__(self):

        def genR():
            class IterRow(object):
                def __init__(self, plane, row):
                    self.plane = plane
                    self.row = row

                def __iter__(self):
                    def genC():
                        for column in xrange(self.plane.resolution[1]):
                            origin = (self.plane.pixel_size *
                                       (column - self.plane.resolution[0] / 2.0 + 0.5)
                                        ,
                                        self.plane.pixel_size *
                                         (self.row - self.plane.resolution[1] / 2.0 + 0.5)
                                        ,
                                        100.0
                                     )
                            yield ( Ray(origin = origin, direction = (0.0,0.0,-1.0)),
                                    (self.row,column)
                                )
                        return
                    return genC()
                def __str__(self):
                    return "Row iterator on row "+str(self.row)
            for row in xrange(self.resolution[0]):
                yield IterRow(self, row)

        return genR()

Briefly stated: ViewPlane contains the pixel plane. This class implements __iter__ which defines and returns a generator function. This generator function yields instances of an internal class IterRow, which in turn has a __iter__ method that returns a generator iterating over the columns.

I personally think that it's difficult to understand, so I was wondering if there's a cleaner strategy to achieve the same result.

class Foo:
  def __iter__(self):
     def innergen():
        yield something
     return innergen()

Is the same as:

class Foo:
  def __iter__(self):
     yield something

In other words, there is no need to define the generator, just yield the values.

Defining a class inside the method just serves to make things complicated and hard to read. Define the class outside of the method.

Having return at the end of a function doesn't do anything and makes it look like you don't know what you are doing. (I'm not saying that you don't, but whenever you include code with effect it raises a red flag. Did you think it do something?)

Unless you really need to access the data row by row I suggesting flattening the iterator.

Stylistically, I prefer to break expressions up rather then trying to split them across several lines as you've done. I think it produces less clutter.

Here is my resulting code:

class ViewPlane(object):
    def __init__(self, resolution, pixel_size, gamma):
        self.resolution = resolution
        self.pixel_size = pixel_size
        self.gamma = gamma

    def __iter__(self):
        for row in xrange(self.resolution[0]):
            for column in xrange(self.resolution[1]):
                origin_x = self.pixel_size * self.resolution[0] / 2.0 + 0.5
                origin_y = self.pixel_size * self.resolution[1] / 2.0 + 0.5
                origin = (origin_x, origin_y, 100)
                ray = Ray(origin = origin, direction = (0.0, 0.0, -1.0) )

                yield ray, (row, column)

I'm not familiar enough with RayTracers to know but I question having the iterator over ViewPlane produce Ray objects. An iterator is for producing the contents of the object. Is a really contained inside the ViewPlane? Or should the iterator produce coordinates and let the caller construct the Ray objects?

EDIT

Here is a version that doesn't flatten the iterator:

class ViewPlane(object):
    def __init__(self, resolution, pixel_size, gamma):
        self.resolution = resolution
        self.pixel_size = pixel_size
        self.gamma = gamma

    def row_iter(self, row):
        for column in xrange(self.resolution[1]):
            origin_x = self.pixel_size * self.resolution[0] / 2.0 + 0.5
            origin_y = self.pixel_size * self.resolution[1] / 2.0 + 0.5
            origin = (origin_x, origin_y, 100)
            ray = Ray(origin = origin, direction = (0.0, 0.0, -1.0) )

            yield ray, (row, column)

    def __iter__(self):
        for row in xrange(self.resolution[0]):
            yield self.row_iter(row)


Producer-consumer code The idea I have in my head is this:

I am not sure if I need the index variable in consumer and producer.

Moreover, I want to know if it is OK like this (it seems to work), or it is just some stupid working code. Can it be more clever/efficient? Leave the readability for me.

My code will use the POSIX semaphore mutex in order to access the CS (which is writing/reading from the shared buffer of words).

full and empty are there to indicate if the buffer is full, or empty, so that we don't write a word if the buffer is full (we will overwrite the first word if we do that, before consumer reads it). Similarly, we don't want to read from any empty buffer!

When the producer is done, it sets a shared flag (initialized in 0). However, in the consumer side I had to check not only for the flag but for the values of full and empty.

The size of the buffer is 15. n (and the typo m) in the picture below is in our case 15.

Producer:

int index = 0;
size_t str_len;
while (fgets(buf, sizeof(buf), fp) != NULL) {
  str_len = strlen(buf);
  if(buf[str_len - 1] != '\n')
    printf("Didn't read whole line!\n");
  else {
    buf[str_len - 1] = '\0';
    sem_wait(&(sh->empty));
    sem_wait(&(sh->mutex));
    printf("|%s|\n", buf);
    strcpy(sh->words[index], buf);
    index = (index + 1) % N;
    sem_post(&(sh->mutex));
    sem_post(&(sh->full));
  }
}
sem_wait(&(sh->mutex));
sh->done = 1;
sem_post(&(sh->mutex));
int full_val;
sem_getvalue(&(sh->full), &full_val);
while(full_val++ <= 0) { // 'if' instead of 'while' should be OK too
  sem_post(&(sh->full));
}

Consumer:

int index = 0;
while(1) {
  sem_wait(&(sh->full));
  sem_wait(&(sh->mutex));
  int full_val, empty_val;
  sem_getvalue(&(sh->full), &full_val);
  sem_getvalue(&(sh->empty), &empty_val);
  if(empty_val != 15)
    printf("got: %s\n", sh->words[index]);
  if(sh->done == 1 && full_val == 0)
    break;
  index = (index + 1) % N;
  sem_post(&(sh->mutex));
  sem_post(&(sh->empty));
}

I am obliged to use sem_wait() and sem_post() by the way.

After the answers, I think that the main question is that if I can avoid the extra post in full semaphore after the while of producer is ended.

The problem if I remove the posting of full is in the following case:

Producer puts all the words in the buffer.
Scheduler gives the CPU to consumer.
Consumer reads all the words and gets suspended because on the full
semaphore.
Now, scheduler gives the CPU to producer which now sets the shared
flag to 1.
Scheduler goes back to consumer, which is suspended on full
semaphore, forever.

Robust code does not assume the first char in buf is not '\0'.

This is a pedantic point, but user IO is a hacker paradise - best to code defensively.

Instead of:

size_t str_len;
while (fgets(buf, sizeof(buf), fp) != NULL) {
  str_len = strlen(buf);
  if(buf[str_len - 1] != '\n')
    printf("Didn't read whole line!\n");
  else {
    buf[str_len - 1] = '\0';

use to prevent undefined behavior (UB) with buf[str_len - 1].

while (fgets(buf, sizeof(buf), fp) != NULL) {
  size_t str_len = strlen(buf);
  if (str_len == 0 || buf[str_len - 1] != '\n') {
    printf("Didn't read whole line or embedded null character detected!\n");
  } else {
    buf[str_len - 1] = '\0';


Mersenne Primes Generator in C# I've made a program in C# that generates Mersenne numbers starting from the current largest known Mersenne prime. It then calculates the modulo between those numbers and smaller ones starting from 3 in order to figure out if it is divisible.

I've used some math tricks to make the process pretty fast, and so I can check the divisibility of what is basically a multi-million digit number against small numbers several times per second. However, I'd like to see if I can make it faster.

Also, if my program does work, it would suggest that the current largest known Mersenne prime (\$2^{74207281} - 1\$) is actually divisible by 7. Therefore I'm hesitant to say that my program actually does work, so if anyone can confirm this for me, I'd also greatly appreciate that!

using System;
using System.IO;
using System.Numerics;

namespace Prime
{
    public class Prime
    {
        const double C = 10601040; //this is a constant that I divide the exponent by to split it into the easiest possible chunks to work with
        static int exp = 74207281;
        static BigInteger big = BigInteger.Pow(2, exp) - 1;
        static TextWriter tw = new StreamWriter(@"prime.txt"); //writes confirmed primes to a text file

        public static void Main()
        {
            for (int i = exp; true; i++) //this loop infinitely runs the prime checker, incrementing the exponent as it goes
            {
                exp++;
                primes(i);
            }
        }

        private static void primes(int exp)
        {
            double div = Math.Floor(exp/C),
                exp1 = div,
                exp2 = exp - (exp1*C), //this is where the exponent is divided into chunks
                result = 0; //anywhere this appears is just to make extra sure that the result stays zero outside the following loop, where the result really matters

            for (long i = 3; i <= (big/i); i+=2)
            {
                double result1 = (Math.Pow(2, exp1) % i)*C;
                double result2 = (Math.Pow(2, exp2) % i);
                result = (((result1 * result2) - 1) % i);
                if (result == 0)
                {
                    Console.WriteLine(exp + ", not prime.");
                    return;
                }
                Console.WriteLine(i);
                result = 0;
            }
            Console.WriteLine(exp);
            tw.WriteLine(exp);
        }
    }
}

I modulo the chunks of the tested number against odd numbers because a Mersenne number will never be even, therefore it's a waste to check for that. Also, I made the stopping point for the loop dynamic so that any extra numbers can be cut out.
It's like this: 377 isn't divisible by 12, so we know that if it is divisible by something, it will be less than \$\frac{377}{12}\$, which is about 31.4. 377's actual factors are 13 and 29 (aside from itself and 1), which are both less than 37.7.

The technique used to modulo these massive numbers is known as modular exponentiation. It works for all numbers. Here's an example.

$2^7, C = 3, I = 5$

$2^7 = 128$

$Mersenne number being tested: 2^7 – 1 = 127 (this is prime)$

$Exp1 = 2$

$Exp2 = 1$

$Result1 = (2^2 % 5) * 3 = (2^2 % 5) * (2^2 % 5) * (2^2 % 5) = 4 * 4 * 4 = 64$

$Result2 = (2^1 % 5) = 2$

$Result = (  (  (64 * 2) – 1) % 5) = (  (  128 – 1) % 5) = (127 % 5) = 2$

Bug

This line seems wrong:

double result1 = (Math.Pow(2, exp1) % i)*C;

It seems like you are implying that:

\$x^y \mod m = ((x \mod m)*y) \mod m\$

But I believe the actual relationship is:

\$x^y \mod m = (x \mod m)^y \mod m\$

This probably explains why your program thinks the known Mersenne prime is divisible by 7.

Loop termination, or lack of it

Your loop is like this:

for (long i = 3; i <= (big/i); i+=2)//***comment below

But big is \$2^{74207281} - 1\$.  Therefore, you intend your loop to run until i reaches \$\sqrt {2^{74207281}-1}\$, meaning i needs to reach about \$2^{37103640}\$.  Unfortunately, your loop index is a long and not a BigInteger, so it can't even represent the numbers you need to represent.  But even if you made i a BigInteger, the loop would run virtually forever because you would need to reach that huge number before you could stop.


LeetCode ZigZag helper function code The string "PAYPALISHIRING" is written in a zigzag pattern on a given number of rows like this: (you may want to display this pattern in a fixed font for better legibility)
P   A   H   N
A P L S I I G
Y   I   R

And then read line by line: "PAHNAPLSIIGYIR"
Write the code that will take a string and make this conversion given a number of rows:
string convert(string s, int numRows);

I used a helper function approach after realizing that the "Grid" needs to be updated in the same manner over and over again

I have received feedback that storing a grid may take too much memory

Would a "mathematical" approach actually be more readable? And would it be something that can be though of within 20ish minutes (interview time)

I have many if statements that are used to exit out of loops to avoid array index errors

Any feedback or criticism is appreciated - nitpick on anything that seems off

glaring issue is that this doesn't handle edge cases well. For example: "A 2"

https://leetcode.com/problems/zigzag-conversion/
class Solution:
    def convert(self, s: str, numRows: int) -> str:
        if numRows == 1:
            return s
        row = [''] * (len(s) // 2)
        self.s = s
        self.numRows = numRows
        self.final_grid = []

        for i in range(numRows):
            self.final_grid.append(list(row))

        self.addElements(0, 0)

        return "".join(map("".join, self.final_grid))

    def addElements(self, count, column):
        print(column, "beg")
        for i in range(self.numRows):
            if count > len(self.s) - 1:
                break
            self.final_grid[i][column] = self.s[count]
            count += 1

        for i in range(1, self.numRows - 1):
            if count > len(self.s) - 1:
                break

            self.final_grid[self.numRows - i - 1][column + i] = self.s[count]
            count += 1
            print(column, "end")
            print(column + self.numRows - 1)

        if count < len(self.s) - 1:
            self.addElements(count, (column + self.numRows - 1))

As you pointed out, you don't actually need to lay the characters out into a grid.  It would be more efficient to use arithmetic to figure out the indexes of the characters in each row.

For example, if num_rows is 3, then each down-up cycle will consist of 4 characters.  (zigzag_size = 2 * num_rows - 2).  Then we also know that the top of each zigzag will consist of indexes 0, 4, 8, 12, … of the string (for z in range(0, len(s), zigzag_size)).

The middle row will consist of indexes z+1 and z+3.  The bottom row will consist of indexes z+2.  The zigzag_indexes() generator generalizes those calculations for any numRows.

import math

def zigzag_indexes(num_rows):
    yield (0, math.inf)                             # Top of each zigzag
    yield from zip(
        range(1, num_rows),                         # Downward
        range(2 * num_rows - 3, num_rows - 1, -1)   # Upward
    )
    yield (num_rows - 1, math.inf)                  # Bottom of each zigzag

def convert(s, num_rows):
    if num_rows == 1:
        return s
    zigzag_size = 2 * num_rows - 2
    return ''.join(
        (s[z+a] if z+a < len(s) else '') + (s[z+b] if z+b < len(s) else '')
        for a, b in zigzag_indexes(num_rows)
        for z in range(0, len(s), zigzag_size)
    )

print(convert('PAYPALISHIRING', 3))


Individual collapsible areas with accessibility in mind I've been learning Javascript by creating reusable individually collapsible areas (not using jQuery) and I was wondering if someone could review it and let me know if there is anything they would do differently?

Also it would be good if it was usable for screen readers so any usability feedback would be appreciated. If this requires using more CSS and less JS then examples would be helpful.

https://codepen.io/mandrewnz/pen/QgKyjq

HTML

Title

Content goes here

        <button class="js-collapsible-btn">Click to show some text</button>

Suprise

JS

/* adding classlist.js polyfill separately */

var btn = document.getElementsByClassName('js-collapsible-btn');

function findAncestor (el, cls) {
    while ((el = el.parentElement) && !el.classList.contains(cls));
    return el;
}

var openHolder = function(e) {
    var region = findAncestor(this, 'js-collapsible-region'),
        hiddenText = this.childNodes[1];

    // set text to reflect collapsible holder status
    hiddenText.textContent == "(collapsed)" ? hiddenText.textContent = "(expanded)" : hiddenText.textContent = "(collapsed)";

    if (region.classList.contains('closed')) {
        region.classList.remove('closed');
        region.classList.add('open');
    } else {
        region.classList.remove('open');
        region.classList.add('closed');
    }
};

// Create visually hidden text in each link to indicate state
for (var i = 0; i < btn.length; i++) {
    var span = document.createElement('span');
    var currentBtn = btn[i];

    span.classList.add('visuallyhidden');

    span.innerHTML = '(collapsed)';
    currentBtn.appendChild(span);

    // add click event to open holder
    btn[i].addEventListener('click', openHolder, false);
}

Everything looks great, and I can see what you are trying to achieve.

Only things I can recommend is to clean your code, make it as simple as possible, to make it easy to read and come back on later.

For example this line here:

while ((el = el.parentElement) && !el.classList.contains(cls));

In the way it is used, it's okay, but it doesn't explain well what the intention is, instead, try to be more "if this, then", very straight forward. Instead of using a while you could use recursion instead, removing more confusion.

Another thing is, you may want to expand your variable names. You probably will put this code through a minifier either way, so no need to keep things short.

function findAncestor(element, findClass) {
   if (element.classList.contains(findClass)) {
      return element;
   }
   if (!element.parentElement) {
      // Did we hit body?
      return false;
   }
   // We didn't find it, so check the parent.
   return findAncestor(element.parentElement, findClass);
}

Now we can read this function, and know exactly whats happening, without making any assumptions.

The next function is great, and works well, but we could make this more accessible using the [hidden] attribute along with a .hidden class, this is so we don't need to worry about compatibility.

We only want to use the parent as a light wrapper, from that parent we can find the child we actually want to show/hide

You can do a bit of reading on [hidden] here
https://www.paciellogroup.com/blog/2012/05/html5-accessibility-chops-hidden-and-aria-hidden/

function openHolder() {
   var region = findAncestor(this, 'js-collapsible-region');
   var content = region.querySelector('.collapsible-area');

   if (content.classList.contains('hidden')) {
      content.classList.remove('hidden');
      content.removeAttribute('hidden');
   } else {
      content.classList.add('hidden');
      content.setAttribute('hidden', '');
   }
}

We also want to leave state out of the DOM if it isn't visual state (like we can check if something is hidden, but we could also track that another way).

We then simply need to add our even listeners on like we did before:

var btn = document.getElementsByClassName('js-collapsible-btn');
for (var i = 0; i < btn.length; i++) {
    // add click event to open holder
    btn[i].addEventListener('click', openHolder, false);
}

We can also change this to a "functional" approach to remove the loop (which we needed to handle manually)

Array.from(document.getElementsByClassName('js-collapsible-btn'))
   .forEach(function(element) {
      element.addEventListener('click', openHolder, false);
   });

The last thing we should do, is instantly tell the screen reader is hidden but having the content marked as hidden to start with:

Title

Content goes here

        <button class="js-collapsible-btn">Click to show some text</button>

Suprise

Title

Content goes here

        <button class="js-collapsible-btn">Click to show some text</button>

Suprise

We also will need to change the styles to match

.js-collapsible-region .collapsible-area {
    overflow: hidden;
}

.js-collapsible-region .collapsible-area.hidden {
    height: 0px !important;
}

.js-collapsible-region .collapsible-area:not(.hidden) {
    transition: height .3s linear;
}

You should note that the attribute hidden applies the styles display: none, which you may not want, so you can change your styles to override the way you want them to work.

Here is the resulting codepen https://codepen.io/anon/pen/KqgmNY

For some fun, since we are using ES6 with classList anyway, have a look at this version https://codepen.io/anon/pen/mwrmpL


How to determine the order of letters by regex? I want to write a regular expression to determine whether a given string is uppercase and sorted in non-descending order.

^A*B*C*D*E*F*G*H*I*J*K*L*M*N*O*P*Q*R*S*T*U*V*Q*X*Y*Z*$

I was just wondering if it is possible to improve the above regex.

Examples of correct strings:

  AEHIIEFKLZ

Examples of wrong strings:

  AbCDE YABKL

There is no shorter solution. Because you are enforcing order, you must list every single entity in desired order. And because you are allowing zero to many instances of each entity, you must provide the * operator. And because you want to match the entire string, you must provide the anchors on both ends.

Could you write one that runs faster? Possibly. But shorter? I don't see how.


Linear regression on points specified by the user in a GUI Here's my take on the Linear Regression exercise from John Zelle's Python book.

  Open a window, let the user click on as many points as they want. Upon pressing the "Done" button the regression line of those points is drawn.

from graphics import *

def dots(win):
    sumx = 0
    sumy = 0
    sumx2 = 0
    sumxy = 0
    count = 0

    click = win.getMouse()
    clickX = click.getX()
    clickY = click.getY()

    while (clickX < 31 or clickX > 39 or
                clickY < 1 or clickY > 5):
            sumx += clickX
            sumy += clickY
            sumx2 += clickX**2
            sumxy += clickX*clickY
            count += 1

            Point(clickX, clickY).draw(win)

            click = win.getMouse()
            clickX = click.getX()
            clickY = click.getY()

    return sumx, sumy, sumx2, sumxy, count

def mean(sum, count):
    mean = sum / count
    return mean

def calcreg(sumx, sumy, sumx2, sumxy, count):
    xleft = 0
    xright = 40

    m = (sumxy - count * mean(sumx, count) * mean(sumy, count)) / (sumx2 - count * mean(sumx, count)**2)

    yleft = mean(sumy, count) + m * (xleft - mean(sumx, count))
    yright = mean(sumy, count) + m * (xright - mean(sumx, count))

    return Line(Point(xleft, yleft), Point(xright, yright))

def main():
    win = GraphWin("Regression Line", 400, 400)
    win.setCoords(0, 0, 40, 40)

    Text(Point(35, 3), "DONE").draw(win)
    Rectangle(Point(31, 1), Point(39 ,5)).draw(win)

    sumx, sumy, sumx2, sumxy, count = dots(win)

    try:
        regline = calcreg(sumx, sumy, sumx2, sumxy, count)
        regline.draw(win)
    except ZeroDivisionError:
        Text(Point(20, 20), "Enter at least 2 points!").draw(win)

    win.getMouse()

main()

Any tips on what I should/could change here?

from <xx> import *

Prevent a * import. You don't know what it will inject in your module namespace. Instead only import the parts you need:

from graphics import GraphWin, Line, Point, Rectangle, Text

The google style guide as linked by Mircea also says not to import the classes, but only modules and packages, but that is a matter of style and taste.

Magic numbers

There are a lot of magic numbers, scattered in your code. If ever you want to change the location of the button, you will need to adapt that in a few places, and will most likely miss some. To prevent such magic numbers, you can do something like this:

def main():
    coord_max = 40
    win = GraphWin("Regression Line", 10 * coord_max, 10 * coord_max)

    win.setCoords(0, 0, coord_max, coord_max)

    button_x, button_y = 35, 3
    button_width, button_height = 8, 4

    Text(Point(button_x, button_y), "DONE").draw(win)
    button = Rectangle(
        Point(button_x - button_width / 2, button_y - button_height / 2),
        Point(button_x + button_width / 2, button_y + button_height / 2),
    )
    button.draw(win)

split functionality

your dots method does a lot of things now. It:

catches the click
checks whether it it is the button
draws the point
does the summary calculations

Especially the last one should be factored to its own method. The easiest way to do this is let dots just yield the x and y coordinate, and let another method take care of the regression.

Checking whether the click is on the button can also be done simpler. Instead of hardcoding the coordinates, simplest would be to pass in the button object, and ask that one for its bounding coordinates:

def dots(win, button):

    while True:

        click = win.getMouse()
        x = click.getX()
        y = click.getY()
        if button.p1.x <= x <= button.p2.x and button.p1.y <= y <= button.p2.y:
            break # button press
        Point(x, y).draw(win)
        yield x, y

Is to me a lot clearer and easier to understand.

If you don't know that p1 and p2 are bottomleft and topright, you can take that into account by doing:

def dots(win, button):
    while True:
        click = win.getMouse()
        x = click.getX()
        y = click.getY()

        x1, x2 = sorted((button.p1.x, button.p2.x))
        y1, y2 =  sorted((button.p1.y, button.p2.y))
        if x1 <= x <= x2 and y1 <= y <= y2: # button press
            break
        Point(x, y).draw(win)
        yield x, y

summarize:

Instead of each summary metric getting its own variable, you can save them in a dict (or a collections.Counter more specifically)

def summarize(points):
    summary = Counter()
    for x, y in points:
        summary["x"] += x
        summary["y"] += y
        summary["x2"] += x ** 2
        summary["xy"] += x * y
        summary["count"] += 1
    summary["mean_x"] = summary["x"] / summary["count"]
    summary["mean_y"] = summary["y"] / summary["count"]
    return summary

regression:

Then pass this on to a method that calculates the regression line. This needs the maximum x of the screen as an argument:

def regression_line(summary, x_max, x_min=0):
    slope = (
        summary["xy"]
        - summary["count"] * summary["mean_x"] * summary["mean_y"]
    ) / (summary["x2"] - summary["count"] * summary["mean_x"] ** 2)

    y_min = summary["mean_y"] + slope * (x_min - summary["mean_x"])
    y_max = summary["mean_y"] + slope * (x_max - summary["mean_x"])

    return Line(Point(x_min, y_min), Point(x_max, y_max))

rest of main

points = dots(win, button=button)
summary = summarize(points)
try:
    regline = regression_line(summary, x_max=coord_max)
except ZeroDivisionError:
    middle_point = Point(coord_max / 2, coord_max / 2)
    Text(middle_point, "Enter at least 2 points!").draw(win)
else:
    regline.draw(win)
win.getMouse()

main guard

If you put the calling of main() behind if __name__ == "__main__":, you can later import this module from somewhere else, and reuse some of the code

if __name__ == "__main__":
    main()


Importing JPG, JPEG, and PNG images from a folder to an Excel worksheet As I am a novice, I was keen to solve the task, so now I want to optimize the code.

Sub AddOlEObject()

    Dim mainWorkBook As Workbook

    Set mainWorkBook = ActiveWorkbook
    Sheets("SingleProfile").Activate
    Folderpath = "C:\Users\sandeep.hc\Pics"
    Set fso = CreateObject("Scripting.FileSystemObject")
    NoOfFiles = fso.GetFolder(Folderpath).Files.Count
    Set listfiles = fso.GetFolder(Folderpath).Files
    For Each fls In listfiles
       strCompFilePath = Folderpath & "\" & Trim(fls.Name)
        If strCompFilePath <> "" Then
            If (InStr(1, strCompFilePath, "jpg", vbTextCompare) > 1 _
            Or InStr(1, strCompFilePath, "jpeg", vbTextCompare) > 1 _
            Or InStr(1, strCompFilePath, "png", vbTextCompare) > 1) Then
                 counter = 29
                 counter1 = counter1 + 1

                Call insert(strCompFilePath, counter, counter1)
                'Sheets("SingleProfile").Activate
                counter1 = counter1 + 17
            End If
        End If
    Next
mainWorkBook.Save
End Sub

Function insert(PicPath, counter, counter1)
'MsgBox PicPath
    With ActiveSheet.Pictures.insert(PicPath)
        With .ShapeRange
            .LockAspectRatio = msoFalse
            .Width = 875
            .Height = 300
        End With
        .Left = ActiveSheet.Cells(counter, counter1).Left
        .Top = ActiveSheet.Cells(counter, counter1).Top
        .Placement = 1
        .PrintObject = True
    End With
End Function

Based on Comments Optimized code

Sub AddImage2()
Dim rgTarget As Range
Dim RowI As Long, ColumnI As Long

    Folderpath = "C:\Users\sandeep.hc\Pics"
    Set fso = CreateObject("Scripting.FileSystemObject")
    NoOfFiles = fso.GetFolder(Folderpath).Files.Count
    Set listfiles = fso.GetFolder(Folderpath).Files
    For Each fls In listfiles
       strCompFilePath = Folderpath & "\" & Trim(fls.Name)
        If strCompFilePath <> "" Then
            If (InStr(1, strCompFilePath, "jpg", vbTextCompare) > 1 _
            Or InStr(1, strCompFilePath, "jpeg", vbTextCompare) > 1 _
            Or InStr(1, strCompFilePath, "png", vbTextCompare) > 1) Then
    RowI = 29
    ColumnI = ColumnI + 1
    Set rgTarget = Cells(RowI, ColumnI)
    Application.ActiveSheet.Shapes.AddPicture strCompFilePath, False, True, rgTarget.Left, rgTarget.Top, 875, 400
    ColumnI = ColumnI + 17
    End If

End If
Next
End Sub

I will summarize what I said before (and add a few new ones):

Use Option Explicit at the top of your module. It helps you to prevent mistakes and forces you to declare the variables
instead of Instr use Right as in If Right(strCompFilePath,4)=".jpg"
rename counter and counter1 so it is clear what they are (row and column indices)
Declare insert as a Sub and not a Function.
instead of passing the row and column index to the insert function/sub and then using the ActiveSheet pass the cell as a Range to the insert function/sub. (Instead of Activesheet you can use cell.Worksheet to get the right sheet)
possibly rename insert so you don't confuse it with Pictures.insert
add the option savewithfile := true to the Pictures.Insert method so the pictures will stay in the file even if you send it somewhere.
as it is now, you don't really need NoOfFiles and mainWorkBook
instead of the FileSystemObject I would use Dir. See this for more on that.